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

From: Mehul Jain <mehul.jain2029@gmail.com>
Date: Mar 16, 2016, 05:00 UTC
Message-ID: <CA+DCAeRWRiY+qCZ440NbU+DMg0OQ6C1bSpYfsHEqjiOvhX69Eg@mail.gmail.com>
In-Reply-To: <CAPig+cSnp+NsBAMib4pExKCLB5ocGsHWyO7qMU0E91WqE6a5_g@mail.gmail.com>

On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 16 quoted lines

>> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
>> @@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
>> +--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.

Show 15 quoted lines

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

Show 15 quoted lines

>> +                       opt_autostash = config_autostash;
>> +
>> +               if (!opt_autostash)
>>                         die_on_unclean_work_tree(prefix);
>>
>>                 if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
>>                         hashclr(rebase_fork_point);
>> -       }
>> +       } else
>> +               if (opt_autostash != -1)
>> +                        die(_("--[no-]autostash option is only valid with --rebase."));
>
> How about formatting this as a normal 'else if'?
>
>     } else if (opt_autostash != -1)

I thought of it earlier but voted against it as it may reduce the readability of the code.

Show 12 quoted lines

> On the other hand, this error case hanging off the 'rebase'
> conditional is somewhat more difficult to reason about than perhaps it
> ought to be. It might be easier to see what's going on if you get the
> error case out of the way early, and then handle the normal case. That
> is, something like this:
>
>     if (!opt_rebase && opt_autostash != -1)
>         die(_("... is only valid with --rebase"));
>
>     if (opt_rebase) {
>         ...
>     }

This is good. I'll make the changes accordingly.

Thanks for the comments.

Mehul