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

[Sam Bobroff <sam.bobroff@au1.ibm.com>]

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