Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 30, 2022, 15:09 UTC
Message-ID: <85278536-0c81-f903-bb87-1ff14d5dbafa@gmail.com>
In-Reply-To: <20220824220945.1683473-1-jonathantanmy@google.com>

Hi Jonathan

On 24/08/2022 23:09, Jonathan Tan wrote:

Show 29 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -1240,6 +1241,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		if (options.root)
>>   			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
>>   	}
>> +	/*
>> +	 * --keep-base defaults to --reapply-cherry-picks as it is confusing if
>> +	 * commits disappear when using this option.
>> +	 */
>> +	if (options.reapply_cherry_picks < 0)
>> +		options.reapply_cherry_picks = keep_base;
> 
> Here, we set options.reapply_cherry_picks to 1 if keep_base is 1, but...
> 
>> @@ -1416,7 +1423,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   	if (options.empty != EMPTY_UNSPECIFIED)
>>   		imply_merge(&options, "--empty");
>>   
>> -	if (options.reapply_cherry_picks)
>> +	/*
>> +	 * --keep-base implements --reapply-cherry-picks by altering upstream so
>> +	 * it works with both backends.
>> +	 */
>> +	if (options.reapply_cherry_picks && !keep_base)
>>   		imply_merge(&options, "--reapply-cherry-picks");
> 
> ...if we implement --reapply-cherry-picks by altering upstream (and
> hence not need to rely on the "merge" backend), shouldn't we suppress
> the setting of options.reapply_cherry_picks too?

I'm not quite sure what you're suggesting. We only alter upstream if options.reapply_cherry_picks is true which happens below this hunk, are you saying we should clear options.reapply_cherry_picks when we change upstream? I'm not sure that has any practical effect as the left hand side of options.upstream...options.orig_head will be empty so when we do the revision walk to generate the todo list.

Best Wishes

Phillip