| Message ID | 20220104214522.10692-2-johncai86@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix bug in pull --rebase not recognizing rebase.autostash | expand |
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 {
Hi John, Le 2022-01-04 à 16:45, John Cai a écrit : > A bug in pull.c causes merge and rebase functions to ignore > rebase.autostash if it is only set in the config. The reported bug only affects fast-forwards as far as I understand, so I don't think "merge and rebase" is the best wording here. Also, 'functions' is not super clear. The actual functions in the code are 'run_rebase' and 'run_merge', if that is what you are referring to. If you mean the different underlying "modes" of 'git pull', I'd phrase it more like "the underlying 'merge' or 'rebase' invocations" or something like that - but again, only the underlying 'git merge' is affected, and only for fast-forwards. > > There are a couple of different scenarios that we need to be mindful of: > 1. --autostash passed in through command line > $ git pull --autostash > merge/rebase should get --autostashed passed through > > 2. --rebase passed in, rebase.autostash set in config > $ git config rebase.autostash true > $ git pull --rebase > > merge/rebase should get --autostash from config > > 3. --no-autostash passed in > $ git pull --no-autostash > --no-autostash should be passed into merge/rebase > > 4. rebase.autostash set but --rebase not used > > $ git config rebase.autostash true > $ git pull > --autostash should not be passed into merge but not rebase Usually we start the commit message by a description of the current behaviour, so in the case of a bugfix like here, a description of the exact behaviour that triggers the bug. As Tilman reported, this only affects fast-forwards, so that should be mentioned in your commit message. While what you wrote is not wrong per se (although I'm not sure what you meant with point 4), almost all of the behaviour is correct, apart from the (rebase + ff) case, so I would focus on that. > > This change adjusts variable names to make it more clear which autostash > setting it is modifying, and ensures --autostash is passed into the > merge/rebase where appropriate. As Junio already pointed out, I'm not sure the changes you propose are really clearer... I agree that adding yet another variable is unneeded. > > Reported-by: "Tilman Vogel" <tilman.vogel@web.de> > Co-authored-by: "Philippe Blain" <levraiphilippeblain@gmail.com> As I remarked in the other thread, I'd prefer if you remove that trailer. > Signed-Off-by: "John Cai" <johncai86@gmail.com> > --- > builtin/pull.c | 15 ++++++------ > t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++ The existing tests for 'git pull --autostash' are in t5520, so I think it might make more sense to add any new tests there instead of t5521. > 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' ' > + 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 > +' This seems to test the same thing as t5520's "--rebase --autostash fast forward", so I don't think it's necessary to add this one. > + > +test_expect_success 'git pull --rebase with 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 && > + test_config -C dst rebase.autostash true && > + git -C dst pull --rebase >actual 2>&1 && > + grep -q "Fast-forward" actual && > + grep -q "Applied autostash." actual > +' OK, this is the actual test that was failing. > + > +test_expect_success 'git pull --rebase --autostash succeeds on non-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 "src_content" file "src content\ncontent\n" && > + test_commit -C dst --append "dst_content" file "dst content" && > + echo "dirty" >>dst/file && > + git -C dst pull --rebase --autostash >actual 2>&1 && > + grep -q "Successfully rebased and updated refs/heads/main." actual && > + grep -q "Applied autostash." actual > +' This seems to test the same thing as t5520's "pull --rebase --autostash & rebase.autostash unset" > + > +test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 "src_content" file "src content\ncontent\n" && > + test_commit -C dst --append "dst_content" file "dst content" && > + echo "dirty" >>dst/file && > + test_config -C dst rebase.autostash true && > + git -C dst pull --rebase >actual 2>&1 && > + grep -q "Successfully rebased and updated refs/heads/main." actual && > + grep -q "Applied autostash." actual > +' This seems to test the same thing as t5520's "pull --rebase succeeds with dirty working directory and rebase.autostash set". Thanks for working on this ! :) Philippe.
Hi Junio, Le 2022-01-04 à 17:46, Junio C Hamano a écrit : > > 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 { > We already have a quite useless 'int autostash' local variable in cmd_pull a few lines up, so an equivalent fix, I think, would be the following: diff --git a/builtin/pull.c b/builtin/pull.c index 1cfaf9f343..9f8a8dd716 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix) oidclr(&orig_head); if (opt_rebase) { - int autostash = config_autostash; - if (opt_autostash != -1) - autostash = opt_autostash; + if (opt_autostash == -1) + opt_autostash = config_autostash; if (is_null_oid(&orig_head) && !is_cache_unborn()) die(_("Updating an unborn branch with changes added to the index.")); - if (!autostash) + if (!opt_autostash) require_clean_work_tree(the_repository, N_("pull with rebase"), _("please commit or stash them."), 1, 0); Thanks, Philippe.
Hi again John, Le 2022-01-04 à 22:40, Philippe Blain a écrit : > Hi John, > > Le 2022-01-04 à 16:45, John Cai a écrit : > > Usually we start the commit message by a description of the current behaviour, > so in the case of a bugfix like here, a description of the exact behaviour > that triggers the bug. As Tilman reported, this only affects fast-forwards, > so that should be mentioned in your commit message. > While what you wrote is not wrong per se (although I'm not sure what you meant > with point 4), almost all of the behaviour is > correct, apart from the (rebase + ff) case, so I would focus on that. I forgot to mention in my previous mail, but in the same vein, I think your commit message title could reflect a little more the bug you are fixing, maybe something like pull: honor 'rebase.autostash' if rebase fast-forwards or something similar.
Hi John and Junio On 04/01/2022 22:46, Junio C Hamano wrote: > 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. I think this is a better approach - it's simpler and it is clear that we only use rebase.autostash when a rebase was requested. Best Wishes Phillip > 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 { >
Hi John, On Tue, 4 Jan 2022, John Cai wrote: > Reported-by: "Tilman Vogel" <tilman.vogel@web.de> > Co-authored-by: "Philippe Blain" <levraiphilippeblain@gmail.com> > Signed-Off-by: "John Cai" <johncai86@gmail.com> We spell the 'o' in 'Signed-off-by' with a lower-case 'o'. That's what `git commit -s` does automatically. Was this the problem why you stopped using GitGitGadget? It would also have helped you avoid the frowned-upon cover letter for single patch contributions. The entire point of GitGitGadget is to _not_ force contributors to know about all these things. Ciao, Dscho
Philippe Blain <levraiphilippeblain@gmail.com> writes: > We already have a quite useless 'int autostash' local variable in cmd_pull > a few lines up, so an equivalent fix, I think, would be the following: > > diff --git a/builtin/pull.c b/builtin/pull.c > index 1cfaf9f343..9f8a8dd716 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > oidclr(&orig_head); > if (opt_rebase) { > - int autostash = config_autostash; > - if (opt_autostash != -1) > - autostash = opt_autostash; > + if (opt_autostash == -1) > + opt_autostash = config_autostash; > if (is_null_oid(&orig_head) && !is_cache_unborn()) > die(_("Updating an unborn branch with changes added to the index.")); > - if (!autostash) > + if (!opt_autostash) > require_clean_work_tree(the_repository, > N_("pull with rebase"), > _("please commit or stash them."), 1, 0); OK, so the idea is to make opt_autostash the _primary_ thing that matters when deciding to do the auto-stashing. We may make it affected by the value we read from the configuration, if there is no command line option that sets it. If there is no place that cares about what is in config_autostash (which is rebase.autostash; nobody reads merge.autostash in this command) other than this part, I think that is an even cleaner approach than what I sent. I very much like it. Here is how I would explain your change. ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- Subject: pull --rebase: honor rebase.autostash even when fast-forwarding "pull --rebase" internally uses the merge machinery when the other history is a descendant of ours (i.e. perform fast-forward). This came from [1], where the discussion was started from a feature request to do so. It is a bit hard to read the rationale behind it in the discussion, but it seems that it was an established fact for everybody involved that does not even need to be mentioned that fast-forwarding done with "rebase" was much undesirable than done with "merge", and more importantly, the result left by "merge" is as good as (or better than) that by "rebase". Except for one thing. Because "git merge" does not (and should not) honor rebase.autostash, "git pull" needs to read it and forward it when we use "git merge" as a (hopefully better) substitute for "git rebase" during the fast-forwarding. But we forgot to do so (we only add "--[no-]autostash" to the "git merge" command when "git pull" was invoked with "--[no-]autostash" command line option, so that "git merge" can honor merge.autostash in such a case). Make sure "git merge" is run with "--autostash" when rebase.autostash is set and used to fast-forward the history on behalf of "git rebase". Incidentally this change also takes care of the case where - "git pull --rebase" (without other command line options) is run - "rebase.autostash" is not set - The history fast-forwards In such a case, "git merge" is run with an explicit "--no-autostash" to prevent it from honoring merge.autostash configuration, which is what we want. After all, we want the "git merge" to pretend as if it is "git rebase" while being used for this purpose. [1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/
Junio C Hamano <gitster@pobox.com> writes: > Here is how I would explain your change. This topic is in "Expecting an ack or two" state for some time. - Philippe, are you OK with the attached patch? If so throw your Signed-off-by to this thread. - John, if you find Philippe's implementation a good idea (I think it is, as it is simpler and cleaner) after reading and understanding it, please throw an Acked-by or Reviewed-by to this thread. ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- From: Philippe Blain <levraiphilippeblain@gmail.com> Date: Tue, 4 Jan 2022 22:58:32 -0500 Subject: [PATCH] pull --rebase: honor rebase.autostash even when fast-forwarding "pull --rebase" internally uses the merge machinery when the other history is a descendant of ours (i.e. perform fast-forward). This came from [1], where the discussion was started from a feature request to do so. It is a bit hard to read the rationale behind it in the discussion, but it seems that it was an established fact for everybody involved that does not even need to be mentioned that fast-forwarding done with "rebase" was much undesirable than done with "merge", and more importantly, the result left by "merge" is as good as (or better than) that by "rebase". Except for one thing. Because "git merge" does not (and should not) honor rebase.autostash, "git pull" needs to read it and forward it when we use "git merge" as a (hopefully better) substitute for "git rebase" during the fast-forwarding. But we forgot to do so (we only add "--[no-]autostash" to the "git merge" command when "git pull" was invoked with "--[no-]autostash" command line option, so that "git merge" can honor merge.autostash in such a case). Make sure "git merge" is run with "--autostash" when rebase.autostash is set and used to fast-forward the history on behalf of "git rebase". Incidentally this change also takes care of the case where - "git pull --rebase" (without other command line options) is run - "rebase.autostash" is not set - The history fast-forwards In such a case, "git merge" is run with an explicit "--no-autostash" to prevent it from honoring merge.autostash configuration, which is what we want. After all, we want the "git merge" to pretend as if it is "git rebase" while being used for this purpose. [1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/ --- builtin/pull.c | 7 +++--- t/t5521-pull-options.sh | 52 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 1cfaf9f343..9f8a8dd716 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix) oidclr(&orig_head); if (opt_rebase) { - int autostash = config_autostash; - if (opt_autostash != -1) - autostash = opt_autostash; + if (opt_autostash == -1) + opt_autostash = config_autostash; if (is_null_oid(&orig_head) && !is_cache_unborn()) die(_("Updating an unborn branch with changes added to the index.")); - if (!autostash) + if (!opt_autostash) require_clean_work_tree(the_repository, N_("pull with rebase"), _("please commit or stash them."), 1, 0); diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index 66cfcb09c5..4046a74cad 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -252,4 +252,56 @@ 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_expect_success 'git pull --rebase with 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 && + test_config -C dst rebase.autostash true && + git -C dst pull --rebase >actual 2>&1 && + grep -q "Fast-forward" actual && + grep -q "Applied autostash." actual +' + +test_expect_success 'git pull --rebase --autostash succeeds on non-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 "src_content" file "src content\ncontent\n" && + test_commit -C dst --append "dst_content" file "dst content" && + echo "dirty" >>dst/file && + git -C dst pull --rebase --autostash >actual 2>&1 && + grep -q "Successfully rebased and updated refs/heads/main." actual && + grep -q "Applied autostash." actual +' + +test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 "src_content" file "src content\ncontent\n" && + test_commit -C dst --append "dst_content" file "dst content" && + echo "dirty" >>dst/file && + test_config -C dst rebase.autostash true && + git -C dst pull --rebase >actual 2>&1 && + grep -q "Successfully rebased and updated refs/heads/main." actual && + grep -q "Applied autostash." actual +' + test_done
Hi Junio, Le 2022-01-13 à 19:00, Junio C Hamano a écrit : > Junio C Hamano <gitster@pobox.com> writes: > >> Here is how I would explain your change. > > This topic is in "Expecting an ack or two" state for some time. > > - Philippe, are you OK with the attached patch? If so throw your > Signed-off-by to this thread. I'm not 100% OK since as I remarked to John in [1], I don't think all 4 tests are necessary, 3 out of 4 are duplicates of existing tests, and I would have put the new test in t5520 with 'git pull's other "autostash" tests. I hoped that John would incorporate my suggestions in a v2, but he seems to be busy, so I'm including an updated patch at the end of this email. I only slightly edited the commit message you wrote, so thanks for that. 'pb/pull-rebase-autostash-fix' could be replaced by the patch below, I would think. > - John, if you find Philippe's implementation a good idea (I think > it is, as it is simpler and cleaner) after reading and > understanding it, please throw an Acked-by or Reviewed-by to this > thread. > Thanks, Philippe. [1] https://lore.kernel.org/git/fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com/ ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- From 28edde4e302e14c900e314268e4eeaeadc240bcb Mon Sep 17 00:00:00 2001 From: Philippe Blain <levraiphilippeblain@gmail.com> Date: Thu, 13 Jan 2022 21:58:05 -0500 Subject: [PATCH] pull --rebase: honor rebase.autostash when fast-forwarding "pull --rebase" internally uses the merge machinery when the other history is a descendant of ours (i.e. perform fast-forward). This came from [1], where the discussion was started from a feature request to do so. It is a bit hard to read the rationale behind it in the discussion, but it seems that it was an established fact for everybody involved that does not even need to be mentioned that fast-forwarding done with "rebase" was much undesirable than done with "merge", and more importantly, the result left by "merge" is as good as (or better than) that by "rebase". Except for one thing. Because "git merge" does not (and should not) honor rebase.autostash, "git pull" needs to read it and forward it when we use "git merge" as a (hopefully better) substitute for "git rebase" during the fast-forwarding. But we forgot to do so (we only add "--[no-]autostash" to the "git merge" command when "git pull" itself was invoked with "--[no-]autostash" command line option. Make sure "git merge" is run with "--autostash" when rebase.autostash is set and used to fast-forward the history on behalf of "git rebase". Incidentally this change also takes care of the case where - "git pull --rebase" (without other command line options) is run - "rebase.autostash" is not set - The history fast-forwards In such a case, "git merge" is run with an explicit "--no-autostash" to prevent it from honoring merge.autostash configuration, which is what we want. After all, we want the "git merge" to pretend as if it is "git rebase" while being used for this purpose. [1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/ Reported-by: Tilman Vogel <tilman.vogel@web.de> Co-authored-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> --- builtin/pull.c | 7 +++---- t/t5520-pull.sh | 13 +++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 1cfaf9f343..9f8a8dd716 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix) oidclr(&orig_head); if (opt_rebase) { - int autostash = config_autostash; - if (opt_autostash != -1) - autostash = opt_autostash; + if (opt_autostash == -1) + opt_autostash = config_autostash; if (is_null_oid(&orig_head) && !is_cache_unborn()) die(_("Updating an unborn branch with changes added to the index.")); - if (!autostash) + if (!opt_autostash) require_clean_work_tree(the_repository, N_("pull with rebase"), _("please commit or stash them."), 1, 0); diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 93ecfcdd24..3e784f18a6 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -330,6 +330,19 @@ test_expect_success '--rebase --autostash fast forward' ' test_cmp_rev HEAD to-rebase-ff ' +test_expect_success '--rebase with 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 && + test_config -C dst rebase.autostash true && + git -C dst pull --rebase >actual 2>&1 && + grep -q "Fast-forward" actual && + grep -q "Applied autostash." actual +' + test_expect_success '--rebase with conflicts shows advice' ' test_when_finished "git rebase --abort; git checkout -f to-rebase" && git checkout -b seq && base-commit: e9d7761bb94f20acc98824275e317fa82436c25d prerequisite-patch-id: 3c6b4be75d7a634bf45f0264b3f04216818a0816
> On Jan 13, 2022, at 10:14 PM, Philippe Blain <levraiphilippeblain@gmail.com> wrote: > > Hi Junio, > >> Le 2022-01-13 à 19:00, Junio C Hamano a écrit : >> Junio C Hamano <gitster@pobox.com> writes: >>> Here is how I would explain your change. >> This topic is in "Expecting an ack or two" state for some time. >> - Philippe, are you OK with the attached patch? If so throw your >> Signed-off-by to this thread. > > I'm not 100% OK since as I remarked to John in [1], I don't think all 4 > tests are necessary, 3 out of 4 are duplicates of existing tests, and I > would have put the new test in t5520 with 'git pull's other "autostash" > tests. I hoped that John would incorporate my suggestions in a v2, but he > seems to be busy, so I'm including an updated patch at the end of this email. > I only slightly edited the commit message you wrote, so thanks for that. > 'pb/pull-rebase-autostash-fix' could be replaced by the patch below, > I would think. > >> - John, if you find Philippe's implementation a good idea (I think >> it is, as it is simpler and cleaner) after reading and >> understanding it, please throw an Acked-by or Reviewed-by to this >> thread. I agree-think Philippe’s implementation is a better solution. I don’t have the bandwidth these couple of days to reroll the patch so if someone else can take over that would be great! > > > Thanks, > > Philippe. > > [1] https://lore.kernel.org/git/fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com/ > > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- > From 28edde4e302e14c900e314268e4eeaeadc240bcb Mon Sep 17 00:00:00 2001 > From: Philippe Blain <levraiphilippeblain@gmail.com> > Date: Thu, 13 Jan 2022 21:58:05 -0500 > Subject: [PATCH] pull --rebase: honor rebase.autostash when fast-forwarding > > "pull --rebase" internally uses the merge machinery when the other > history is a descendant of ours (i.e. perform fast-forward). This > came from [1], where the discussion was started from a feature > request to do so. It is a bit hard to read the rationale behind it > in the discussion, but it seems that it was an established fact for > everybody involved that does not even need to be mentioned that > fast-forwarding done with "rebase" was much undesirable than done > with "merge", and more importantly, the result left by "merge" is as > good as (or better than) that by "rebase". > > Except for one thing. Because "git merge" does not (and should not) > honor rebase.autostash, "git pull" needs to read it and forward it > when we use "git merge" as a (hopefully better) substitute for "git > rebase" during the fast-forwarding. But we forgot to do so (we only > add "--[no-]autostash" to the "git merge" command when "git pull" itself > was invoked with "--[no-]autostash" command line option. > > Make sure "git merge" is run with "--autostash" when > rebase.autostash is set and used to fast-forward the history on > behalf of "git rebase". Incidentally this change also takes care of > the case where > > - "git pull --rebase" (without other command line options) is run > - "rebase.autostash" is not set > - The history fast-forwards > > In such a case, "git merge" is run with an explicit "--no-autostash" > to prevent it from honoring merge.autostash configuration, which is > what we want. After all, we want the "git merge" to pretend as if > it is "git rebase" while being used for this purpose. > > [1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/ > > Reported-by: Tilman Vogel <tilman.vogel@web.de> > Co-authored-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > --- > builtin/pull.c | 7 +++---- > t/t5520-pull.sh | 13 +++++++++++++ > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 1cfaf9f343..9f8a8dd716 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > oidclr(&orig_head); > if (opt_rebase) { > - int autostash = config_autostash; > - if (opt_autostash != -1) > - autostash = opt_autostash; > + if (opt_autostash == -1) > + opt_autostash = config_autostash; > if (is_null_oid(&orig_head) && !is_cache_unborn()) > die(_("Updating an unborn branch with changes added to the index.")); > - if (!autostash) > + if (!opt_autostash) > require_clean_work_tree(the_repository, > N_("pull with rebase"), > _("please commit or stash them."), 1, 0); > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh > index 93ecfcdd24..3e784f18a6 100755 > --- a/t/t5520-pull.sh > +++ b/t/t5520-pull.sh > @@ -330,6 +330,19 @@ test_expect_success '--rebase --autostash fast forward' ' > test_cmp_rev HEAD to-rebase-ff > ' > +test_expect_success '--rebase with 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 && > + test_config -C dst rebase.autostash true && > + git -C dst pull --rebase >actual 2>&1 && > + grep -q "Fast-forward" actual && > + grep -q "Applied autostash." actual > +' > + > test_expect_success '--rebase with conflicts shows advice' ' > test_when_finished "git rebase --abort; git checkout -f to-rebase" && > git checkout -b seq && > > base-commit: e9d7761bb94f20acc98824275e317fa82436c25d > prerequisite-patch-id: 3c6b4be75d7a634bf45f0264b3f04216818a0816 > -- > 2.29.2
Philippe Blain <levraiphilippeblain@gmail.com> writes: > tests. I hoped that John would incorporate my suggestions in a v2, but he > seems to be busy, so I'm including an updated patch at the end of this email. Was about to say "Will replace the in-tree version with this. Thanks", before I realized that your message is "text/plain; format=flawed" X-<. I think I fixed it up correctly. I'll pick this version up from the list and replace what's in-tree with it. Thanks. ----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 ----- From: Philippe Blain <levraiphilippeblain@gmail.com> Date: Thu, 13 Jan 2022 22:14:51 -0500 Subject: [PATCH] pull --rebase: honor rebase.autostash when fast-forwarding "pull --rebase" internally uses the merge machinery when the other history is a descendant of ours (i.e. perform fast-forward). This came from [1], where the discussion was started from a feature request to do so. It is a bit hard to read the rationale behind it in the discussion, but it seems that it was an established fact for everybody involved that does not even need to be mentioned that fast-forwarding done with "rebase" was much undesirable than done with "merge", and more importantly, the result left by "merge" is as good as (or better than) that by "rebase". Except for one thing. Because "git merge" does not (and should not) honor rebase.autostash, "git pull" needs to read it and forward it when we use "git merge" as a (hopefully better) substitute for "git rebase" during the fast-forwarding. But we forgot to do so (we only add "--[no-]autostash" to the "git merge" command when "git pull" itself was invoked with "--[no-]autostash" command line option. Make sure "git merge" is run with "--autostash" when rebase.autostash is set and used to fast-forward the history on behalf of "git rebase". Incidentally this change also takes care of the case where - "git pull --rebase" (without other command line options) is run - "rebase.autostash" is not set - The history fast-forwards In such a case, "git merge" is run with an explicit "--no-autostash" to prevent it from honoring merge.autostash configuration, which is what we want. After all, we want the "git merge" to pretend as if it is "git rebase" while being used for this purpose. [1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/ Reported-by: Tilman Vogel <tilman.vogel@web.de> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> --- builtin/pull.c | 7 +++---- t/t5520-pull.sh | 13 +++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 1cfaf9f343..9f8a8dd716 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix) oidclr(&orig_head); if (opt_rebase) { - int autostash = config_autostash; - if (opt_autostash != -1) - autostash = opt_autostash; + if (opt_autostash == -1) + opt_autostash = config_autostash; if (is_null_oid(&orig_head) && !is_cache_unborn()) die(_("Updating an unborn branch with changes added to the index.")); - if (!autostash) + if (!opt_autostash) require_clean_work_tree(the_repository, N_("pull with rebase"), _("please commit or stash them."), 1, 0); diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 93ecfcdd24..081808009b 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -330,6 +330,19 @@ test_expect_success '--rebase --autostash fast forward' ' test_cmp_rev HEAD to-rebase-ff ' +test_expect_success '--rebase with 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 && + test_config -C dst rebase.autostash true && + git -C dst pull --rebase >actual 2>&1 && + grep -q "Fast-forward" actual && + grep -q "Applied autostash." actual +' + test_expect_success '--rebase with conflicts shows advice' ' test_when_finished "git rebase --abort; git checkout -f to-rebase" && git checkout -b seq &&
Hi Junio, Le 2022-01-14 à 14:40, Junio C Hamano a écrit : > Philippe Blain <levraiphilippeblain@gmail.com> writes: > >> tests. I hoped that John would incorporate my suggestions in a v2, but he >> seems to be busy, so I'm including an updated patch at the end of this email. > > Was about to say "Will replace the in-tree version with this. Thanks", > before I realized that your message is "text/plain; format=flawed" X-<. I'm sorry for that. I was in a rush and *tought* that I had Thunderbird configured correctly so that pasting the patch would work out. It seems not. I'll be more careful next time:) > > I think I fixed it up correctly. I'll pick this version up from the > list and replace what's in-tree with it. > > Thanks. Thanks! Philippe.
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; static int check_trust_level = 1; static struct strvec opt_strategies = STRVEC_INIT; static struct strvec opt_strategy_opts = STRVEC_INIT; @@ -361,7 +362,7 @@ static int git_pull_config(const char *var, const char *value, void *cb) int status; if (!strcmp(var, "rebase.autostash")) { - config_autostash = git_config_bool(var, value); + cfg_rebase_autostash = git_config_bool(var, value); return 0; } else if (!strcmp(var, "submodule.recurse")) { recurse_submodules = git_config_bool(var, value) ? @@ -689,7 +690,7 @@ static int run_merge(void) strvec_push(&args, opt_gpg_sign); if (opt_autostash == 0) strvec_push(&args, "--no-autostash"); - else if (opt_autostash == 1) + else if (rebase_autostash == 1 || opt_autostash == 1) strvec_push(&args, "--autostash"); if (opt_allow_unrelated_histories > 0) strvec_push(&args, "--allow-unrelated-histories"); @@ -901,7 +902,7 @@ static int run_rebase(const struct object_id *newbase, strvec_push(&args, opt_signoff); if (opt_autostash == 0) strvec_push(&args, "--no-autostash"); - else if (opt_autostash == 1) + else if (rebase_autostash == 1) strvec_push(&args, "--autostash"); if (opt_verify_signatures && !strcmp(opt_verify_signatures, "--verify-signatures")) @@ -1038,14 +1039,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix) oidclr(&orig_head); if (opt_rebase) { - int autostash = config_autostash; + rebase_autostash = cfg_rebase_autostash; if (opt_autostash != -1) - autostash = opt_autostash; + rebase_autostash = opt_autostash; if (is_null_oid(&orig_head) && !is_cache_unborn()) die(_("Updating an unborn branch with changes added to the index.")); - if (!autostash) + if (!rebase_autostash) require_clean_work_tree(the_repository, N_("pull with rebase"), _("please commit or stash them."), 1, 0); 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' ' + 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_expect_success 'git pull --rebase with 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 && + test_config -C dst rebase.autostash true && + git -C dst pull --rebase >actual 2>&1 && + grep -q "Fast-forward" actual && + grep -q "Applied autostash." actual +' + +test_expect_success 'git pull --rebase --autostash succeeds on non-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 "src_content" file "src content\ncontent\n" && + test_commit -C dst --append "dst_content" file "dst content" && + echo "dirty" >>dst/file && + git -C dst pull --rebase --autostash >actual 2>&1 && + grep -q "Successfully rebased and updated refs/heads/main." actual && + grep -q "Applied autostash." actual +' + +test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 "src_content" file "src content\ncontent\n" && + test_commit -C dst --append "dst_content" file "dst content" && + echo "dirty" >>dst/file && + test_config -C dst rebase.autostash true && + git -C dst pull --rebase >actual 2>&1 && + grep -q "Successfully rebased and updated refs/heads/main." actual && + grep -q "Applied autostash." actual +' test_done
A bug in pull.c causes merge and rebase functions to ignore rebase.autostash if it is only set in the config. There are a couple of different scenarios that we need to be mindful of: 1. --autostash passed in through command line $ git pull --autostash merge/rebase should get --autostashed passed through 2. --rebase passed in, rebase.autostash set in config $ git config rebase.autostash true $ git pull --rebase merge/rebase should get --autostash from config 3. --no-autostash passed in $ git pull --no-autostash --no-autostash should be passed into merge/rebase 4. rebase.autostash set but --rebase not used $ git config rebase.autostash true $ git pull --autostash should not be passed into merge but not rebase This change adjusts variable names to make it more clear which autostash setting it is modifying, and ensures --autostash is passed into the merge/rebase where appropriate. Reported-by: "Tilman Vogel" <tilman.vogel@web.de> Co-authored-by: "Philippe Blain" <levraiphilippeblain@gmail.com> Signed-Off-by: "John Cai" <johncai86@gmail.com> --- builtin/pull.c | 15 ++++++------ t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 7 deletions(-)