Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Mar 25, 2016, 18:37 UTC
Message-ID: <vpq7fgql7zh.fsf@anie.imag.fr>
In-Reply-To: <CA+DCAeTNv-2RkbGo+ciKP_bfCvThKjGAsJEr=xuBYBFgrTvGtg@mail.gmail.com>

Mehul Jain <mehul.jain2029@gmail.com> writes:

Show 19 quoted lines

> On Fri, Mar 25, 2016 at 2:35 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Mehul Jain <mehul.jain2029@gmail.com> writes:
>>
>>> +--autostash::
>>> +--no-autostash::
>>> +     Before starting rebase, stash local modifications away (see
>>> +     linkgit:git-stash[1]) if needed, and apply the stash when
>>> +     done. `--no-autostash` is useful to override the `rebase.autoStash`
>>> +     configuration variable (see linkgit:git-config[1]).
>>> ++
>>> +This option is only valid when "--rebase" is used.
>>
>> This does not have to be added to this series (I don't want to break
>> everything at v10 ...), but I think it would be nice to allow "git pull
>> --autostash" even without --rebase if pull.rebase=true.
>
> This is a nice observation. As current patch allow "git pull --autostash"
> to be run without --rebase if pull.rebase=true,

OK, I misread the patch assuming that opt_rebase was only reflecting the options, but it is also set by the config:

	if (opt_rebase < 0)
		opt_rebase = config_get_rebase();

> hence correct documentation should be something like this
>
>     This option is only valid when "--rebase" is used or pull.rebase=true.

... or just "when pull is used in rebase mode", which is shorter and still technically accurate. I don't think you need to be exhaustive in this kind of documentation, the user will notice anyway if he tries to use --autostash in a forbidden situation.

Show 5 quoted lines

> But OTOH users who knows about pull.rebase understands that
> pull.rebase=true means "git pull --rebase ..." will be executed whenever
> "git pull ..." is called, thus for those users it might be easy to deduce that
> need of "--rebase" for validity of "--autostash" is not necessary if
> pull.rebase=true.

I'd rather have something technically correct.

I think you should also change one of the tests to use pull.resbase=true so that this behavior is properly tested.

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/