Message ID | 9749fa2c-b08d-c08b-ce43-041d13852d02@web.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [BUG] git pull --rebase ignores rebase.autostash config when fast-forwarding | expand |
On 03/01/2022 18:08, Tilman Vogel wrote: > Hi git-people, > > I ran into strange behavior when having rebase.autostash enabled and > doing a git pull --rebase: > >> git config rebase.autostash true >> git pull --rebase > Updating cd9ff8a..f3c9840 > error: Your local changes to the following files would be overwritten by > merge: > content > Please commit your changes or stash them before you merge. > Aborting > > Confusingly, this fixes the issue: > >> git config merge.autostash true >> git pull --rebase > Updating cd9ff8a..f3c9840 > Created autostash: c615fda > Fast-forward > content | 1 + > 1 file changed, 1 insertion(+) > Applied autostash. > > Leaving me wonder why merge config options fix rebase behavior. > > So, in order to make it easier to check the problem, I added some > test-cases to the git test-suite. Please see the attached patch. > > Or here: > https://github.com/tvogel/git/commit/bc941f9357518a34cfa11788dfb8e7fa7f711705 > > I did not try to find the root-cause as I am not experienced with the > code-base but if there are questions, let me know. Which version are you running? There was a problem in 2.33.1 as per thread discussion: https://lore.kernel.org/git/YYFEE%2F2g3SiM04zx@hades.panopticon/ (I'd searched the logs for ".autostash true") Does that provide any guidance? -- Philip
Hi Tilman, Le 2022-01-04 à 07:59, Philip Oakley a écrit : > On 03/01/2022 18:08, Tilman Vogel wrote: >> Hi git-people, >> >> I ran into strange behavior when having rebase.autostash enabled and >> doing a git pull --rebase: >> >>> git config rebase.autostash true >>> git pull --rebase >> Updating cd9ff8a..f3c9840 >> error: Your local changes to the following files would be overwritten by >> merge: >> content >> Please commit your changes or stash them before you merge. >> Aborting >> >> Confusingly, this fixes the issue: >> >>> git config merge.autostash true >>> git pull --rebase >> Updating cd9ff8a..f3c9840 >> Created autostash: c615fda >> Fast-forward >> content | 1 + >> 1 file changed, 1 insertion(+) >> Applied autostash. >> >> Leaving me wonder why merge config options fix rebase behavior. >> >> So, in order to make it easier to check the problem, I added some >> test-cases to the git test-suite. Please see the attached patch. Thanks, this really makes it easier to bisect the issue. >> >> Or here: >> https://github.com/tvogel/git/commit/bc941f9357518a34cfa11788dfb8e7fa7f711705 >> >> I did not try to find the root-cause as I am not experienced with the >> code-base but if there are questions, let me know. > > Which version are you running? > That's a good info to include indeed. I'm guessing you are using v2.34.1 as that's the version indicated at the bottom of your attached patch. I can replicate the behaviour on my side on 2.34.1. I did not bisect manually but I'm pretty sure it's a regression caused by 340062243a (pull: cleanup autostash check, 2021-06-17) (author CC'ed). I checked that the parent of that commit passes all 4 of your added tests, provided this is squashed in: diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index 4046a74cad..5ad19b1028 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -260,7 +260,6 @@ test_expect_success 'git pull --rebase --autostash succeeds on ff' ' 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 ' @@ -273,7 +272,6 @@ test_expect_success 'git pull --rebase with rebase.autostash succeeds on ff' ' 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 ' After that commit, in case of fast-forward, 'git pull --rebase --autostash' delegates the fast-forward operation to 'git merge' under the hood, which was not the case before. The '--autostash' flag seems to be forwarded correctly to that 'git merge' invocation, but the config 'rebase.autostash' seems to not be passed along. I did not yet look into why in the code itself. That does explain however why 'merge.autostash' makes it work - the 'git merge' invocation does check its own config, and if merge.autostash is there the autostash behaviour is activated. Philippe.
Hi again, Le 2022-01-04 à 13:03, Philippe Blain a écrit : > Hi Tilman, > > Le 2022-01-04 à 07:59, Philip Oakley a écrit : >> On 03/01/2022 18:08, Tilman Vogel wrote: >>> Hi git-people, >>> >>> I ran into strange behavior when having rebase.autostash enabled and >>> doing a git pull --rebase: >>> >>>> git config rebase.autostash true >>>> git pull --rebase >>> Updating cd9ff8a..f3c9840 >>> error: Your local changes to the following files would be overwritten by >>> merge: >>> content >>> Please commit your changes or stash them before you merge. >>> Aborting >>> >>> Confusingly, this fixes the issue: >>> >>>> git config merge.autostash true >>>> git pull --rebase >>> Updating cd9ff8a..f3c9840 >>> Created autostash: c615fda >>> Fast-forward >>> content | 1 + >>> 1 file changed, 1 insertion(+) >>> Applied autostash. >>> >>> Leaving me wonder why merge config options fix rebase behavior. >>> >>> So, in order to make it easier to check the problem, I added some >>> test-cases to the git test-suite. Please see the attached patch. > > Thanks, this really makes it easier to bisect the issue. > >>> >>> Or here: >>> https://github.com/tvogel/git/commit/bc941f9357518a34cfa11788dfb8e7fa7f711705 >>> >>> I did not try to find the root-cause as I am not experienced with the >>> code-base but if there are questions, let me know. >> >> Which version are you running? >> > > That's a good info to include indeed. I'm guessing you are using v2.34.1 as that's the version > indicated at the bottom of your attached patch. I can replicate the behaviour on my side on 2.34.1. > I did not bisect manually but I'm pretty sure it's a regression caused by 340062243a (pull: cleanup autostash > check, 2021-06-17) (author CC'ed). I checked that the parent of that commit passes all 4 of your added tests, provided > this is squashed in: > > diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh > index 4046a74cad..5ad19b1028 100755 > --- a/t/t5521-pull-options.sh > +++ b/t/t5521-pull-options.sh > @@ -260,7 +260,6 @@ test_expect_success 'git pull --rebase --autostash succeeds on ff' ' > 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 > ' > > @@ -273,7 +272,6 @@ test_expect_success 'git pull --rebase with rebase.autostash succeeds on ff' ' > 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 > ' > > After that commit, in case of fast-forward, 'git pull --rebase --autostash' delegates the fast-forward > operation to 'git merge' under the hood, which was not the case before. The '--autostash' flag seems > to be forwarded correctly to that 'git merge' invocation, but the config 'rebase.autostash' seems to not > be passed along. > > I did not yet look into why in the code itself After looking at it a bit, this seems to fix the bug: diff --git a/builtin/pull.c b/builtin/pull.c index 1cfaf9f343..0b206bf1d5 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -87,6 +87,7 @@ static char *opt_verify_signatures; static char *opt_verify; static int opt_autostash = -1; static int config_autostash; +static int autostash = -1; static int check_trust_level = 1; static struct strvec opt_strategies = STRVEC_INIT; static struct strvec opt_strategy_opts = STRVEC_INIT; @@ -687,9 +688,9 @@ static int run_merge(void) strvec_pushv(&args, opt_strategy_opts.v); if (opt_gpg_sign) strvec_push(&args, opt_gpg_sign); - if (opt_autostash == 0) + if (autostash == 0) strvec_push(&args, "--no-autostash"); - else if (opt_autostash == 1) + else if (autostash == 1) strvec_push(&args, "--autostash"); if (opt_allow_unrelated_histories > 0) strvec_push(&args, "--allow-unrelated-histories"); @@ -1036,7 +1037,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) oidclr(&orig_head); if (opt_rebase) { - int autostash = config_autostash; + autostash = config_autostash; if (opt_autostash != -1) autostash = opt_autostash; If nobody beats me to it (if that's the case, be my guest !), I'll try to submit a proper patch later today or this week. In case anybody needs it: Signed-off by: Philippe Blain <levraiphilippeblain@gmail.com> Cheers, Philippe.
Oh, please excuse my lack of providing the version info: I discovered the behaviour on 2.34.1 as shipped by current openSUSE Tumbleweed. Then, I worked on-top of current git master (2ae0a9cb). Regards and thanks for investigating and fixing this so quickly! Tilman Am Di., 4. Jan. 2022 um 19:03 Uhr schrieb Philippe Blain <levraiphilippeblain@gmail.com>: > > Hi Tilman, > > Le 2022-01-04 à 07:59, Philip Oakley a écrit : > > On 03/01/2022 18:08, Tilman Vogel wrote: > >> Hi git-people, > >> > >> I ran into strange behavior when having rebase.autostash enabled and > >> doing a git pull --rebase: > >> > >>> git config rebase.autostash true > >>> git pull --rebase > >> Updating cd9ff8a..f3c9840 > >> error: Your local changes to the following files would be overwritten by > >> merge: > >> content > >> Please commit your changes or stash them before you merge. > >> Aborting > >> > >> Confusingly, this fixes the issue: > >> > >>> git config merge.autostash true > >>> git pull --rebase > >> Updating cd9ff8a..f3c9840 > >> Created autostash: c615fda > >> Fast-forward > >> content | 1 + > >> 1 file changed, 1 insertion(+) > >> Applied autostash. > >> > >> Leaving me wonder why merge config options fix rebase behavior. > >> > >> So, in order to make it easier to check the problem, I added some > >> test-cases to the git test-suite. Please see the attached patch. > > Thanks, this really makes it easier to bisect the issue. > > >> > >> Or here: > >> https://github.com/tvogel/git/commit/bc941f9357518a34cfa11788dfb8e7fa7f711705 > >> > >> I did not try to find the root-cause as I am not experienced with the > >> code-base but if there are questions, let me know. > > > > Which version are you running? > > > > That's a good info to include indeed. I'm guessing you are using v2.34.1 as that's the version > indicated at the bottom of your attached patch. I can replicate the behaviour on my side on 2.34.1. > I did not bisect manually but I'm pretty sure it's a regression caused by 340062243a (pull: cleanup autostash > check, 2021-06-17) (author CC'ed). I checked that the parent of that commit passes all 4 of your added tests, provided > this is squashed in: > > diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh > index 4046a74cad..5ad19b1028 100755 > --- a/t/t5521-pull-options.sh > +++ b/t/t5521-pull-options.sh > @@ -260,7 +260,6 @@ test_expect_success 'git pull --rebase --autostash succeeds on ff' ' > 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 > ' > > @@ -273,7 +272,6 @@ test_expect_success 'git pull --rebase with rebase.autostash succeeds on ff' ' > 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 > ' > > After that commit, in case of fast-forward, 'git pull --rebase --autostash' delegates the fast-forward > operation to 'git merge' under the hood, which was not the case before. The '--autostash' flag seems > to be forwarded correctly to that 'git merge' invocation, but the config 'rebase.autostash' seems to not > be passed along. > > I did not yet look into why in the code itself. That does explain however why 'merge.autostash' makes it > work - the 'git merge' invocation does check its own config, and if merge.autostash is there the autostash > behaviour is activated. > > Philippe. > >
Hi John, Le 2022-01-04 à 13:49, John Cai a écrit : > >> On Jan 4, 2022, at 1:29 PM, Philippe Blain <levraiphilippeblain@gmail.com <mailto:levraiphilippeblain@gmail.com>> wrote: >> >> Hi again, >> >> Le 2022-01-04 à 13:03, Philippe Blain a écrit : >>> Hi Tilman, >>> Le 2022-01-04 à 07:59, Philip Oakley a écrit : >>>> On 03/01/2022 18:08, Tilman Vogel wrote: >>>>> Hi git-people, >>>>> >>>>> I ran into strange behavior when having rebase.autostash enabled and >>>>> doing a git pull --rebase: >>>>> >>>>>> git config rebase.autostash true >>>>>> git pull --rebase >>>>> Updating cd9ff8a..f3c9840 >>>>> error: Your local changes to the following files would be overwritten by >>>>> merge: >>>>> content >>>>> Please commit your changes or stash them before you merge. >>>>> Aborting >>>>> >>>>> Confusingly, this fixes the issue: >>>>> >>>>>> git config merge.autostash true >>>>>> git pull --rebase >>>>> Updating cd9ff8a..f3c9840 >>>>> Created autostash: c615fda >>>>> Fast-forward >>>>> content | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> Applied autostash. >>>>> >>>>> Leaving me wonder why merge config options fix rebase behavior. >>>>> >>>>> So, in order to make it easier to check the problem, I added some >>>>> test-cases to the git test-suite. Please see the attached patch. >>> Thanks, this really makes it easier to bisect the issue. >>>>> >>>>> Or here: >>>>> https://github.com/tvogel/git/commit/bc941f9357518a34cfa11788dfb8e7fa7f711705 <https://github.com/tvogel/git/commit/bc941f9357518a34cfa11788dfb8e7fa7f711705> >>>>> >>>>> I did not try to find the root-cause as I am not experienced with the >>>>> code-base but if there are questions, let me know. >>>> >>>> Which version are you running? >>>> >>> That's a good info to include indeed. I'm guessing you are using v2.34.1 as that's the version >>> indicated at the bottom of your attached patch. I can replicate the behaviour on my side on 2.34.1. >>> I did not bisect manually but I'm pretty sure it's a regression caused by 340062243a (pull: cleanup autostash >>> check, 2021-06-17) (author CC'ed). I checked that the parent of that commit passes all 4 of your added tests, provided >>> this is squashed in: >>> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh >>> index 4046a74cad..5ad19b1028 100755 >>> --- a/t/t5521-pull-options.sh >>> +++ b/t/t5521-pull-options.sh >>> @@ -260,7 +260,6 @@ test_expect_success 'git pull --rebase --autostash succeeds on ff' ' >>> 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 >>> ' >>> @@ -273,7 +272,6 @@ test_expect_success 'git pull --rebase with rebase.autostash succeeds on ff' ' >>> 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 >>> ' >>> After that commit, in case of fast-forward, 'git pull --rebase --autostash' delegates the fast-forward >>> operation to 'git merge' under the hood, which was not the case before. The '--autostash' flag seems >>> to be forwarded correctly to that 'git merge' invocation, but the config 'rebase.autostash' seems to not >>> be passed along. >>> I did not yet look into why in the code itself >> >> After looking at it a bit, this seems to fix the bug: >> >> diff --git a/builtin/pull.c b/builtin/pull.c >> index 1cfaf9f343..0b206bf1d5 100644 >> --- a/builtin/pull.c >> +++ b/builtin/pull.c >> @@ -87,6 +87,7 @@ static char *opt_verify_signatures; >> static char *opt_verify; >> static int opt_autostash = -1; >> static int config_autostash; >> +static int autostash = -1; >> static int check_trust_level = 1; >> static struct strvec opt_strategies = STRVEC_INIT; >> static struct strvec opt_strategy_opts = STRVEC_INIT; >> @@ -687,9 +688,9 @@ static int run_merge(void) >> strvec_pushv(&args, opt_strategy_opts.v); >> if (opt_gpg_sign) >> strvec_push(&args, opt_gpg_sign); >> -if (opt_autostash == 0) >> +if (autostash == 0) >> strvec_push(&args, "--no-autostash"); >> -else if (opt_autostash == 1) >> +else if (autostash == 1) >> strvec_push(&args, "--autostash"); >> if (opt_allow_unrelated_histories > 0) >> strvec_push(&args, "--allow-unrelated-histories"); >> @@ -1036,7 +1037,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) >> oidclr(&orig_head); >> if (opt_rebase) { >> -int autostash = config_autostash; >> +autostash = config_autostash; >> if (opt_autostash != -1) >> autostash = opt_autostash; It seems your email client is messing up whitespace (I'm guessing you might be using the Gmail web UI, it's known to have this problem), you should try to find a email client that does not have this behaviour :) >> >> >> If nobody beats me to it (if that's the case, be my guest !), I'll try to submit a proper patch later >> today or this week. > > (Resending due to my previous message containing HTML and being rejected by the mail server) It seems this here email did not either reach the mailing list archive. > > Hi Philippe, > > Looks like we were working on this in parallel :) I have a PR I was about to submit as a patch through GGG: github.com/git/git/pull/1179 <http://github.com/git/git/pull/1179> > > My fix is very similar to yours. I can add you ad a co-author if you’d like? > Thanks for asking. I saw you sent your patch at [1] with a "Co-authored-by" trailer. I would have prefered that you wait for my approbation before adding that "Co-authored-by", since the code you are adding is different than what I posted above. I'll comment some more on your patch there. Cheers, Philippe. [1] https://lore.kernel.org/git/20220104214522.10692-1-johncai86@gmail.com/T/#t
From bc941f9357518a34cfa11788dfb8e7fa7f711705 Mon Sep 17 00:00:00 2001 From: Tilman Vogel <tilman.vogel@web.de> Date: Mon, 3 Jan 2022 18:31:59 +0100 Subject: [PATCH] t5521: Show inconsistent rebase behavior (--autostash vs. rebase.autostash) When pull --rebase can be done as a fast-forward, the rebase.autostash config is neglected such that the second test-case added in this commit will fail to autostash and succeed. In fact, setting merge.autostash as a workaround would cause autostash to be done. The other three test-cases added in this commit document that autostashing works as expected when either requested explicitly with --autostash or when pull --rebase cannot be done as a fast-forward. Signed-off-by: Tilman Vogel <tilman.vogel@web.de> --- t/t5521-pull-options.sh | 52 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) 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 -- 2.34.1