threads / discuss / 25987

git format-patch should honor notes

Subject: git format-patch should honor notes

## tl;dr

8 messages between Dec 7, 2010 and Dec 8, 2010.

replies: 7people: 7as markdown or json

Eric Blake· Dec 7, 2010, 21:53 UTC · lore

I'm just starting to experiment with 'git notes', because it seems to fit well with my workflow on several projects, except for one drawback.

My workflow is that I post patch series for upstream review via 'git send-email'. Often, that results in feedback that requires me to amend/rebase my series, and post a v2 or v3 of the series. By adding 'git config notes.rewriteRef refs/notes/commits', I can add notes that will carry across my rebase, and remind me what I changed in v2 (for example, git notes add -m 'v2: fix foo, per mail xyz@example.com'). This is handy for me, and I think it is also handy for reviewers - someone who took the time to read through v1 should know what I changed in response to their comments, and only have to focus in on commits with changes, rather than on the entire resent series.

However, I think such review helps are informational only - that is, in 'git send-email' parlance, they belong between the '-- ' and diffstat lines of the email, and not in the upstream commit. After all, once my series is finally accepted upstream, it will no longer be rebased, and 'git bisect' sees only the final version. I see no reason for the commit message to carry the cruft of extra information that was only helpful during reviewing the amended series, nor any reason why upstream should carry around my notes.

So, what I'm missing is the ability for 'git send-email' (or more fundamentally, 'git format-patch') to be able to include contents of a particular (set of) notes reference in each patch file it generates, where the note falls in the informative portion of the email, and is intentionally omitted from the upstream commit when someone else runs 'git am' on my email.

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Junio C Hamano· Dec 7, 2010, 22:10 UTC · re: Eric Blake · lore

Re: git format-patch should honor notes

Eric Blake <eblake@redhat.com> writes:
Show 6 quoted lines
> So, what I'm missing is the ability for 'git send-email' (or more
> fundamentally, 'git format-patch') to be able to include contents of a
> particular (set of) notes reference in each patch file it generates,
> where the note falls in the informative portion of the email, and is
> intentionally omitted from the upstream commit when someone else runs
> 'git am' on my email.

I do not know if "should" is the right word, but it certainly sounds like it would be nice to have such an option for the usecase you described.

Jeff King· Dec 7, 2010, 22:11 UTC · re: Eric Blake · lore

Re: git format-patch should honor notes

On Tue, Dec 07, 2010 at 02:53:09PM -0700, Eric Blake wrote:
Show 10 quoted lines
> My workflow is that I post patch series for upstream review via 'git
> send-email'.  Often, that results in feedback that requires me to
> amend/rebase my series, and post a v2 or v3 of the series.  By adding
> 'git config notes.rewriteRef refs/notes/commits', I can add notes that
> will carry across my rebase, and remind me what I changed in v2 (for
> example, git notes add -m 'v2: fix foo, per mail xyz@example.com').
> This is handy for me, and I think it is also handy for reviewers -
> someone who took the time to read through v1 should know what I changed
> in response to their comments, and only have to focus in on commits with
> changes, rather than on the entire resent series.

Yeah, that is a workflow that some others have mentioned using here, too. And I think there is general agreement that notes should go after the "---" in format-patch. We just need a working patch.

Thomas posted one in February:
  http://article.gmane.org/gmane.comp.version-control.git/140819

But there were some issues and it never got polished. Michael suggested that he does something similar here:

  http://article.gmane.org/gmane.comp.version-control.git/140819

but there was no indication on whether it happens manually or if he has a patch. I don't know if anything else has happened in that area. I'm sure if you feel like working on a patch it would be well received.

-Peff
Michael J Gruber· Dec 8, 2010, 08:20 UTC · re: Jeff King · lore

Re: git format-patch should honor notes

Jeff King venit, vidit, dixit 07.12.2010 23:11:
Show 31 quoted lines
> On Tue, Dec 07, 2010 at 02:53:09PM -0700, Eric Blake wrote:
> 
>> My workflow is that I post patch series for upstream review via 'git
>> send-email'.  Often, that results in feedback that requires me to
>> amend/rebase my series, and post a v2 or v3 of the series.  By adding
>> 'git config notes.rewriteRef refs/notes/commits', I can add notes that
>> will carry across my rebase, and remind me what I changed in v2 (for
>> example, git notes add -m 'v2: fix foo, per mail xyz@example.com').
>> This is handy for me, and I think it is also handy for reviewers -
>> someone who took the time to read through v1 should know what I changed
>> in response to their comments, and only have to focus in on commits with
>> changes, rather than on the entire resent series.
> 
> Yeah, that is a workflow that some others have mentioned using here,
> too. And I think there is general agreement that notes should go after
> the "---" in format-patch. We just need a working patch.
> 
> Thomas posted one in February:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/140819
> 
> But there were some issues and it never got polished. Michael suggested
> that he does something similar here:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/140819
> 
> but there was no indication on whether it happens manually or if he has
> a patch. I don't know if anything else has happened in that area. I'm
> sure if you feel like working on a patch it would be well received.
> 
> -Peff

I do it with ":r!git notes show" in vim (after "/---"), which has the advantage over "format-patch --show-notes" that the notes are not indented nor preceded by a "Notes:" header. (I wouldn't mind the latter.) This is comfortable enough to have kept me from writing a patch.

Also, in order to be really useful, I would need a place to store the cover letter also. I was experimenting a while back with a design for annotating branchnames which "basically" worked but haven't had time to really implement it. If I remember correctly, I had to set up some "bogus" refs to keep my notes from being garbage collected and was still figuring out the best place to put them. I'll dig it up when I have time to.

Michael
Johan Herland· Dec 8, 2010, 10:12 UTC · re: Michael J Gruber · lore

Re: git format-patch should honor notes

On Wednesday 08 December 2010, Michael J Gruber wrote:
Show 7 quoted lines
> Also, in order to be really useful, I would need a place to store the
> cover letter also. I was experimenting a while back with a design for
> annotating branchnames which "basically" worked but haven't had time
> to really implement it. If I remember correctly, I had to set up some
> "bogus" refs to keep my notes from being garbage collected and was
> still figuring out the best place to put them. I'll dig it up when I
> have time to.

I believe the last time the issue of adding notes to branch names was discussed, the consensus was that rather than using notes, they could be stored using a custom entry in the config file, e.g.

  git config branch.mybranch.description "Description of mybranch"
I might have misremembered this, though.
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Michael J Gruber· Dec 8, 2010, 10:24 UTC · re: Johan Herland · lore

Re: git format-patch should honor notes

Johan Herland venit, vidit, dixit 08.12.2010 11:12:
Show 16 quoted lines
> On Wednesday 08 December 2010, Michael J Gruber wrote:
>> Also, in order to be really useful, I would need a place to store the
>> cover letter also. I was experimenting a while back with a design for
>> annotating branchnames which "basically" worked but haven't had time
>> to really implement it. If I remember correctly, I had to set up some
>> "bogus" refs to keep my notes from being garbage collected and was
>> still figuring out the best place to put them. I'll dig it up when I
>> have time to.
> 
> I believe the last time the issue of adding notes to branch names was 
> discussed, the consensus was that rather than using notes, they could 
> be stored using a custom entry in the config file, e.g.
> 
>   git config branch.mybranch.description "Description of mybranch"
> 
> I might have misremembered this, though.

They certainly "could". The question whether they "should" depends on what they are used for:

- config is neither versioned nor easily shareable; perfect for your own
scratch notes to go away once work is done
- notes are versioned and can be shared (I don't need to tell you...);
perfect for longer term annotations you want to keep

Note that "sharing" here includes also pushing to your backup repo and cloning around. I'd certainly put patch series cover letters in the second category.

Michael
Johan Herland· Dec 8, 2010, 10:50 UTC · re: Michael J Gruber · lore

Re: git format-patch should honor notes

On Wednesday 08 December 2010, Michael J Gruber wrote:
Show 30 quoted lines
> Johan Herland venit, vidit, dixit 08.12.2010 11:12:
> > On Wednesday 08 December 2010, Michael J Gruber wrote:
> >> Also, in order to be really useful, I would need a place to store
> >> the cover letter also. I was experimenting a while back with a
> >> design for annotating branchnames which "basically" worked but
> >> haven't had time to really implement it. If I remember correctly,
> >> I had to set up some "bogus" refs to keep my notes from being
> >> garbage collected and was still figuring out the best place to put
> >> them. I'll dig it up when I have time to.
> >
> > I believe the last time the issue of adding notes to branch names
> > was discussed, the consensus was that rather than using notes, they
> > could be stored using a custom entry in the config file, e.g.
> >
> >   git config branch.mybranch.description "Description of mybranch"
> >
> > I might have misremembered this, though.
>
> They certainly "could". The question whether they "should" depends on
> what they are used for:
>
> - config is neither versioned nor easily shareable; perfect for your
> own scratch notes to go away once work is done
>
> - notes are versioned and can be shared (I don't need to tell
> you...); perfect for longer term annotations you want to keep
>
> Note that "sharing" here includes also pushing to your backup repo
> and cloning around. I'd certainly put patch series cover letters in
> the second category.

True. I was wrong to equate cover letters with local-only branch name descriptions.

As has been discussed before, you can use notes to store the cover letter, the question is which SHA-1 to attach it to.

Using a SHA-1 that doesn't exist in the repo (e.g. the SHA-1 of the branch name) leaves the note vulnerable to 'git notes prune', but maybe that is an acceptable restriction ('git notes prune' must be manually invoked in any case). For extra safety, we could add a config option that refuses 'git notes prune' for a given notes ref, something like:

  git config notes.mynotes.refusePrune true
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Thomas Rast· Dec 8, 2010, 11:15 UTC · re: Jeff King · lore

Re: git format-patch should honor notes

Jeff King wrote:
Show 8 quoted lines
> > My workflow is that I post patch series for upstream review via 'git
> > send-email'.  Often, that results in feedback that requires me to
> > amend/rebase my series, and post a v2 or v3 of the series.  By adding
> > 'git config notes.rewriteRef refs/notes/commits', I can add notes that
> > will carry across my rebase, and remind me what I changed in v2 (for
> > example, git notes add -m 'v2: fix foo, per mail xyz@example.com').
> 
> Yeah, that is a workflow that some others have mentioned using here,
Incidentally it's what I wrote the rewriteRef support for :-)
Show 8 quoted lines
> too. And I think there is general agreement that notes should go after
> the "---" in format-patch. We just need a working patch.
> 
> Thomas posted one in February:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/140819
> 
> But there were some issues and it never got polished.

I got pretty frustrated with gfp being rather brittle. It is very hard to insert anything anywhere in the output stream in such a way that the output is not affected in any *other* scenario where this option is disabled.

So I think a good angle of attack if you want to hack around on this would be to clean up gfp so that it becomes easier to work on, and/or come up with a better/cleaner place to insert the notes support than I had.

That being said, the version I still use just shifts around a linefeed after the ---, IIRC, and so far nobody complained about that in practice ;-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

← back to recent threads