[PATCH] format-patch: use raw format for notes

[PATCH] format-patch: use raw format for notes

[Sam Bobroff]

If "--notes=..." is used with "git format-patch", the notes are
prefixed with the ref's local name and indented, which looks odd and
exposes the path of the ref.

Extend the test that suppresses this behaviour so that it also catches
this case, causing the notes to be included without additional
processing.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---

Notes (foo):
    Hi,
    
    I've noticed what appears to be a small cosmetic bug in git format-patch, as
    I've described in the commit message.
    
    I'm not sure if this patch is the right way to fix it (or perhaps it's not even
    a bug), but it should at least help to explain what I'm talking about.
    
    I've used "git format-patch --notes=foo" to prepare this email so that it is an
    example of the issue :-)
    
    Cheers,
    Sam.

 log-tree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/log-tree.c b/log-tree.c
index 410ab4f02..26bc21ad3 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -655,7 +655,8 @@ 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);
 		format_display_notes(&commit->object.oid, &notebuf,
 				     get_log_output_encoding(), raw);
 		ctx.notes_message = notebuf.len
-- 
2.11.0

Re: [PATCH] format-patch: use raw format for notes

[Junio C Hamano]

Sam Bobroff <sam.bobroff@au1.ibm.com> writes:

> If "--notes=..." is used with "git format-patch", the notes are
> prefixed with the ref's local name and indented, which looks odd and
> exposes the path of the ref.
>
> Extend the test that suppresses this behaviour so that it also catches
> this case, causing the notes to be included without additional
> processing.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
>
> Notes (foo):
>     Hi,
>     
>     I've noticed what appears to be a small cosmetic bug in git format-patch, as
>     I've described in the commit message.
>     
>     I'm not sure if this patch is the right way to fix it (or perhaps it's not even
>     a bug), but it should at least help to explain what I'm talking about.
>     
>     I've used "git format-patch --notes=foo" to prepare this email so that it is an
>     example of the issue :-)
>     
>     Cheers,
>     Sam.

Is the above addition from your 'foo' notes with or without this
patch?  I think the answer is "without", and the above "example"
looks just fine to me.

 - It is very much intended to allow The "(foo)" after the "Notes"
   label to show which notes ref the note comes from, because there
   can be more than one notes refs that annotate the same commit.

 - And the contents are indented, just like the diffstat and other
   stuff we place after "---" but before the first "diff", to ensure
   no matter what text appears there it will not be mistaken as part
   sure that the contents from the notes will not be mistaken as part
   of the patch.

I do not think an unconditional change of the established format,
like your patch does, is acceptable, as existing users have relied
on, and expect to be able to continue relying on, the above two
aspect of the current format.

But I am somewhat curious what your use case that wants to insert
the raw contents there is.  We may be able to construct a valid
argument to add such an output as an optional feature if there is a
good use case for it.

Thanks.

>  log-tree.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index 410ab4f02..26bc21ad3 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -655,7 +655,8 @@ 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);
>  		format_display_notes(&commit->object.oid, &notebuf,
>  				     get_log_output_encoding(), raw);
>  		ctx.notes_message = notebuf.len

Re: [PATCH] format-patch: use raw format for notes

[Sam Bobroff]

On Wed, Sep 06, 2017 at 12:34:48PM +0900, Junio C Hamano wrote:
> Sam Bobroff <sam.bobroff@au1.ibm.com> writes:
> 
> > If "--notes=..." is used with "git format-patch", the notes are
> > prefixed with the ref's local name and indented, which looks odd and
> > exposes the path of the ref.
> >
> > Extend the test that suppresses this behaviour so that it also catches
> > this case, causing the notes to be included without additional
> > processing.
> >
> > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> > ---
> >
> > Notes (foo):
> >     Hi,
> >     
> >     I've noticed what appears to be a small cosmetic bug in git format-patch, as
> >     I've described in the commit message.
> >     
> >     I'm not sure if this patch is the right way to fix it (or perhaps it's not even
> >     a bug), but it should at least help to explain what I'm talking about.
> >     
> >     I've used "git format-patch --notes=foo" to prepare this email so that it is an
> >     example of the issue :-)
> >     
> >     Cheers,
> >     Sam.
> 
> Is the above addition from your 'foo' notes with or without this
> patch?  I think the answer is "without", and the above "example"
> looks just fine to me.

Yes that's correct, it is without the patch.

>  - It is very much intended to allow The "(foo)" after the "Notes"
>    label to show which notes ref the note comes from, because there
>    can be more than one notes refs that annotate the same commit.

Right, that makes perfect sense to me when it's being output locally.

But the ref names are local to my git repo and there is no reaason why
they should be meaningful or even known to the recipients of the patch
email.

>  - And the contents are indented, just like the diffstat and other
>    stuff we place after "---" but before the first "diff", to ensure
>    no matter what text appears there it will not be mistaken as part
>    sure that the contents from the notes will not be mistaken as part
>    of the patch.

I don't quite agree here. I think it would be fine to indent the whole
block by one space, like the diffstat, but the main text is indented an
additional four which looks odd and breaks line wrapping. (Yes, the line
wrapping can be worked around, but still...)

> I do not think an unconditional change of the established format,
> like your patch does, is acceptable, as existing users have relied
> on, and expect to be able to continue relying on, the above two
> aspect of the current format.

Sure and I'm happy look at some kind of optional change but, just out of
curiousity, is there some specific use case that this might break? (Not
that there needs to be one, I agree with the general "don't break
things" approach.)

> But I am somewhat curious what your use case that wants to insert
> the raw contents there is.  We may be able to construct a valid
> argument to add such an output as an optional feature if there is a
> good use case for it.

Perhaps I'm misunderstanding something about it but I just want to
insert text into the comments section automatically, so that I don't
have to post-process the output of format-patch with some horrible sed
script :-)

(If only there were a way to set the coverletter text automatically as
well...)

> Thanks.

Thanks for the reply!
Cheers,
Sam.

> 
> >  log-tree.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/log-tree.c b/log-tree.c
> > index 410ab4f02..26bc21ad3 100644
> > --- a/log-tree.c
> > +++ b/log-tree.c
> > @@ -655,7 +655,8 @@ 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);
> >  		format_display_notes(&commit->object.oid, &notebuf,
> >  				     get_log_output_encoding(), raw);
> >  		ctx.notes_message = notebuf.len

Re: [PATCH] format-patch: use raw format for notes

[Stefan Beller]

On Sun, Sep 10, 2017 at 9:27 PM, Sam Bobroff <sam.bobroff@au1.ibm.com> wrote:

> (If only there were a way to set the coverletter text automatically as
> well...)
>

Checkout branch.<name>.description

Re: [PATCH] format-patch: use raw format for notes

[Sam Bobroff]

On Tue, Sep 12, 2017 at 10:33:37AM -0700, Stefan Beller wrote:
> On Sun, Sep 10, 2017 at 9:27 PM, Sam Bobroff <sam.bobroff@au1.ibm.com> wrote:
> 
> > (If only there were a way to set the coverletter text automatically as
> > well...)
> >
> 
> Checkout branch.<name>.description

AH! I had seen this section of the format-patch man page...

       --[no-]cover-letter
           In addition to the patches, generate a cover letter file containing the branch description, shortlog and the overall diffstat. You can fill in a description in the file before
           sending it out.

... but I didn't realize that it meant that it would insert the text
from branch.<name>.description into the cover letter. Perhaps there
should be a hint to point you to the branch.<name>.description section
of "git config"?

Also, it's not very useful for automating patch submission, as it
doesn't set the subject line or remove the "*** BLURB HERE ***" marker,
so it must still be post-processed.

(To be honest, I was surprised that it didn't use first line as the
subject and leave out the markers.)

Cheers,
Sam.

Re: [PATCH] format-patch: use raw format for notes

[Jeff King]

On Mon, Sep 11, 2017 at 02:27:38PM +1000, Sam Bobroff wrote:

> >  - It is very much intended to allow The "(foo)" after the "Notes"
> >    label to show which notes ref the note comes from, because there
> >    can be more than one notes refs that annotate the same commit.
> 
> Right, that makes perfect sense to me when it's being output locally.
> 
> But the ref names are local to my git repo and there is no reaason why
> they should be meaningful or even known to the recipients of the patch
> email.

I can see how your notes names might not be of interest to others. But I
can also see how they _could_ be. For instance, if you kept test result
annotations in a notes ref, you would want to mark them as such.

The idea of the current output is that you'd put general text into
"refs/notes/commits" (which shows up only as "Notes:"). And if you are
putting notes in another ref, you have some reason to do so, which
implies that it's worth showing that they're not in the default ref.

I grant that there are reasons to do so which might not be worth showing
(e.g., you might be pushing and fetching refs, and keep some hierarchy).
But I don't think "are we emailing them" is a robust determiner of "are
they worth showing". So this probably needs to be a separate option,
rather than tied to the output format.

Or possibly there should be a naming convention (e.g., that everything
that ends in "/commits" doesn't have its name shown, which would allow
multiple hierarchies). It's hard to say without knowing the reason you
chose a non-default refname in the first place.

-Peff

[PATCH] format-patch: use raw format for notes

[Tuomas Ahola]

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(-)

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, &notebuf,
 				     get_log_output_encoding(), raw);
 		ctx.notes_message = strbuf_detach(&notebuf, NULL);

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
-- 
2.30.2

Re: [PATCH] format-patch: use raw format for notes

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

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.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] format-patch: use raw format for notes

[Junio C Hamano]

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.

Re: [PATCH] format-patch: use raw format for notes

[Tuomas Ahola]

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.

Re: [PATCH] format-patch: use raw format for notes

[Junio C Hamano]

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.