Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges

From: Glen Choo <chooglen@google.com>
Date: Mar 8, 2023, 00:09 UTC
Message-ID: <kl6l4jqwhznr.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <7cf19017-518b-245e-aea2-5dee55f88276@dunelm.org.uk>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 25 quoted lines

>> @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data)
>>   		return 0;
>>   	}
>>   
>> +	if (!strcmp(var, "rebase.rebasemerges")) {
>> +		opts->config_rebase_merges = git_parse_maybe_bool(value);
>> +		if (opts->config_rebase_merges < 0) {
>> +			opts->config_rebase_merges = 1;
>> +			parse_rebase_merges_value(opts, value);
>> +		}
>
> I think we need
>
> 	} else {
> 		opts->rebase_cousins = 0;
> 	}
>
> here. Otherwise if rebase.rebaseMerges is set twice we wont follow the 
> usual "last config wins" convention. For example
>
> 	[rebase]
> 		rebaseMerges=rebase-cousins
> 		rebaseMerges=true
>
> will result in us unexpectedly rebasing cousins

Ah, good catch.

An alternative might be to use the "lookup" functions (e.g. repo_config_get_maybe_bool()), which already implement "last one wins" semantics. It's a bigger change, but it may be easier for future readers to understand. I don't have a strong opinion on which is the 'better' approach.