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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2026, 18:07 UTC
Message-ID: <xmqqqzqaggln.fsf@gitster.g>
In-Reply-To: <6a0c7aecfd6dc1ee873d5e81110b723fa2d225fb.1771925291.git.mroik@delayed.space>

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.

Show 14 quoted lines

> @@ -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")) {

Show 11 quoted lines

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

Show 5 quoted lines

> +		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";
	}