From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Tue, 04 Jan 2022 18:29:27 GMT
Subject: Re: [BUG] git pull --rebase ignores rebase.autostash config when fast-forwarding
Message-ID: <507d156f-9135-229e-423d-a8ab68bfa9b7@gmail.com>
In-Reply-To: <cb8d6e8a-aaca-c2eb-6cc8-5c4ad33adfdb@gmail.com>

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.