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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi John and Junio

On 04/01/2022 22:46, Junio C Hamano wrote:
> John Cai <johncai86@gmail.com> writes:
> 
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 100cbf9fb8..fb700c2d39 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -86,7 +86,8 @@ static char *opt_ff;
>>   static char *opt_verify_signatures;
>>   static char *opt_verify;
>>   static int opt_autostash = -1;
>> -static int config_autostash;
>> +static int rebase_autostash = 0;
>> +static int cfg_rebase_autostash;
> 
> Do not explicitly initialize statics to '0' (or NULL for that matter).
> 
> But more importantly, I have a feeling that you are making a piece
> of code that is already hard to read impossible to follow by adding
> yet another variable.
> 
>> 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' '
> 
> Missing blank line between tests.
> 
> 
> I thought that the root cause of the problem is that run_merge() is
> called even when rebase was asked for (either via pull.rebase=true
> configuration or "pull --rebase" option), when the other side is a
> descendant of HEAD.  The basic idea behind that behaviour may be
> sound (i.e. if we do not have any of our own development on top of
> their history, rebase vs merge shouldn't make any difference while
> fast-forwarding HEAD to their history), except that rebase vs merge
> look at different configuration variables.
> 
> I wonder if the following two-liner patch is a simpler fix that is
> easier to understand.  run_merge() decides if we should pass the
> "--[no-]autostash" option based on the value of opt_autostash, and
> we know the value of rebase.autostash in config_autostash variable.
> 
> It appears to pass all four tests your patch adds.

I think this is a better approach - it's simpler and it is clear that we 
only use rebase.autostash when a rebase was requested.

Best Wishes

Phillip

>   builtin/pull.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git c/builtin/pull.c w/builtin/pull.c
> index 100cbf9fb8..d459a91a64 100644
> --- c/builtin/pull.c
> +++ w/builtin/pull.c
> @@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>   			die(_("cannot rebase with locally recorded submodule modifications"));
>   
>   		if (can_ff) {
> -			/* we can fast-forward this without invoking rebase */
> +			/*
> +			 * We can fast-forward without invoking
> +			 * rebase, by calling run_merge().  But we
> +			 * have to allow rebase.autostash=true to kick
> +			 * in.
> +			 */
> +			if (opt_autostash < 0)
> +				opt_autostash = config_autostash;
>   			opt_ff = "--ff-only";
>   			ret = run_merge();
>   		} else {
>