Re: [PATCH] pull: Allow pull to preserve merges when rebasing.

From: Andres Perera <andres.p@zoho.com>
Date: Aug 11, 2013, 23:31 UTC
Message-ID: <CAPrKj1aMURcVoaiJ+WS64ekafUZgSagKrYSknTUk3+TL6tCETQ@mail.gmail.com>
In-Reply-To: <20130811180915.390d660a@sh9>

On Sun, Aug 11, 2013 at 6:39 PM, Stephen Haberman <stephen@exigencecorp.com> wrote:

Show 25 quoted lines

>
>> 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

Show 7 quoted lines

>
> Thanks!
>
> - Stephen
>
>
>