From: Jeff King <peff@peff•net>
To: Mirko Faina <mroik@delayed•space>
Cc: git@vger•kernel.org, Junio C Hamano <gitster@pobox•com>
Subject: Re: [PATCH 2/3] format-patch: add ability to use alt cover format
Date: Tue, 24 Feb 2026 04:02:07 -0500 [thread overview]
Message-ID: <20260224090207.GB986367@coredump.intra.peff.net> (raw)
In-Reply-To: <20260224040400.751247-3-mroik@delayed.space>
On Tue, Feb 24, 2026 at 05:03:57AM +0100, Mirko Faina wrote:
> +static void generate_commit_list_cover(FILE *cover_file, const char *format, struct commit **list, int n)
> +{
OK, so we're expecting "format" here to be the full format string here
(so "%s" or whatever).
But here...
> + get_commit_format(format, &rev);
...this isn't quite the function you want to parse it. This function is
more about parsing the --pretty option for git-log, etc. It allows named
formats like "oneline", "medium", and so on, as well as "format:%s" (and
just "%s", as we treat unknown names with a percent as if they had
tformat: prepended).
As a side effect, it sets up the global user_format variable. Which is
horrible and subtle, but a result of historical function interfaces.
I'll get to that at the bottom of this email.
Back to your patch, I guess here:
> + if (commit_format_is_empty(CMIT_FMT_USERFORMAT))
> + die(_("invalid format spec"));
you are trying to check if we got a USERFORMAT, versus something else. A
more direct way to check that would be:
if (rev->commit_format != CMIT_FMT_USERFORMAT)
But if we do not use get_commit_format() in the first place, we don't
need to worry about that. And I guess you used it because of:
> + pp_commit_easy(CMIT_FMT_USERFORMAT, list[i], &commit_line);
that line, which is using the full-on pretty-print system that git-log,
etc, use. It doesn't really work with CMIT_FMT_USERFORMAT because you
have to set up the global user_format first (for those same historical
reasons).
So I think the entry point you want is repo_format_commit_message(),
which does a one-off custom format into a strbuf without needing a
rev_info, or touching the global user_format, etc.
Something like this:
diff --git a/builtin/log.c b/builtin/log.c
index 5e9e337be4..370367a15a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1326,16 +1326,12 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev)
static void generate_commit_list_cover(FILE *cover_file, const char *format, struct commit **list, int n)
{
struct strbuf commit_line = STRBUF_INIT;
- struct rev_info rev = REV_INFO_INIT;
-
- strbuf_init(&commit_line, 0);
- get_commit_format(format, &rev);
- if (commit_format_is_empty(CMIT_FMT_USERFORMAT))
- die(_("invalid format spec"));
+ struct pretty_print_context ctx = {0};
for (int i = n - 1; i >= 0; i--) {
strbuf_addf(&commit_line, "[%0*d/%d] ", decimal_width(n), n - i, n);
- pp_commit_easy(CMIT_FMT_USERFORMAT, list[i], &commit_line);
+ repo_format_commit_message(the_repository, list[i],
+ format, &commit_line, &ctx);
fprintf(cover_file, "%s\n", commit_line.buf);
strbuf_reset(&commit_line);
}
I think that should make your series do what you want. Now...is the
pretty-print code a horrible minefield of booby traps waiting to spring
on the unwary developer? Yes, it is. ;)
The global user_format thing is there because it was bolted onto the
existing pretty-print code, which used a single enum to store the
format. But that enum isn't enough for a user-format, because we also
have an associated string. The "right" type is probably something like:
struct commit_format {
enum cmit_fmt fmt;
const char *user_format;
};
But C being what it is, switching all of the functions to that breaks
all of the callers which just want to pass CMIT_FMT_ONELINE or whatever.
So we have sort of a split world, where you can use pp_commit_easy()
with the baked-in formats, and using the custom format code uses a
totally different function.
I think in the long run we probably do need to clean up the global
user_format. Two pieces of code interleaving calls to the pretty-printer
would stomp on each other's formats, for example. We've mostly gotten by
because Git, being organized as a set of small programs, generally has
one "main" output (for git-log, etc) and then any smaller outputs within
a program are done by one-off calls to repo_format_commit_message(),
etc.
-Peff
next prev parent reply other threads:[~2026-02-24 9:02 UTC|newest]
Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-20 23:06 [RFC PATCH] format-patch: better commit list for cover letter Mirko Faina
2026-02-20 23:55 ` [RFC PATCH v2] " Mirko Faina
2026-02-21 0:51 ` Mirko Faina
2026-02-21 4:54 ` [RFC PATCH] " Junio C Hamano
2026-02-21 5:18 ` Mirko Faina
2026-02-21 5:55 ` Junio C Hamano
2026-02-21 6:02 ` Junio C Hamano
2026-02-21 15:59 ` Mirko Faina
2026-02-21 17:33 ` Junio C Hamano
2026-02-21 19:16 ` Mirko Faina
2026-02-24 4:03 ` [PATCH 0/3] format-patch: add cover-letter-format option Mirko Faina
2026-02-24 4:06 ` Mirko Faina
2026-02-24 9:29 ` [PATCH v2 0/2] " Mirko Faina
2026-02-24 9:29 ` [PATCH v2 1/2] format-patch: add ability to use alt cover format Mirko Faina
2026-02-24 17:40 ` Junio C Hamano
2026-02-24 23:54 ` Mirko Faina
2026-02-25 0:29 ` Junio C Hamano
2026-02-25 13:47 ` Jeff King
2026-02-24 20:25 ` Junio C Hamano
2026-02-25 13:56 ` Jeff King
2026-02-25 22:55 ` Mirko Faina
2026-02-24 9:29 ` [PATCH v2 2/2] format-patch: add commitListFormat config Mirko Faina
2026-02-24 18:07 ` Junio C Hamano
2026-02-25 0:14 ` Mirko Faina
2026-02-25 17:25 ` Junio C Hamano
2026-02-26 21:40 ` Mirko Faina
2026-02-26 22:19 ` Junio C Hamano
2026-02-24 20:38 ` [PATCH v2 0/2] format-patch: add cover-letter-format option Junio C Hamano
2026-02-24 21:39 ` Junio C Hamano
2026-02-25 0:19 ` Mirko Faina
2026-02-25 2:46 ` Junio C Hamano
2026-02-27 1:52 ` [PATCH v3 0/4] " Mirko Faina
2026-02-27 1:52 ` [PATCH v3 1/4] pretty.c: add %(count) and %(total) placeholders Mirko Faina
2026-02-27 1:52 ` [PATCH v3 2/4] format-patch: move cover letter summary generation Mirko Faina
2026-02-27 1:52 ` [PATCH v3 3/4] format-patch: add ability to use alt cover format Mirko Faina
2026-02-27 4:23 ` Junio C Hamano
2026-02-27 12:41 ` Mirko Faina
2026-02-27 1:52 ` [PATCH v3 4/4] format-patch: add commitListFormat config Mirko Faina
2026-02-27 13:18 ` [PATCH v4 0/4] format-patch: add cover-letter-format option Mirko Faina
2026-02-27 13:18 ` [PATCH v4 1/4] pretty.c: add %(count) and %(total) placeholders Mirko Faina
2026-02-27 13:18 ` [PATCH v4 2/4] format-patch: move cover letter summary generation Mirko Faina
2026-02-27 13:18 ` [PATCH v4 3/4] format-patch: add ability to use alt cover format Mirko Faina
2026-02-27 13:18 ` [PATCH v4 4/4] format-patch: add commitListFormat config Mirko Faina
2026-02-27 16:42 ` [PATCH v4 5/4] docs: add usage for the cover-letter fmt feature Mirko Faina
2026-02-27 17:51 ` [PATCH v4 4/4] format-patch: add commitListFormat config Junio C Hamano
2026-02-27 21:51 ` Mirko Faina
2026-02-27 22:21 ` Junio C Hamano
2026-02-27 22:48 ` [PATCH v5 0/5] format-patch: add cover-letter-format option Mirko Faina
2026-02-27 22:48 ` [PATCH v5 1/5] pretty.c: add %(count) and %(total) placeholders Mirko Faina
2026-02-27 22:48 ` [PATCH v5 2/5] format-patch: move cover letter summary generation Mirko Faina
2026-02-27 22:48 ` [PATCH v5 3/5] format-patch: add ability to use alt cover format Mirko Faina
2026-02-27 22:48 ` [PATCH v5 4/5] format-patch: add commitListFormat config Mirko Faina
2026-02-27 22:48 ` [PATCH v5 5/5] docs: add usage for the cover-letter fmt feature Mirko Faina
2026-03-06 22:33 ` [PATCH v5 0/5] format-patch: add cover-letter-format option Junio C Hamano
2026-03-06 22:49 ` Mirko Faina
2026-03-06 22:58 ` [PATCH v6 " Mirko Faina
2026-03-06 22:58 ` [PATCH v6 1/5] pretty.c: add %(count) and %(total) placeholders Mirko Faina
2026-03-06 22:58 ` [PATCH v6 2/5] format-patch: move cover letter summary generation Mirko Faina
2026-03-06 22:58 ` [PATCH v6 3/5] format-patch: add ability to use alt cover format Mirko Faina
2026-03-10 22:14 ` Junio C Hamano
2026-03-10 22:32 ` Mirko Faina
2026-03-06 22:58 ` [PATCH v6 4/5] format-patch: add commitListFormat config Mirko Faina
2026-03-06 22:58 ` [PATCH v6 5/5] docs: add usage for the cover-letter fmt feature Mirko Faina
2026-03-06 23:18 ` Junio C Hamano
2026-03-06 23:34 ` [PATCH v7 0/5] format-patch: add cover-letter-format option Mirko Faina
2026-03-06 23:34 ` [PATCH v7 1/5] pretty.c: add %(count) and %(total) placeholders Mirko Faina
2026-03-10 14:32 ` Phillip Wood
2026-03-10 20:55 ` Mirko Faina
2026-03-06 23:34 ` [PATCH v7 2/5] format-patch: move cover letter summary generation Mirko Faina
2026-03-06 23:34 ` [PATCH v7 3/5] format-patch: add ability to use alt cover format Mirko Faina
2026-03-10 14:33 ` Phillip Wood
2026-03-10 21:05 ` Mroik
2026-03-06 23:34 ` [PATCH v7 4/5] format-patch: add commitListFormat config Mirko Faina
2026-03-10 14:34 ` Phillip Wood
2026-03-10 16:45 ` Junio C Hamano
2026-03-10 21:23 ` Mirko Faina
2026-03-11 10:38 ` Phillip Wood
2026-03-11 17:13 ` Junio C Hamano
2026-03-11 10:32 ` Phillip Wood
2026-03-11 17:18 ` Junio C Hamano
2026-03-10 21:19 ` Mirko Faina
2026-03-06 23:34 ` [PATCH v7 5/5] docs: add usage for the cover-letter fmt feature Mirko Faina
2026-03-10 9:51 ` Bert Wesarg
2026-03-10 14:34 ` Phillip Wood
2026-03-12 16:20 ` [PATCH v8 0/4] format-patch: add cover-letter-format option Mirko Faina
2026-03-12 16:20 ` [PATCH v8 1/4] format-patch: move cover letter summary generation Mirko Faina
2026-03-12 16:28 ` Junio C Hamano
2026-03-12 16:20 ` [PATCH v8 2/4] format-patch: add ability to use alt cover format Mirko Faina
2026-03-12 16:52 ` Junio C Hamano
2026-03-12 17:18 ` Mirko Faina
2026-03-12 17:25 ` Junio C Hamano
2026-03-12 17:27 ` Junio C Hamano
2026-03-13 10:38 ` Phillip Wood
2026-03-13 17:20 ` Junio C Hamano
2026-03-13 19:17 ` Mirko Faina
2026-03-13 20:22 ` Junio C Hamano
2026-03-12 16:20 ` [PATCH v8 3/4] format-patch: add "chronological" format for cover Mirko Faina
2026-03-12 16:55 ` Junio C Hamano
2026-03-12 16:20 ` [PATCH v8 4/4] format-patch: add commitListFormat config Mirko Faina
2026-03-12 17:00 ` Junio C Hamano
2026-03-12 17:20 ` [PATCH v8 0/4] format-patch: add cover-letter-format option Junio C Hamano
2026-03-12 17:45 ` Mirko Faina
2026-03-12 18:12 ` Junio C Hamano
2026-02-24 4:03 ` [PATCH 1/3] pretty.c: fix null pointer dereference Mirko Faina
2026-02-24 6:25 ` Junio C Hamano
2026-02-24 7:08 ` Mirko Faina
2026-02-24 7:43 ` Mirko Faina
2026-02-24 8:41 ` Jeff King
2026-02-24 4:03 ` [PATCH 2/3] format-patch: add ability to use alt cover format Mirko Faina
2026-02-24 9:02 ` Jeff King [this message]
2026-02-24 9:09 ` Mirko Faina
2026-02-24 9:18 ` Jeff King
2026-02-24 4:03 ` [PATCH 3/3] format-patch: add commitListFormat config Mirko Faina
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260224090207.GB986367@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=mroik@delayed$(echo .)space \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox