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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 15, 2016, 21:43 UTC
Message-ID: <CAPig+cSnp+NsBAMib4pExKCLB5ocGsHWyO7qMU0E91WqE6a5_g@mail.gmail.com>
In-Reply-To: <1458061904-26516-2-git-send-email-mehul.jain2029@gmail.com>

On Tue, Mar 15, 2016 at 1:11 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

Show 7 quoted lines

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

Show 12 quoted lines

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

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

Show 11 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)

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;