Jacob Keller <jacob.keller@xxxxxxxxx> writes:
> I never got any better suggestion on how to allow the behavior
> desired, which is to enable merging from a non-notes location, in
> order to provide a standard location for remote notes, ie:
> refs/remote-notes/<remote>/<ref>
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/*.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html