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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 12, 2016, 19:47 UTC
Message-ID: <xmqq37qqr4ms.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1460342873-28900-5-git-send-email-xiaolong.ye@intel.com>

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

Show 5 quoted lines

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

Show 11 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 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".