Re: [PATCH v4 4/4] format-patch: introduce format.base configuration

From: Ye Xiaolong <xiaolong.ye@intel.com>
Date: Apr 13, 2016, 15:55 UTC
Message-ID: <20160413155558.GA2680@yexl-desktop>
In-Reply-To: <xmqq37qqr4ms.fsf@gitster.mtv.corp.google.com>

On Tue, Apr 12, 2016 at 12:47:23PM -0700, Junio C Hamano wrote:

Show 34 quoted lines

>Xiaolong Ye <xiaolong.ye@intel.com> writes:
>
>> +static int config_base_commit;
>
>This variable is used as a simple boolean whose name is overly broad
>(if it were named "config_base_auto" this complaint would not
>apply).  If you envision possible future enhancements for this
>configuration variable, "int config_base_commit" might make sense
>but I don't think of anything offhand that would be happy with
>"int".
>
>> @@ -786,6 +787,12 @@ static int git_format_config(const char *var, const char *value, void *cb)
>>  	}
>>  	if (!strcmp(var, "format.outputdirectory"))
>>  		return git_config_string(&config_output_directory, var, value);
>> +	if (!strcmp(var, "format.base")){
>
>Style. s/)){/)) {/
>
>> +		if (value && !strcasecmp(value, "auto")) {
>
>Does it make sense to allow "Auto" here?  Given that the command
>line parsing uses strcmp() to require "auto", I do not think so.
>
>> +			config_base_commit = 1;
>> +			return 0;
>> +		}
>
>When a value other than "auto" is given, is it sane to ignore them
>without even warning?
>
>I am wondering if this wants to be a format.useAutoBase boolean
>variable.
>

Thanks for the reminder, seems boolean variable is more suitable for this case, I'll adopt to it in next iteration.

Show 14 quoted lines

>> @@ -1215,7 +1222,12 @@ static void prepare_bases(struct base_tree_info *bases,
>>  	DIFF_OPT_SET(&diffopt, RECURSIVE);
>>  	diff_setup_done(&diffopt);
>>  
>> -	if (!strcmp(base_commit, "auto")) {
>> +	if (base_commit && strcmp(base_commit, "auto")) {
>> +		base = lookup_commit_reference_by_name(base_commit);
>> +		if (!base)
>> +			die(_("Unknown commit %s"), base_commit);
>> +		oidcpy(&bases->base_commit, &base->object.oid);
>> +	} else if ((base_commit && !strcmp(base_commit, "auto")) || config_base_commit) {
>
>It may be a poor design to teach prepare_bases() about "auto" thing.
>Doesn't it belong to the caller?  The caller used to say "If a base

Good point, as I understand your comments, we need to extract the "auto" thing from prepare_bases() and call it early, then we could have a concrete base before calling prepare_bases().

Thanks, Xiaolong.

Show 5 quoted lines

>is given, then call that function, by the way, the base must be a
>concrete one", and with the new "auto" feature, the caller loosens
>the last part of the statement and says "If a base is given, call
>that function, but if it is specified as "auto", I'd have to compute
>it for the user before doing so".