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

[Philippe Blain <levraiphilippeblain@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.

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