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

> On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +--autostash::
>>> +--no-autostash::
>>> +       Before starting rebase, stash local modifications away (see
>>> +       linkgit:git-stash.txt[1]) if needed, and apply the stash when
>>> +       done (this option is only valid when "--rebase" is used).
>>> ++
>>> +`--no-autostash` is useful to override the `rebase.autoStash`
>>> +configuration variable (see linkgit:git-config[1]).
>>
>> The last couple sentences seem reversed. It feels odd to have the bit
>> about --rebase dropped dead in the middle of the description of
>> --autostash and --no-autostash. I'd have expected to see --autostash
>> and --no-autostash discussed together, and then, as a follow-on,
>> mention them being valid only with --rebase.
>
> So you are suggesting something like this:
>
> --autostash::
> --no-autostash::
>     Before starting rebase, stash local modifications away (see
>     linkgit:git-stash.txt[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.
>
> Can be done and it make more sense to talk about the validity of the
> option in a seperate line.

>>> diff --git a/builtin/pull.c b/builtin/pull.c
>>> @@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>>>                 if (is_null_sha1(orig_head) && !is_cache_unborn())
>>>                         die(_("Updating an unborn branch with changes added to the index."));
>>>
>>> -               if (config_autostash)
>>> +               if (opt_autostash == -1)
>>
>> In patch 1/2, this changed from 'if (autostash)' to 'if
>> (config_autostash)'; it's a bit sad that patch 2/2 then has to touch
>> the same code to change it yet again, this time to 'if
>> (opt_autostash)'. Have you tried just keeping the original 'autostash'
>> variable and modifying its value based upon config_autostash and
>> opt_autostash instead? (Not saying that this would be better, but
>> interested in knowing if the result is as clean or cleaner or worse.)
>
> Yes, I tried that. Things looked something like this:
>
> In patch 1/2
> ...
>
> -    int autostash = 0;
> +    int autostash = config_autoshash;
>
>     if (is_null_sha1(orig_head) && !is_cache_unborn())
>             die(_("Updating ..."));
>
> -    git_config_get_bool("rebase.autostash", &autostash);
>     if (!autostash)
>             die_on_unclean_work_tree(prefix);

> In patch 2/2
> ...
>     int autostash = config_autoshash;
>
>     if (is_null_sha1(orig_head) && !is_cache_unborn())
>             die(_("Updating ..."));
>
> +    if (opt_autostash != -1)
> +        autostash = opt_autostash;

>     if (!autostash)
>         die_on_unclean_work_tree(prefix);
> ...
>
> This implementation looks much more cleaner but we are using some
> extra space (autostash) to do the task. If everyone is fine with this
> trade off then I can re-roll a new patch with this method. Comments please.