From: Philippe Blain Date: Tue, 04 Jan 2022 23:20:27 GMT Subject: Re: [BUG] git pull --rebase ignores rebase.autostash config when fast-forwarding Message-ID: <3d56ab46-bb65-59d0-8b4d-c0e5807814be@gmail.com> In-Reply-To: Hi John, Le 2022-01-04 à 13:49, John Cai a écrit : > >> On Jan 4, 2022, at 1:29 PM, Philippe Blain > 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 >>>>> >>>>> 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 > > 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