[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