On Wed, Mar 9, 2016 at 12:18 PM, Mehul Jain <mehul.jain2029@xxxxxxxxx> 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.
>
> Helped-by: Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx>
> Helped-by: Junio C Hamano <gitster@xxxxxxxxx>
> Helped-by: Paul Tan <pyokagan@xxxxxxxxx>
> Helped-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
> Signed-off-by: Mehul Jain <mehul.jain2029@xxxxxxxxx>
> ---
> Previous patches: $gname287709
>
> Changes:
> - Slight change is documentation.
>
> Documentation/git-pull.txt | 9 +++++++++
> builtin/pull.c | 16 ++++++++++++++--
> t/t5520-pull.sh | 39 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index a62a2a6..da89be6 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
> --no-rebase::
> Override earlier --rebase.
>
> +--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]).
> +
> Options related to fetching
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 8a318e9..a01058a 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -86,6 +86,7 @@ static char *opt_commit;
> static char *opt_edit;
> static char *opt_ff;
> static char *opt_verify_signatures;
> +static int opt_autostash = -1;
> static int config_autostash = -1;
Hmm, why can't config_autostash just default to 0?
> static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
> static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
> @@ -150,6 +151,8 @@ static struct option pull_options[] = {
> OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
> N_("verify that the named commit has a valid GPG signature"),
> PARSE_OPT_NOARG),
> + OPT_BOOL(0, "autostash", &opt_autostash,
> + N_("automatically stash/stash pop before and after rebase")),
> OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
> N_("merge strategy to use"),
> 0),
> @@ -801,6 +804,10 @@ 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);
> + if (opt_autostash == 1)
> + argv_array_push(&args, "--autostash");
> + else if (opt_autostash == 0)
> + argv_array_push(&args, "--no-autostash");
The precise testing for specific values of -1, 0 and 1 throughout the
code makes me uncomfortable. Ordinarily, I would expect a simple
argv_array_push(&args, opt_autostash ? "--autostash" : "--no-autostash");
Stepping back a bit, the only reason why we introduced opt_autostash =
-1 as a possible value is because we need to detect if
--[no-]autostash is being used with git-merge. I consider that a
kludge, because if git-merge supports --autostash as well (possibly in
the future), then we will not need this -1 value.
So, from that point of view, a -1 value is okay as a workaround, but
kludges, and hence the -1 value, should be gotten rid off as soon as
possible.
>
> argv_array_push(&args, "--onto");
> argv_array_push(&args, sha1_to_hex(merge_head));
> @@ -851,12 +858,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 != 1)
> + if (opt_autostash == -1)
> + opt_autostash = config_autostash;
So, if config_autostash defaults to zero, we can be certain that
opt_autostash will be either true or false.
> +
> + if (opt_autostash != 1)
And then we can do if (!opt_autostash) here, instead of making readers
wonder why we are testing for the value "1" specifically.
> 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."));
>
> if (run_fetch(repo, refspecs))
> return 1;
Thanks,
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