pull request for Espeak pitch range fork

[Luke Yelavich]

On Fri, Oct 17, 2014 at 11:15:49AM EDT, Hussain Jasim wrote:
> Hi, I couldn't find anywhere on the website to do this.

I am aware of the features github provides for proposing changes to a 
repository, however at this time, the Speech Dispatcher project does not have a 
presence on github, so the best way to contribute changes is via the mailing 
list. The website does not go into detail as to the best way to propose 
changes, so I will see what can be done about clarrifying this.

> I've pollished up my Speech Dispatcher fork to add support for
> Espeak's pitch range (AKA inflection) control, and I'd like to get it
> merged into upstream. I've been using it for a while now on my system,
> and no issues have come up.

Thanks for your work. Pulling from a branch from a maintenance point of view 
may be a little easier, however it makes it harder for code review. This 
doesn't appear to be a large patch set, so in future, please consider sending 
such changes as a patch set to the mailing list, to allow for easier commenting 
on changes in the diff. As above, I will see what I can do about making this 
clearer on the website. I suggest you read the man pages for the git 
format-patch and git send-email commands, which will give you information on 
how to create a patch set, and send them via email to the list.

Firstly, the 2 most recent commits in your changes, commits 
f7e97dff618ae0c7ade00ff7b7958411a88e1cca and 
0022c0709be4bdbd886e0ff08d1ba46096cb81c3 respectively. From the description of 
the commits, I can only guess that you were attempting to update your 
repository to be in sync with more recent changes from the main Speech 
Dispatchre git repository. There are a couple of ways that you could do this, 
either with the git merge command, or the git rebase command. Git rebase in 
this instance is preferable, because it pulls the latest changes from the main 
Speech Dispatcher repository, and attempts to put your own commits on top of 
the latest Speech Dispatcher changes. I suggest you have a read of the git 
rebase man page to learn more. The commits I mentioned actually revert some of 
the changes you made in previous commits.

I haven't tested the code yet, but it looks ok to me so far at a glance. 
However, in git commit 24d551d421ef0a8aa6cebce431c73b1853edbdc7, in 
src/server/msg.h I see you changed the return values for a lot of the message 
return strings. I understand that you thought it best to keep the pitch related 
return strings logically grouped in the file, and that is ok, however it is 
wiser to assign a return value that is greater than all other return values 
already in use. There are other bindings and clients of Speech Dispatcher that 
do not use the C library interface or the python bindings. These clients and 
other language bindings talk to Speech Dispatcher directly via the SSIp 
protocol. These clients and bindings may very well look for specific return 
values when communicating, so bumping the return values as you did would likely 
break some client or bindings in a bad way.

As for the autotools files, please make sure you clean your repo of any build 
files before committing. In the meantime, I will see about adding autotools 
build files to .gitignore such that they don't get included accidentally in the 
future.

At this point, I don't expect you to totally revise your branch and fix things 
up. I see that you are somewhat unfamiliar with git, and how contributions are 
made to Speech Dispatcher, so if you could consider my advice for the future, 
that would be appreciated.

I will cherry-pick your changes into a local branch, and test them. I will get 
back to you with further information once I have tested things myself and 
looked at the code in greater detail.

thanks again for your contribution.

Luke