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

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

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