| Message ID | 20250318180251.3712-1-taahol@utu.fi (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | format-patch: use raw format for notes | expand |
On 2025-03-18 at 18:02:51, Tuomas Ahola wrote: > The default formatting of commit notes by git format-patch --notes > doesn't make a very good fit. It would be more beneficial to use the > raw format for CMIT_FMT_EMAIL and CMIT_FMT_MBOXRD. I don't really use notes, so I don't have a strong opinion, but I think "doesn't make a very good fit" isn't really a compelling argument, since it's very opinionated and short on details. Maybe you could explain the current status in terms of the output one receives and mention in detail why it's unsuitable, and then explain the benefits of the raw format in terms of its output and why it's better. Ideally, I, someone who has touched the notes code but is not intimately familiar with it, would be able to understand the advantages and disadvantages of the change by reading the commit message, and I'm afraid I don't right now. My guess, based on the very small amount of code I've touched there and my recollection from that, is that there's some sort of prefix printed in the format-patch output, and that prevents the notes output from being nicely formatted as an additional explainer when sending a patch, so it requires further editing, which is a hassle. Therefore, it would be more convenient for users to not have to do that by using the raw mode. But that's just a guess.
Tuomas Ahola <taahol@utu.fi> writes: > The default formatting of commit notes by git format-patch --notes > doesn't make a very good fit. It would be more beneficial to use the > raw format for CMIT_FMT_EMAIL and CMIT_FMT_MBOXRD. Hmph. That is unfortunately quite subjective. "doesn't make a very good fit" why? "more benefitial" why? And it turns out that using "raw" is not a good choice in the context of e-mailed patches. Read on. > Signed-off-by: Tuomas Ahola <taahol@utu.fi> > --- > log-tree.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/log-tree.c b/log-tree.c > index 8b184d6776..c40a7599d0 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -857,7 +857,9 @@ void show_log(struct rev_info *opt) > int raw; > struct strbuf notebuf = STRBUF_INIT; > > - raw = (opt->commit_format == CMIT_FMT_USERFORMAT); > + raw = (opt->commit_format == CMIT_FMT_USERFORMAT || > + opt->commit_format == CMIT_FMT_EMAIL || > + opt->commit_format == CMIT_FMT_MBOXRD); After applying this patch and running $ git format-patch --notes=amlog -1 (where refs/notes/amlog holds commit to original e-mail mapping), I get this: ... Subject: [PATCH] format-patch: use raw format for notes ... Signed-off-by: Tuomas Ahola <taahol@utu.fi> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Notes (amlog): Message-Id: <20250318180251.3712-1-taahol@utu.fi> log-tree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) ... But with this patch in place, I instead get this: ... Subject: [PATCH] format-patch: use raw format for notes ... Signed-off-by: Tuomas Ahola <taahol@utu.fi> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Message-Id: <20250318180251.3712-1-taahol@utu.fi> log-tree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) ... There is no indication where the note came from, and more importantly, the contents of the note loses its crucial leading spaces that makes sure that any random lines in the note that happen to begin with "diff", "---", etc. are not mistaken as the beginning of the first patch. So, no, this change is not a good thing to do, at least in its current form. Besides, unconditional change like this will break existing users.
From: Junio C Hamano <gitster@pobox.com> Subject: Re: [PATCH] format-patch: use raw format for notes Date: Tue, 18 Mar 2025 14:17:32 -0700 > [--] more importantly, the contents of the note loses its crucial > leading spaces that makes sure that any random lines in the note > that happen to begin with "diff", "---", etc. are not mistaken as > the beginning of the first patch. Thanks for quick response. That was indeed a compelling point. > So, no, this change is not a good thing to do, at least in its > current form. Besides, unconditional change like this will break > existing users. I see that similar patch was proposed in 2017. I should have searched more thoroughly, I guess. --Tuomas A.
Tuomas Ahola <taahol@utu.fi> writes: > From: Junio C Hamano <gitster@pobox.com> > Subject: Re: [PATCH] format-patch: use raw format for notes > Date: Tue, 18 Mar 2025 14:17:32 -0700 > >> [--] more importantly, the contents of the note loses its crucial >> leading spaces that makes sure that any random lines in the note >> that happen to begin with "diff", "---", etc. are not mistaken as >> the beginning of the first patch. > > Thanks for quick response. That was indeed a compelling point. > >> So, no, this change is not a good thing to do, at least in its >> current form. Besides, unconditional change like this will break >> existing users. > > I see that similar patch was proposed in 2017. I should have searched > more thoroughly, I guess. Heh, your archive spelunking skills are far superiour than mine, it seems. And in https://lore.kernel.org/git/xmqqingw8ppj.fsf@gitster.mtv.corp.google.com/ I see that I said exactly the same thing to exactly the same patch. It is not to say that I've been a good person to be very consistent (I do not have to be---over the years I can hear more opinions from others that may sway how I think about the same issue), but says that there aren't new arguments to sway the old decision in the past 7.5 years. And exactly the same way as back then, I am open to a valid argument to add such an output as an optional feature if there is a good use case for it. Thanks.
diff --git a/log-tree.c b/log-tree.c index 8b184d6776..c40a7599d0 100644 --- a/log-tree.c +++ b/log-tree.c @@ -857,7 +857,9 @@ void show_log(struct rev_info *opt) int raw; struct strbuf notebuf = STRBUF_INIT; - raw = (opt->commit_format == CMIT_FMT_USERFORMAT); + raw = (opt->commit_format == CMIT_FMT_USERFORMAT || + opt->commit_format == CMIT_FMT_EMAIL || + opt->commit_format == CMIT_FMT_MBOXRD); format_display_notes(&commit->object.oid, ¬ebuf, get_log_output_encoding(), raw); ctx.notes_message = strbuf_detach(¬ebuf, NULL);
The default formatting of commit notes by git format-patch --notes doesn't make a very good fit. It would be more beneficial to use the raw format for CMIT_FMT_EMAIL and CMIT_FMT_MBOXRD. Signed-off-by: Tuomas Ahola <taahol@utu.fi> --- log-tree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e