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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2015, 18:37 UTC
Message-ID: <xmqqa8se47wa.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CA+P7+xrwM44tMHMW+dEJx_tMtcMVEQELNq=J=rqs1uQ+feuZqQ@mail.gmail.com>

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

> The other issue here is that expand_notes_ref is called on the --ref
> argument waaay before the current code even decides if the operation
> is "read" or "write". Thus we'd have to break this out and handle
> things very differently.

I think you hit the right nail here. The handling of --ref argument is what you need to adjust to the new world order.

And "safety" is a red herring.  Those who are used to run

	git log --notes=refs/heads/master

and relies on it to refer to refs/notes/refs/heads/master must continue to be able to do so. Changing expand_notes_ref() the way the proposed patch does will break them, won't it? "safety" is not the only (or even the primary) requirement, but the correctness must also be kept.

Show 9 quoted lines

> It seems like a lot more heavy lifting to change the entire flow of
> how we decide when to expand "--ref" for "read" operations vs "write"
> operations.
>
> That is, git notes list.
>
> It's easy to change behavior of git notes merge as it handles its
> argument for where to merge from separately, but it's not so easy to
> change git notes show...

Yes, exactly.

I am _more than_ OK to see that read-only accesses to the notes tree allowed anything under refs/ (like the proposed patch did) and also a raw 40-hex object name in the endgame, but I really would not want to see "we meant well and attempted to enhance 'notes merge' but we ended up regressing the behaviour of unrelated operations all of a sudden".

If you cannot do your change without breaking the existing users, then you at least need a sound transition plan. But I am not sure yet if you have to break the world (yet). Let me think aloud a bit more.

There aren't all that many callers of expand_notes_ref().

My preference is to leave that function as-is, especially keep the behaviour of the caller in revision.c that handles --show-notes= (and --notes= that is its synonym) the same as before.

All the other callers are only reachable from the codepath that originates from cmd_notes(), if I am reading the code correctly, and that can be enhanced without breaking the existing users. One obvious way to do so would be to make "--ref" to keep the call to expand_notes_ref(), and add another "--notes-rawref" or whatever option that does not restrict it to "refs/notes", but there may be other ways.