On Sun, Aug 11, 2013 at 6:39 PM, Stephen Haberman <stephen@xxxxxxxxxxxxxxxx> wrote: > >> 1. i'm not sure why you are testing $3 == preserve. it looks like a >> typo > > Yes, good catch. I've added a test that fails, and will fix that. > >> 2. clearer than a string of yoda conditions: >> >> case $2 in >> true|false|preserve) > > Makes sense, will change. > >> 1. in the error message you say that rebase should be a trystate of >> true, false, or preserve. why then do you allow $rebase == '' ? > > Because it may be unset, like if the user ran "git pull . copy" and > the pull.rebase setting was not set. > >> 2. clearer than a string of yoda conditions: > > Will change again. > > I'll wait to see if I get any more feedback and then will send out > another version. i just realized that there are ambiguities: pull -r (true|false|preserve) foo there are 2 ways to interpret this: pull --rebase=(true|false|preserve) foo # pull from remote named foo pull --rebase (true|false|preserve) foo # pull from remote named (true|false|preserve), branch foo options with optional operands usually require that the operands be concatenated with the option argument, so that pull --rebase[=(true|false|preserve)] | -r[(true|false|preserve)] avoids the ambiguity of pull --rebase [(true|false|preserve)] | -r [(true|false|preserve)] 1. you can make it a disambiguation by appending ? to the optspec (according to man git-rev-parse) 2. you could also disambiguate by testing if the argument is a configured remote and warn the user, but this makes option parsing inconsistent, requires additional logic, and is overall inelegant > > Thanks! > > - Stephen > > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html