Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits

From: Junio C Hamano <junkio@cox.net>
Date: May 27, 2007, 20:09 UTC
Message-ID: <7vejl2do5w.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <200705271608.02122.johan@herland.net>

Johan Herland <johan@herland.net> writes:

> I've been working on combining tag objects and --decorate into a useful
> proof-of-concept that provides the after-the-fact commit annotations I
> requested above, and here's the result:
> ...

Very nicely presented. I appreciate [PATCH 0/N] summary for a series like this that talks about not just what and how it does, but focus more on why it does it in a particular way, on design issues, and decisions. It makes it very pleasant to comment on the series to have a well written summary like this, because in the early stage of a review cycle, we would want to talk about the design, ignoring implementation details.

Show 5 quoted lines

> However, there are still some remaining questions:
>
> - Is the creation of a unique tag name (i.e. the 'tag' field _inside_ the
>   object) for note objects really necessary? Which parts of the system rely
>   on these names to be unique across tag objects?

None. Although your use of note-X{40} would make "recovering" easier, I would imagine.

> - Should notes have their own object type instead of piggy-backing on "tag"?
>
> - What about having a note object type with minimal header fields, and make
>   tags just a special case of a note object with an extra tag name header?

Two things struck me:

 - The "reverse" mapping 'note' tag uses uses FS as a database.
   Clever but would not scale well.

 - *BUT* the reverse mapping 'note' tag uses is exactly what we
    would want to use for tags that are not notes, for tools
    like gitk to attach little flags to commits.

So maybe we _should_ not even need to call this a new 'notes' system. I am just saying your second point above in a different way, but I suspect all what is needed is to make the filtering and presentation of tag objects a bit more flexible, and making use of the 'peeled' (see .git/packed-refs, for example) mapping ultra cheap for ordinary tags.

What you would need are:

 - Ultra-cheap reverse lookup: "I have an object; which tags
   point at this?"

   You solved it with refs/notes/<sha1-of-tagged>/, but it would
   be useful to extend this to any tag in general.

 - Easy way to filter the above question: "I have an object;
   which tags of this particlar class point at this?"

   You are essentially prefiltering by only making notes tags
   available via refs/notes/ hierarchy, but you can also filter
   with your naming convention "tag notes-<sha1>".  We _could_
   (I am not seriously suggesting this yet as I haven't thought
   through the issues yet) allow "keyword" header to tag
   objects, and allow --decorate='kwd1,kwd2,...' to limit the
   tags we take object decorations from the ones that has one of
   the specified keywords.  If we do that, I suspect your
   'notes' system would naturally fall out as regular tags that
   happen to have a keyword header with 'note' on it.

> - What about notes on notes? How are tags on tags treated? How should they be
>   treated?

You can create a tag object that points at another tag. When we peel a tag using "^{}" notation, currently it is peeled all the way until we hit a non tag, so if you need to look at intermediate tags, you need to parse them yourself. IOW, there is no Porcelain feature to let you take advantage of this capability, easily. But we could use this to say "I attest that he tagged that object".

> - Currently noted objects (notees?) are treated as reachable from their
>   associated notes, i.e. like tags. This means that an otherwise dangling
>   object will not be detected and removed if it has associated notes.

I think it largely depends on how you intend to use 'notes' if this is a problem.

The reachability issue is not limited to cruft removal; it also affects the fetching.

I would imagine the intended use of a 'note' is to annotate objects (mostly commits, but not necessarily) after the fact, e.g. "this is reported to fix the issue 35833". You may or may not want to propagate this across repositories, and the way you implemented it is to have a ref pointing at the note -- then it would make it fetchable by other people. Otherwise it would not. With your implementation, unfortunately, not having a ref under refs/notes/ would also make the information unavailable to yourself.

I think the semantics you would want, when a 'note' object that points at another object (pointee) exists, is to allow creating a fake (reverse) reachability that says "pointee points at the note". When such a reachability exists:

 - A fetch from such a repository that transfers out the pointee
   will drag the note attached to it along with it (so you would
   teach rev-list/upload-pack about the extended reachability).

 - Incidentally, having a ref that points at the pointee is
   enough to protect the note that points at the pointee from
   getting pruned (so you would teach fsck/prune about the
   extended reachability).

Othewise, the 'note' is subject to garbage collection. This is pretty much an extension to the existing 'grafts' mechanism, which can only override commit and its parents; what is done in the above is to add (not override) to existing reachability to any object (not just commit, but anything that can get 'noted').