| Message ID | 20220106150226.77458-1-johncai86@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config | expand |
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
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
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
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
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
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(-)