Re: [PATCH v2 1/2] format-patch: add ability to use alt cover format
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 24, 2026, 17:40 UTC
- Message-ID
- <xmqqpl5uhwex.fsf@gitster.g>
- In-Reply-To
- <66cac565f8a40f8de3dc3d857feb681bb80cb136.1771925291.git.mroik@delayed.space>
Mirko Faina <mroik@delayed.space> writes:
Show 37 quoted lines
> Often when sending patch series there's a need to clarify to the > reviewer what's the purpose of said series, since it might be difficult > to understand it from reading the commits messages one by one. > > "git format-patch" provides the useful "--cover-letter" flag to declare > if we want it to generate a template for us to use. By default it will > generate a "git shortlog" of the changes, which developers find less > useful than they'd like, mainly because the shortlog groups commits by > author, and gives no obvious chronological order. > > Give the ability to format-patch to specify an alternative format spec > through the "--cover-letter-format" option. This option either takes > "shortlog", which is the current format, or a format spec prefixed with > "log:". > > Example: > git format-patch --cover-letter \ > --cover-letter-format="log:%s (%an)" HEAD~3 > > [1/3] this is a commit summary (Mirko Faina) > [2/3] this is another commit summary (Mirko Faina) > ... > > Signed-off-by: Mirko Faina <mroik@delayed.space> > --- > builtin/log.c | 65 ++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 49 insertions(+), 16 deletions(-) > > diff --git a/builtin/log.c b/builtin/log.c > index c1cd3999a7..5e99660d7c 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1324,13 +1324,32 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev) > } > } > > +static void generate_commit_list_cover(FILE *cover_file,const char *format,
"cover_file,const" -> "cover_file, const" (missing SP).
Show 6 quoted lines
> + struct commit **list, int n)
> +{
> + struct strbuf commit_line = STRBUF_INIT;
> + struct pretty_print_context ctx = {0};
> +
> + strbuf_init(&commit_line, 0);So a single commit_line is given to repo_format_commit_message() repeatedly to accumulate those lines in it, which makes sense.
We prepare pretty_print_context once above and feed it repeatedly to repo_format_commit_message()---is that intended? Not a rhetorical question; I do not know the answer. I guess some existing callers (like builtin/archive.c) do reuse the structure when making multiple calls to the function, so this would be kosher, perhaps?
> + for (int i = n - 1; i >= 0; i--) {
> + strbuf_addf(&commit_line, "[%0*d/%d] ", decimal_width(n), n - i, n);
> + repo_format_commit_message(the_repository, list[i], format, &commit_line, &ctx);Let's line-wrap this overly long line (my lithmus test to complain about "overly long lines" is after losing three columns to the left for "> +" in e-mail quote like the above the right edge of the line does not fit on my 92-column terminal, which will never happen if you stick to the official "fit 80-column after quoted for a few times in e-mail" guideline).
repo_format_commit_message(the_repository, list[i], format, &commit_line, &ctx);
perhaps.
> + fprintf(cover_file, "%s\n", commit_line.buf);
I somehow would have expected that as we internally prepare "format" string given to this function , we ensure it ends with "\n" so we do not have to do a fprintf() here.
> + strbuf_reset(&commit_line); > + } > + fprintf(cover_file, "\n");
OK, so we have a blank line after a block of one line per commit. Sensible.
Show 21 quoted lines
> + strbuf_release(&commit_line);
> +}
> +
> static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> struct commit *origin,
> int nr, struct commit **list,
> const char *description_file,
> const char *branch_name,
> int quiet,
> - const struct format_config *cfg)
> + const struct format_config *cfg,
> + const char *format)
> {
> const char *from;
> struct shortlog log;
> @@ -1342,6 +1361,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> struct commit *head = list[0];
> char *to_free = NULL;
>
> + assert(format);
> +Curious. We do not assert() for any other pointer arguments. What makes this so special?
Show 17 quoted lines
> if (!cmit_fmt_is_mail(rev->commit_format))
> die(_("cover letter needs email format"));
>
> @@ -1377,18 +1398,22 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> free(pp.after_subject);
> strbuf_release(&sb);
>
> - shortlog_init(&log);
> - log.wrap_lines = 1;
> - log.wrap = MAIL_DEFAULT_WRAP;
> - log.in1 = 2;
> - log.in2 = 4;
> - log.file = rev->diffopt.file;
> - log.groups = SHORTLOG_GROUP_AUTHOR;
> - shortlog_finish_setup(&log);
> - for (i = 0; i < nr; i++)
> - shortlog_add_commit(&log, list[i]);It would have been much nicer to first create a short helper function generate_shortlog_cover(), move the above plus a call to shortlog_output(&log) to that helper function, without doing anything else and make it a preliminary patch [1/n]. Then introduce the corresponding generate_commit_list_cover() function in patch [2/n] (which is what this step is doing), which would have resulted in the body of the make_cover_letter() around here a short-and-sweet
if (... we are doing shortlog style ...) generate_shortlog_cover(...); else generate_commit_list_cover(...);
Just like readers of _this_ function are helped by not having to know the details of what happens inside commit-list style cover generation, they can concentratre on the flow without having to care about details on shortlog side that way.
Show 22 quoted lines
> @@ -1906,6 +1931,7 @@ int cmd_format_patch(int argc,
> int just_numbers = 0;
> int ignore_if_in_upstream = 0;
> int cover_letter = -1;
> + char *cover_letter_fmt = NULL;
> int boundary_count = 0;
> int no_binary_diff = 0;
> int zero_commit = 0;
> @@ -1952,6 +1978,8 @@ int cmd_format_patch(int argc,
> N_("print patches to standard out")),
> OPT_BOOL(0, "cover-letter", &cover_letter,
> N_("generate a cover letter")),
> + OPT_STRING(0, "cover-letter-format", &cover_letter_fmt, N_("format-spec"),
> + N_("format spec used for the commit list in the cover letter")),
> OPT_BOOL(0, "numbered-files", &just_numbers,
> N_("use simple number sequence for output file names")),
> OPT_STRING(0, "suffix", &fmt_patch_suffix, N_("sfx"),
> @@ -2289,13 +2317,14 @@ int cmd_format_patch(int argc,
> /* nothing to do */
> goto done;
> total = list.nr;
> +What is this churn about?
> if (cover_letter == -1) {
> if (cfg.config_cover_letter == COVER_AUTO)
> - cover_letter = (total > 1);
> + cover_letter = total > 1;What is this churn about?
> else if ((idiff_prev.nr || rdiff_prev) && (total > 1)) > - cover_letter = (cfg.config_cover_letter != COVER_OFF); > + cover_letter = cfg.config_cover_letter != COVER_OFF;
What is this churn about?
> else > - cover_letter = (cfg.config_cover_letter == COVER_ON); > + cover_letter = cfg.config_cover_letter == COVER_ON;
What is this churn about?
Please do not distract reviewers by mixing immaterial "style fixes" on existing code to a patch whose primary purpose is to introduce new code. A separate "preliminary fix and/or clean-up" patch before the main series begins is often a welcome addition, though.
Show 8 quoted lines
> @@ -2375,12 +2404,16 @@ int cmd_format_patch(int argc, > } > rev.numbered_files = just_numbers; > rev.patch_suffix = fmt_patch_suffix; > + > + if (cover_letter && !cover_letter_fmt) > + cover_letter_fmt = "shortlog"; > +
I do not quite see the point of doing this. There is no law that "cover_letter_fmt != NULL" is a crime when cover_letter is false.
cover_letter_fmt can be initialized to a fixed string "shortlog", and nobody cares what random value cover_letter_fmt has when cover_letter is false.
Show 7 quoted lines
> if (cover_letter) {
> if (cfg.thread)
> gen_message_id(&rev, "cover");
> make_cover_letter(&rev, !!output_directory,
> origin, list.nr, list.items,
> - description_file, branch_name, quiet, &cfg);
> + description_file, branch_name, quiet, &cfg, cover_letter_fmt);This line has become overly long. Please wrap it.
> print_bases(&bases, rev.diffopt.file); > print_signature(signature, rev.diffopt.file); > total++;
In any case, the implementation in this iteration looks much nicer than the previous one.
New set of tests should come together with implementation of a new feature in the same patch.
Thanks.