[PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config

[PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config

[John Cai]

On a git pull --rebase, if fast forward is possible we run merge.
However, merge will not honor rebase.autostash if it is configured. This
has the unfortunate result of

$ git config rebase.autostash true
$ git pull --rebase

to ignore the rebase.autostash value.

Allow run_merge() to honor rebase.autostash by passing in
config_autostash if --autostash or --no-autostash flags are not
explicitly set.

Reported-by: "Tilman Vogel" <tilman.vogel@web.de>
Co-authored-by: "Junio C Hamano" <gitster@pobox.com>
Signed-off-by: "John Cai" <johncai86@gmail.com>
---

Notes:
    Fix a bug that prevents git pull --rebase from honoring the rebase.autostash
    config value.

    Changes since V2:
    - fixed Junio's email in trailer

    Changes since V1:
    - used simpler fix as proposed by Junio
    - removed redundant test cases

 builtin/pull.c          |  9 ++++++++-
 t/t5521-pull-options.sh | 12 ++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 100cbf9fb8..8423e420ee 100644
--- a/builtin/pull.c
+++ b/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 {
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 66cfcb09c5..66fac99d2b 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -252,4 +252,16 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
 	test_must_fail git -C dst pull --no-ff --no-verify --verify
 '
 
+test_expect_success 'git pull --rebase --autostash succeeds on ff' '
+	test_when_finished "rm -fr src dst actual" &&
+	git init src &&
+	test_commit -C src "initial" file "content" &&
+	git clone src dst &&
+	test_commit -C src --printf "more_content" file "more content\ncontent\n" &&
+	echo "dirty" >>dst/file &&
+	git -C dst pull --rebase --autostash >actual 2>&1 &&
+	grep -q "Fast-forward" actual &&
+	grep -q "Applied autostash." actual
+'
+
 test_done
-- 
2.34.1

Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config

[Taylor Blau]

On Thu, Jan 06, 2022 at 10:02:26AM -0500, John Cai wrote:
> On a git pull --rebase, if fast forward is possible we run merge.
> However, merge will not honor rebase.autostash if it is configured. This
> has the unfortunate result of
>
> $ git config rebase.autostash true
> $ git pull --rebase
>
> to ignore the rebase.autostash value.
>
> Allow run_merge() to honor rebase.autostash by passing in
> config_autostash if --autostash or --no-autostash flags are not
> explicitly set.
>
> Reported-by: "Tilman Vogel" <tilman.vogel@web.de>
> Co-authored-by: "Junio C Hamano" <gitster@pobox.com>
> Signed-off-by: "John Cai" <johncai86@gmail.com>
> ---
>
> Notes:
>     Fix a bug that prevents git pull --rebase from honoring the rebase.autostash
>     config value.
>
>     Changes since V2:
>     - fixed Junio's email in trailer
>
>     Changes since V1:
>     - used simpler fix as proposed by Junio
>     - removed redundant test cases
>
>  builtin/pull.c          |  9 ++++++++-
>  t/t5521-pull-options.sh | 12 ++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 100cbf9fb8..8423e420ee 100644
> --- a/builtin/pull.c
> +++ b/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;

This looks OK, and prefers the value of autostash given over options
over the configured one. But it may be a little clearer to construct it
that way explicitly (see the conditional "if (opt_rebase)" inside of
cmd_pull()).

>  			opt_ff = "--ff-only";
>  			ret = run_merge();
>  		} else {
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 66cfcb09c5..66fac99d2b 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -252,4 +252,16 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
>  	test_must_fail git -C dst pull --no-ff --no-verify --verify
>  '
>
> +test_expect_success 'git pull --rebase --autostash succeeds on ff' '
> +	test_when_finished "rm -fr src dst actual" &&
> +	git init src &&
> +	test_commit -C src "initial" file "content" &&

A handful of these arguments do not need extra quotes. It would be fine
to instead write:

    test_commit -C src initial file content &&

but if you don't care about the commit message being "content", you can
shorten this to just:

    test_commit -C src content file &&

since test_commit defaults all of its arguments to the value of
"<message>".

> +	git clone src dst &&
> +	test_commit -C src --printf "more_content" file "more content\ncontent\n" &&

Same note here, but I think the `--printf` is unnecessary. Running
`echo` with "\n" characters in its argument is fine, so this could be
shortened to:

    test_commit -C src blah file "more\ncontent"

Unfortunately using `--append` isn't an option here, since the "content"
anchor needs to be at the end of the file in order to apply the stash
without conflict.

Thanks,
Taylor

Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config

[Taylor Blau]

On Thu, Jan 06, 2022 at 12:12:40PM -0500, Taylor Blau wrote:
> On Thu, Jan 06, 2022 at 10:02:26AM -0500, John Cai wrote:
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 100cbf9fb8..8423e420ee 100644
> > --- a/builtin/pull.c
> > +++ b/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;
>
> This looks OK, and prefers the value of autostash given over options
> over the configured one. But it may be a little clearer to construct it
> that way explicitly (see the conditional "if (opt_rebase)" inside of
> cmd_pull()).

Oops, ignore this suggestion. run_merge() looks at the static variable
opt_autostash, so you really do need to be manipulating it directly.

Nevermind: what you wrote here looks fine.

Thanks,
Taylor

Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> +	git clone src dst &&
>> +	test_commit -C src --printf "more_content" file "more content\ncontent\n" &&
>
> Same note here, but I think the `--printf` is unnecessary. Running
> `echo` with "\n" characters in its argument is fine, so this could be
> shortened to:
>
>     test_commit -C src blah file "more\ncontent"

Is that true for everybody's shell, or just dash?

$ bash -c 'echo "a\nb\nc"'
a\nb\nc
$ dash -c 'echo "a\nb\nc"'
a
b
c

Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config

[Taylor Blau]

On Thu, Jan 06, 2022 at 10:31:04AM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> +	git clone src dst &&
> >> +	test_commit -C src --printf "more_content" file "more content\ncontent\n" &&
> >
> > Same note here, but I think the `--printf` is unnecessary. Running
> > `echo` with "\n" characters in its argument is fine, so this could be
> > shortened to:
> >
> >     test_commit -C src blah file "more\ncontent"
>
> Is that true for everybody's shell, or just dash?
>
> $ bash -c 'echo "a\nb\nc"'
> a\nb\nc
> $ dash -c 'echo "a\nb\nc"'
> a
> b
> c

Ah, of course: thanks for the reminder. Do ignore my comment about
`--printf` being unnecessary, since it is quite necessary depending on
your shell.

In either case, I still think the quoting around "more_content" is
unnecessary (but the same is not true for the string containing
newlines).

Thanks,
Taylor