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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Mar 15, 2016 at 1:11 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> If rebase.autoStash configuration variable is set, there is no way to
> override it for "git pull --rebase" from the command line.
>
> Teach "git pull --rebase" the --[no-]autostash command line flag which
> overrides the current value of rebase.autoStash, if set. As "git rebase"
> understands the --[no-]autostash option, it's just a matter of passing
> the option to underlying "git rebase" when "git pull --rebase" is called.

The below comments may or may not be worth a re-roll (you decide)...

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> 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.

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

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

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) {
        ...
    }

>         if (run_fetch(repo, refspecs))
>                 return 1;