Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Jan 5, 2022, 03:40 UTC
Message-ID: <fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com>
In-Reply-To: <20220104214522.10692-2-johncai86@gmail.com>

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.

Show 21 quoted lines

> 
> 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.

Show 19 quoted lines

> 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.

Show 13 quoted lines

> +
> +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.

Show 13 quoted lines

> +
> +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"

Show 14 quoted lines

> +
> +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.