Re: [PATCH v2 2/2] format-patch: add commitListFormat config

[Junio C Hamano <gitster@pobox.com>]

Mirko Faina <mroik@delayed.space> writes:

> +	char* fmt_cover_letter_commit_list;

In this project, asterisk sticks to the variable, not the type,
i.e.,

	char *fmt_cover_letter_commit_list;

I think you got this point right in the previous patch.

> @@ -1052,6 +1054,19 @@ static int git_format_config(const char *var, const char *value,
>  		cfg->config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
>  		return 0;
>  	}
> +	if (!strcmp(var, "format.commitlistformat")) {
> +		struct strbuf tmp = STRBUF_INIT;
> +		strbuf_init(&tmp, 0);
> +		strbuf_addstr(&tmp, "log:");
> +		if (value)
> +			strbuf_addstr(&tmp, value);
> +		else
> +			strbuf_addstr(&tmp, "%s");
> +
> +		git_config_string(&cfg->fmt_cover_letter_commit_list, var, tmp.buf);

What if /etc/gitconfig has "[format] commitListFormat = shortlog",
~/.gitconfig has a different setting, and then .git/config has yet
another setting?  Woudln't cfg->fmt_cover_letter_commit_list at this
point have a copy of the value read from the previous configuration
file?  Without first freeing it, wouldn't we leak the previous value?

    $ git grep -C2 git_config_string\(

gives plenty of precedence, like this one.

builtin/commit.c-	if (!strcmp(k, "commit.cleanup")) {
builtin/commit.c-		FREE_AND_NULL(cleanup_config);
builtin/commit.c:		return git_config_string(&cleanup_config, k, v);
builtin/commit.c-	}
builtin/commit.c-	if (!strcmp(k, "commit.gpgsign")) {

> +		strbuf_release(&tmp);
> +		return 0;
> +	}
>  	if (!strcmp(var, "format.outputdirectory")) {
>  		FREE_AND_NULL(cfg->config_output_directory);
>  		return git_config_string(&cfg->config_output_directory, var, value);
> @@ -2318,6 +2333,13 @@ int cmd_format_patch(int argc,
>  		goto done;
>  	total = list.nr;
>  
> +	if (cover_letter_fmt && (strcmp(cover_letter_fmt, "shortlog") && strncmp(cover_letter_fmt, "log:", 4))) {

Overly long line.

What if it turns out that the --cover-letter option is not given
(and we are dealing with a single-patch topic, so auto setting has
decided that there is no need for cover letter)?  Shouldn't we
continue ignoring the typo on a setting that we are not going to use
anyway?

Stepping back a bit, even if we do not validate the format *here*,
shouldn't the code that does use cover_letter_fmt later in the
control flow *already* be checking the validity of the format and
complaining?  If that happens early enough, perhaps we do not want
to have an extra "early check and die" here.

> +		die(_("--cover-letter: invalid format spec"));
> +	}
> +
> +	if (!cover_letter_fmt)
> +		cover_letter_fmt = cfg.fmt_cover_letter_commit_list;

As I pointed out in my review of [1/2], it is not a crime to set a
value to cover_letter_fmt even when !cover_letter, and the above
line does exactly that ;-).

By the way, the usual technique used in this codebase when handing
configuration and command line option is to these in this order:

 * Initialize a variable to the built-in hardcoded default (e.g.,
   "shortlog") upon variable declaration.

 * Let repo_config() call overwrite that same variable.  This is the
   typical implementation of "if there is no configuration, we use
   the hardcoded default, but the configured value can override it".

 * Then parse_options() overwrites that same variable.

But because we read configuration into a separarte variable (i.e.,
members of cfg structure), this function cannot literally follow the
usual pattern.  But the pattern we instead can follow is this:

	/* initiailize to NULL */
	char *cover_letter_fmt = NULL;

        /* read configuration */
        repo_config(... &cfg);

	/* cover_letter_fmt will point at command line arg */
	parse_options(...);

        /* NULL if no command line argument */
	if (!cover_letter_fmt) {
		/* perhaps configuration has one */
        	cover_letter_fmt = cfg.fmt_cover_letter_commit_list;

                /* otherwise, use hardcoded default */
                if (!cover_letter_fmt)
                	cover_letter_fmt = "shortlog";
	}