Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

Jacob Keller <jacob.keller@gmail.com> writes:

Step back a bit and think again. I think you are blinded by your refs/remote-notes/*.

It is fine to wish that

    $ notes merge refs/notes/commits refs/remote-notes/origin/commits

to work, but you shouldn't force your users to use remote-tracking refs in the first place. Your users should be allowed to do this as well:

    $ fetch origin refs/notes/commits
    $ THEIRS=$(git rev-parse FETCH_HEAD)
    $ notes merge refs/notes/commits $THEIRS

We need to realize that "notes merge" involves two notes trees and they are of different nature. The notes tree you are merging into and recording the result (the destination), which will be a local note, and the other notes tree you obtained from elsewhere and update that local note with (the source).

The current code before your patch limits the allowed pair of notes trees by insisting that both appear as the tips of refs somewhere in refs/notes/*. For allowing to merge from outside refs/notes/, you need to loosen the location the latter notes tree, the one to update your local notes tree with, can come from. But that does not mean you would want to loosen the location where the resulting notes tree can go.

I think the proposed patch conflates them, and that conflation does not help anything. The rule of that function used to be "It must come from refs/notes/ and nowhere else". That made sense in the old world order where both must be from refs/notes/, and the rule still makes sense in the new world order for the destination of the merge.

The new rule of that function after the proposed patch says "It must come from either refs/notes or refs/ somewhere". This does not make sense for the destination because it is too loose (and we didn't see any justification why loosening it is a good idea), and it does not make sense for the source because it still is too tight. It should be able to take anything get_sha1() understands (including $THEIRS in the above example).

In addition you might also want to allow additional DWIMs from X to refs/remote-notes/*/X as well as from X to refs/notes/X, but that is secondary and should be done as a follow-up "nice to have" change, because both "notes/remote-notes/origin/commits" and "notes/commits" would be understood by get_sha1() already, and it is questinable if it is a good idea to introduce special DWIMs that kick in only when the commands are talking about notes in the first place (an equally questionable alternative is to teach get_sha1() about refs/notes/* and refs/remote-notes/*/*, which will make the disambiguation noisy in the normal non-notes codepath---my knee-jerk reaction is to suggest not to go there, either).

In any case, to get us closer to that endgame, change in the proposed patch does not help. It tries to cover two different cases with a logic that is not a good match to either. You need to have two separate helpers to interpret the source and the destination.

Calls expand_notes_ref() made on a command line argument that specifies the source (which I think is similar to what the other recent topic calls "read-only") should be made to calls to a more lenient version (and you can start with get_sha1() for that purpose without introducing your own DWIMs in the first step), while leaving calls to expand_notes_ref() for destination and the implementation of expand_notes_ref() itself unmolested, so that we can keep the safety in expands_notes_ref() that makes sure that the destination of a local operation is under refs/notes/*.