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

[Junio C Hamano <gitster@pobox.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".

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

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