Git notes list/show <revision-range>

Hi all,

Considering a work-flow using topic branches just as the git development uses itself, I do miss a certain feature.

When a feature/patch is proposed, it will "cook" for a while in "pu" and "next". During this period people can comment on the patch. These comments can be stored in git notes added to the commits on which is commented.

At a certain point I want to decide whether the feature can be merged into "next" or "master", so I want to collect all notes for a certain branch to see what still has to be done before this feature can be merged. One can think about this of automatically generating a "What's cooking in X" listing the topic branches and the comments for the commits in each branch.

That's why I want to (try to) implement the feature that 'git notes list' and 'git notes show' accept a revision range and collect all notes for the commits in this range.

Do you think this will be a useful feature and that it has chances to be merged into git ?

Vincent

On Mon, Apr 18, 2011 at 08:19:59PM +0200, Vincent van Ravesteijn wrote:

If you haven't already seen it, you might find the "notes/*" refs here:

  git://repo.or.cz/git/trast.git

They reference the messages discussing the patch on the mailing list by message-id and gmane link.

You can do more-or-less what you want with something like:

  git log --format="%h %s%n%N"

Jeff King venit, vidit, dixit 18.04.2011 20:27:

Exactly, the log family does that and more, since it makes the whole revision walk machinery available.

But since the OP is volunteering to code for notes :-) We could need a feature which allows to log the history of a note. The alias

`git noteslog' is aliased to `!sh -c 'git log $(git notes get-ref) "$@"' -'

gives you the history of the notes tree (try it with "-p"), but sometimes I would like the history of the notes to a specific commit, and in

git noteslog -p -- $(commit)

I would have to use for $(commit) all possible breakdowns of the sha1 of the commit for all possible notes tree structures. It feels as of the revision walker needs to learn another pathspec, say

":(note):<sha1>"

in line with our magic pathspec discussion.

Michael

On Tue, Apr 19, 2011 at 09:17:51AM +0200, Michael J Gruber wrote:

Hmm, I just use "git log notes/<whatever>", which works fine. It does help if you know that the default ref is "notes/commits", though.

It's not something I do often, though (most of my notes use has been things that automatically make notes, so the history tends to be uninteresting and useful only for debugging the note-making code).

That's a clever solution. It is a little non-intuitive for a user to need to know about notes storage, though. Maybe you were already thinking this, but we could have something like:

  git notes log [revs] [--] [pathspec]

where "[revs]" are checked for in refs/notes/*, defaulting to "refs/notes/commits". And each element of the pathspec gets the ":(note):" magic automatically. I wonder if we could even resolve the pathspec bits as regular refs.

So you could write:

  # long form, just as you can do with "git log"
  git notes log notes/commits -- ":(note):`git rev-parse HEAD`"

  # or with automagic ref lookup for pathspec
  git notes log notes/commits -- HEAD

  # and automagic default ref
  git notes log -- HEAD

  # and I think you should be able to write a disambiguator similar to
  # what we use for the revs/paths distinction, but this time for
  # notes-refs versus regular refs. And then drop the "--":
  git notes log HEAD

I think it would need a little refactoring of setup_revisions() to be more flexible, but most of the hard work is already done by the usual revision traversal mechanism.

-Peff

Jeff King venit, vidit, dixit 19.04.2011 22:32:

Sure. That's what the alias does.

In particular, the log messages "Notes added by..." are useless. I use notes for patch boiler plates and branch annotations, and in these cases history can be interesting (between versions of a patch, e.g.).

Here we are back at my alias is a first step in that direction... "git notes log" should do all that, of course.

I don't think "notes log" should take a notes ref argument like that. It already has "--ref" for that purpose, so I would suggest

git notes [--ref <notesref>] log [<rev>]

with <rev> being mapped to :(notes):<rev> automatically, <rev> defaulting to HEAD. That is much more in line with the other notes subcommands. (We may or may note check for ":(notes):" being there already.)

? I haven't checked in detail, but I think all we need is:

- Make "git notes --ref <notesref> log <rev>" call "git log
<resolvednotesref> -- :(notes):<rev>" (and pass on log options).

- Make the pathspec ":(notes):<rev>" resolve <rev> to <sha1> and match
with all possible breakdowns of <sha1> which we accept for a notes tree.

Michael

On Wed, Apr 20, 2011 at 09:16:55AM +0200, Michael J Gruber wrote:

Hmm. You are probably right. I was modeling it after "log", but it is a notes subcommand. I was thinking you could also do something like:

  git notes log commits some-other-notes $sha1

or even

  # see what any notes ref has to say about this commit
  git notes log --all $sha1

but it is probably a better idea to keep the interface more consistent with the other notes subcommands.

I think it should be more like:

  git notes [--ref <notesref] log [<rev options>] [<rev> ...]

That is:

  1. There may be arbitrary rev options. Because you might want "log
     -p", or "log --format".

  2. We may want to allow multiple revs (e.g., if you have a patch
     series with 5 commits, you may want to see the history of any notes
     that refer to any of those commits).

I think (1) means we may need to feed the result to the revision options-parser.

I was worried about how to get the log options. If you see:

  git notes log --foo bar

is "bar" a revision, or an argument to --foo? I think right now the answer is always "revision", but that is only because the revision options do not currently use parse-options, which would allow "--format=foo" to be written as "--format foo". So I'd rather not rely on that.

I was assuming that :(notes): would only accept a sha1. But it probably makes sense for it to handle resolution itself.

-Peff