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

[Junio C Hamano <gitster@pobox.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.

> 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.