builtin/pull.c: teach run_merge() to honor rebase.autostash config

PATCH In review John Cai 3 participants 5 messages Jan 6, 2022

Share & tools

John Cai

author

2022-01-06 15:03 UTC #0 lore raw

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

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" <[email protected]>

Co-authored-by "Junio C Hamano" <[email protected]>

Signed-off-by "John Cai" <[email protected]>

---

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

builtin/pull.c +8 -1

index 100cbf9fb8..8423e420ee 100644
@@ -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 {

t/t5521-pull-options.sh +12 -0

index 66cfcb09c5..66fac99d2b 100755
@@ -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

Signature · 1 line

2.34.1

Taylor Blau

2022-01-06 17:12 UTC #1 lore raw

Quoted 40 lines · 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" <[email protected]>
Co-authored-by "Junio C Hamano" <[email protected]>
Signed-off-by "John Cai" <[email protected]>
---
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(-)
builtin/pull.c +0 -0
index 100cbf9fb8..8423e420ee 100644
@@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("cannot rebase with locally recorded submodule modifications"));
 

builtin/pull.c int cmd_pull(int argc, const char **argv, const char *prefix) · line 1143

Quoted 10 lines

builtin/pull.c +8 -1

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

Quoted 8 lines

builtin/pull.c +0 -0

 			opt_ff = "--ff-only";
 			ret = run_merge();
 		} else {

t/t5521-pull-options.sh +0 -0

index 66cfcb09c5..66fac99d2b 100755

t/t5521-pull-options.sh test_expect_success 'git pull --no-verify --verify passed to merge' ' · line 258

Quoted 7 lines

t/t5521-pull-options.sh +4 -0

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

t/t5521-pull-options.sh test_expect_success 'git pull --no-verify --verify passed to merge' ' · line 260

Quoted 2 lines

t/t5521-pull-options.sh +2 -0

+	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

Taylor Blau

2022-01-06 17:14 UTC #2 lore raw

Quoted 8 lines · Taylor Blau

Quoted 7 lines · John Cai

builtin/pull.c +0 -0

index 100cbf9fb8..8423e420ee 100644
@@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("cannot rebase with locally recorded submodule modifications"));

Quoted 15 lines

builtin/pull.c int cmd_pull(int argc, const char **argv, const char *prefix) · line 1143
Quoted 10 lines
builtin/pull.c +8 -1
 		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

Junio C Hamano

2022-01-06 18:31 UTC #3 lore raw

In reply to Taylor Blau

Quoted 8 lines · Taylor Blau

t/t5521-pull-options.sh test_expect_success 'git pull --no-verify --verify passed to merge' ' · line 260
Quoted 2 lines
t/t5521-pull-options.sh +2 -0
+	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

Taylor Blau

2022-01-06 19:36 UTC #4 lore raw

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Taylor Blau
t/t5521-pull-options.sh test_expect_success 'git pull --no-verify --verify passed to merge' ' · line 260
Quoted 2 lines
t/t5521-pull-options.sh +2 -0
+	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"

Quoted 8 lines

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