From: Phillip Wood Date: Wed, 11 Mar 2026 10:32:50 GMT Subject: Re: [PATCH v7 4/5] format-patch: add commitListFormat config Message-ID: <3fb4baf7-a820-401d-815b-a0b7c11fe6c3@gmail.com> In-Reply-To: On 10/03/2026 16:45, Junio C Hamano wrote: > Phillip Wood writes: > >>> Possible values: >>> - commitListFormat is set but no string is passed: it will default to >>> "[%(count)/%(total)] %s" >> >> It is unusual for an empty config value to mean something different from >> it not being set. The reason for this is that it allows >> >> git -c config.key some-command >> >> to act as though config.key was not set. > > That syntax is the same as setting config.key=true; disabling the > feature triggered by config.key is quite counter-intuitive, isn't > it? I'd forgotten about the boolean case, I was thinking about an empty or missing value clearing multi-valued keys which is quite common I think. Anyway we seem to agree that we don't want to use a missing value to signal a default here. The suggestion below using true and false seems quite reasonable to me. Thanks Phillip > We are by default using "shortlog", but use of this configuration > variable is a sign that the user wants to use a more modern custom > format that is not the traditional "shortlog". It would be quite > natural to invoke the modern default by setting it to "true" (i.e., > "I want to enable the new format.commitlistformat feature, but I am > not saying which format, and the "log:[%(count)/%(total)] %s" format > is used). > > Perhaps "format.commitlistformat = false" should disable the modern > format and fall back to "shortlog", setting it to true (including > the use of "valueless true" syntax) should enable it and use the > modern default "log:[%c/%t] %s" format, and non-bool text should be > used as a custom specification ("shortlog", or "log:")? > > I.e. > > switch (git_parse_maybe_bool_text(value)) { > case 0: /* false */ > fmt_cover_letter_commit_list = "shortlog"; > break; > case 1: /* true - use the modern default format */ > fmt_cover_letter_commit_list = "log:[%c/%t] %s"; > break; > default: > fmt_cover_letter_commit_list = value; > break; > } > > Hmm? > > >> It would be nice to support a default format on the commandline as well. > > >> >>> - if a string is passed: will use it as a format spec. Note that this >>> is either "shortlog" or a format spec prefixed by "log:" >>> e.g."log:%s (%an)" >> >> Having the config value behave like --cover-letter-format= is >> sensible >> >>> - if commitListFormat is not set: it will default to the shortlog >>> format. >> >> makes sense >> >> Thanks >> >> Phillip >> >>> Signed-off-by: Mirko Faina >>> --- >>> builtin/log.c | 21 ++++++++++++++++ >>> t/t4014-format-patch.sh | 53 +++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 74 insertions(+) >>> >>> diff --git a/builtin/log.c b/builtin/log.c >>> index 95e5d9755f..5fec0ddaf9 100644 >>> --- a/builtin/log.c >>> +++ b/builtin/log.c >>> @@ -886,6 +886,7 @@ struct format_config { >>> char *signature; >>> char *signature_file; >>> enum cover_setting config_cover_letter; >>> + char *fmt_cover_letter_commit_list; >>> char *config_output_directory; >>> enum cover_from_description cover_from_description_mode; >>> int show_notes; >>> @@ -930,6 +931,7 @@ static void format_config_release(struct format_config *cfg) >>> string_list_clear(&cfg->extra_cc, 0); >>> strbuf_release(&cfg->sprefix); >>> free(cfg->fmt_patch_suffix); >>> + free(cfg->fmt_cover_letter_commit_list); >>> } >>> >>> static enum cover_from_description parse_cover_from_description(const char *arg) >>> @@ -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); >>> + if (value) >>> + strbuf_addstr(&tmp, value); >>> + else >>> + strbuf_addstr(&tmp, "log:[%(count)/%(total)] %s"); >>> + >>> + FREE_AND_NULL(cfg->fmt_cover_letter_commit_list); >>> + git_config_string(&cfg->fmt_cover_letter_commit_list, var, tmp.buf); >> >> >> >>> + 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); >>> @@ -2329,6 +2344,12 @@ int cmd_format_patch(int argc, >>> goto done; >>> total = list.nr; >>> >>> + if (!cover_letter_fmt) { >>> + cover_letter_fmt = cfg.fmt_cover_letter_commit_list; >>> + if (!cover_letter_fmt) >>> + cover_letter_fmt = "shortlog"; >>> + } >>> + >>> if (cover_letter == -1) { >>> if (cfg.config_cover_letter == COVER_AUTO) >>> cover_letter = (total > 1); >>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh >>> index 458da80721..4891389a53 100755 >>> --- a/t/t4014-format-patch.sh >>> +++ b/t/t4014-format-patch.sh >>> @@ -428,6 +428,59 @@ test_expect_success 'cover letter no format' ' >>> test_line_count = 1 result >>> ' >>> >>> +test_expect_success 'cover letter config with count, subject and author' ' >>> + test_when_finished "rm -rf patches result" && >>> + test_when_finished "git config unset format.coverletter" && >>> + test_when_finished "git config unset format.commitlistformat" && >>> + git config set format.coverletter true && >>> + git config set format.commitlistformat "log:[%(count)/%(total)] %s (%an)" && >>> + git format-patch -o patches HEAD~2 && >>> + grep -E "^[[[:digit:]]+/[[:digit:]]+] .* \(A U Thor\)" patches/0000-cover-letter.patch >result && >>> + test_line_count = 2 result >>> +' >>> + >>> +test_expect_success 'cover letter config with count and author' ' >>> + test_when_finished "rm -rf patches result" && >>> + test_when_finished "git config unset format.coverletter" && >>> + test_when_finished "git config unset format.commitlistformat" && >>> + git config set format.coverletter true && >>> + git config set format.commitlistformat "log:[%(count)/%(total)] (%an)" && >>> + git format-patch -o patches HEAD~2 && >>> + grep -E "^[[[:digit:]]+/[[:digit:]]+] \(A U Thor\)" patches/0000-cover-letter.patch >result && >>> + test_line_count = 2 result >>> +' >>> + >>> +test_expect_success 'cover letter config commitlistformat set but no format' ' >>> + test_when_finished "rm -rf patches result" && >>> + test_when_finished "git config unset format.coverletter" && >>> + test_when_finished "git config unset format.commitlistformat" && >>> + git config set format.coverletter true && >>> + printf "\tcommitlistformat" >> .git/config && >>> + git format-patch -o patches HEAD~2 && >>> + grep -E "^[[[:digit:]]+/[[:digit:]]+] .*" patches/0000-cover-letter.patch >result && >>> + test_line_count = 2 result >>> +' >>> + >>> +test_expect_success 'cover letter config commitlistformat set to shortlog' ' >>> + test_when_finished "rm -rf patches result" && >>> + test_when_finished "git config unset format.coverletter" && >>> + test_when_finished "git config unset format.commitlistformat" && >>> + git config set format.coverletter true && >>> + git config set format.commitlistformat shortlog && >>> + git format-patch -o patches HEAD~2 && >>> + grep -E "^A U Thor \([[:digit:]]+\)" patches/0000-cover-letter.patch >result && >>> + test_line_count = 1 result >>> +' >>> + >>> +test_expect_success 'cover letter config commitlistformat not set' ' >>> + test_when_finished "rm -rf patches result" && >>> + test_when_finished "git config unset format.coverletter" && >>> + git config set format.coverletter true && >>> + git format-patch -o patches HEAD~2 && >>> + grep -E "^A U Thor \([[:digit:]]+\)" patches/0000-cover-letter.patch >result && >>> + test_line_count = 1 result >>> +' >>> + >>> test_expect_success 'reroll count' ' >>> rm -fr patches && >>> git format-patch -o patches --cover-letter --reroll-count 4 main..side >list &&