Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Junio C Hamano <gitster@pobox.com>]

John Cai <johncai86@gmail.com> writes:

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 100cbf9fb8..fb700c2d39 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -86,7 +86,8 @@ static char *opt_ff;
>  static char *opt_verify_signatures;
>  static char *opt_verify;
>  static int opt_autostash = -1;
> -static int config_autostash;
> +static int rebase_autostash = 0;
> +static int cfg_rebase_autostash;

Do not explicitly initialize statics to '0' (or NULL for that matter).

But more importantly, I have a feeling that you are making a piece
of code that is already hard to read impossible to follow by adding
yet another variable.

> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 66cfcb09c5..28f551db8e 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
>  	test_commit -C src two &&
>  	test_must_fail git -C dst pull --no-ff --no-verify --verify
>  '
> +test_expect_success 'git pull --rebase --autostash succeeds on ff' '

Missing blank line between tests.


I thought that the root cause of the problem is that run_merge() is
called even when rebase was asked for (either via pull.rebase=true
configuration or "pull --rebase" option), when the other side is a
descendant of HEAD.  The basic idea behind that behaviour may be
sound (i.e. if we do not have any of our own development on top of
their history, rebase vs merge shouldn't make any difference while
fast-forwarding HEAD to their history), except that rebase vs merge
look at different configuration variables.

I wonder if the following two-liner patch is a simpler fix that is
easier to understand.  run_merge() decides if we should pass the
"--[no-]autostash" option based on the value of opt_autostash, and
we know the value of rebase.autostash in config_autostash variable.

It appears to pass all four tests your patch adds.

 builtin/pull.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git c/builtin/pull.c w/builtin/pull.c
index 100cbf9fb8..d459a91a64 100644
--- c/builtin/pull.c
+++ w/builtin/pull.c
@@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("cannot rebase with locally recorded submodule modifications"));
 
 		if (can_ff) {
-			/* we can fast-forward this without invoking rebase */
+			/*
+			 * We can fast-forward without invoking
+			 * rebase, by calling run_merge().  But we
+			 * have to allow rebase.autostash=true to kick
+			 * in.
+			 */
+			if (opt_autostash < 0)
+				opt_autostash = config_autostash;
 			opt_ff = "--ff-only";
 			ret = run_merge();
 		} else {