Re: [PATCH] notes: allow merging from arbitrary references

From: Johan Herland <johan@herland.net>
Date: Nov 16, 2015, 07:55 UTC
Message-ID: <CALKQrgdDH2WZc-xi3ROLUBxdk=yVqfFGN3jN1GjQq4qJj_K+-A@mail.gmail.com>
In-Reply-To: <CA+P7+xoyCwgYWaiVj0FNVHuaY=kUZA5a3LBMtpe6SirOVeK9rA@mail.gmail.com>

On Mon, Nov 16, 2015 at 12:23 AM, Jacob Keller <jacob.keller@gmail.com> wrote:

Show 14 quoted lines

> On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland <johan@herland.net> wrote:
>> A related topic that has been discussed (although I cannot remember if
>> any conclusion was reached) is whether to allow more notes operations
>> - specifically _read-only_ operations - on notes trees outside
>> refs/notes/. I believe this should also become possible, although I
>> haven't thoroughly examined all implications.
>
> This was discussed at some point on one of the versions of my patch.
> The tricky part is in how to get it implemented correctly.
>
> We need to be able to correctly handle DWIM logic for things, and
> ensure that what we're operating on actually looks "note-like" since
> we don't really want to perform read-only ops on refs that don't hold
> notes like objects.

I believe read-only operations on non-notes trees is harmless (although suboptimal). When reading in a notes tree, the notes code maintains non-note entries in a sorted linked list. Only paths that contain exactly 40 hex characters (modulo '/') ends up as "notes" (i.e. false positives). The rest ends up in the non-notes list. The overwhelming majority of non-notes trees will have no "notes" in them (zero false positives).

For those few trees that do contain note-like paths: since we never write out the tree again, we don't end up corrupting the non-notes tree itself (which would typically look like changing the "fanout" of note-like paths, e.g. moving 'de/adbeef...' to 'deadbeef...'). Hence, the only damage we can get from reading in a non-notes tree depend on what we subsequently do with the "notes" information read from that tree.

Again, since the number of "notes" read from a non-notes tree is typically zero, the subsequent damage is typically, also, zero.

For "git notes merge", false positives from a non-notes tree are merged into the first (proper) notes tree.

For "git log --notes", false positives end up being displayed as part of the output. Note that here, a false positive must not only match the above criteria (40 hex chars, modulo '/'), but must also correctly name a commit that occurs in the log.

Are there other cases where a false positive would wreak considerable havoc?

Additionally, if we suspect that passing non-notes trees to read-only operations will be a common error, we could add a simple heuristic to the notes code, to warn (or even abort) if we strongly suspect that we are reading in a non-notes tree. For example, if the ratio of non-notes to notes entries goes above, say, 1:1 (or even 10:1), then what we're reading is probably not a proper notes tree...

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net