Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 4, 2016 at 12:13 AM, Mehul Jain <mehul.jain2029@xxxxxxxxx> wrote:
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 10eff03..b338b83 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -85,6 +85,7 @@ static char *opt_squash;
>  static char *opt_commit;
>  static char *opt_edit;
>  static char *opt_ff;
> +static int opt_autostash = -1;
>  static char *opt_verify_signatures;
>  static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
>  static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
> @@ -146,6 +147,8 @@ static struct option pull_options[] = {
>         OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
>                 N_("abort if fast-forward is not possible"),
>                 PARSE_OPT_NOARG | PARSE_OPT_NONEG),
> +       OPT_BOOL(0, "autostash", &opt_autostash,
> +               N_("automatically stash/stash pop before and after rebase")),
>         OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
>                 N_("verify that the named commit has a valid GPG signature"),
>                 PARSE_OPT_NOARG),
> @@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,
>         argv_array_pushv(&args, opt_strategy_opts.argv);
>         if (opt_gpg_sign)
>                 argv_array_push(&args, opt_gpg_sign);
> -

Minor nit: but when I wrote the code for run_rebase() I separated the
options for "Shared options" and "Options passed to git-rebase" into
different code block groups from the other code, and I would like it
if it remained that way :-(

> +       if (opt_autostash)
> +               argv_array_push(&args, "--autostash");

Hmm, interesting. If rebase.autostash=true !opt_autostash, we don't
need to pass --no-autostash to git-rebase because it will only stash
if the worktree is dirty, but a dirty worktree will be caught by
git-pull's die_on_unclean_worktree() anyway.

Still, it may be a problem because the worktree may become dirty
in-between our "worktree is clean" check and when git-rebase is run
(during the git-fetch), and the user may be surprised if git-rebase
attempted to stash in that case.

So we may wish to pass "--no-autostash" to git-rebase as well.

>         argv_array_push(&args, "--onto");
>         argv_array_push(&args, sha1_to_hex(merge_head));
>
> @@ -835,18 +839,25 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 hashclr(orig_head);
>
>         if (opt_rebase) {
> -               int autostash = 0;
> -
>                 if (is_null_sha1(orig_head) && !is_cache_unborn())
>                         die(_("Updating an unborn branch with changes added to the index."));
>
> -               git_config_get_bool("rebase.autostash", &autostash);
> -               if (!autostash)
> +               if (opt_autostash == -1)
> +                       git_config_get_bool("rebase.autostash", &opt_autostash);
> +
> +               if (opt_autostash == 0 || opt_autostash == -1)
>                         die_on_unclean_work_tree(prefix);
>
>                 if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
>                         hashclr(rebase_fork_point);
>         }
> +       else {

Git code style puts the else on the same line, not on a new one.

> +               /* If --[no-]autostash option is called without --rebase */

Yeah, I agree with Eric that this comment should be dropped,

> +               if (opt_autostash == 0)
> +                       die(_("--no-autostash option is only valid with --rebase."));
> +               else if (opt_autostash == 1)
> +                       die(_("--autostash option is only valid with --rebase."));
> +       }

and these error messages combined.

>
>         if (run_fetch(repo, refspecs))
>                 return 1;
> --
> 2.7.1.340.g69eb491.dirty

Regards,
Paul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]