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

On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller [off-list ref] wrote:

The main other issue is how to get notes DWIM things to work for all
cases where we want to use notes refs, since right now the DWIM is
basically done at the top level and only handles notes like things.
The problem with it is that if you specify a full ref that *isn't*
refs/notes, you will always prefix it with refs/notes, like so:

refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,

I am becoming convinced that this is a bug. I don't know anywhere else
in Git, where a fully qualified ref name (i.e. anything starting with
"refs/") is not interpreted verbatim. For the notes code to do just
that adds unnecessary confusion.

This makes it really difficult to expand a ref. However, Junio seemed
to think this was a possibly valuable expansion under normal
circumstances.

I doubt it. It carries its own set of problems in that refs/foo/bar
(=> refs/notes/refs/foo/bar) is treated differently from refs/notes/bar
(=> refs/notes/bar). If users _really_ want to create
refs/notes/refs/$whatever, they should have to be explicit about that
(i.e. we should require them to say refs/notes/refs/$whatever instead
of allowing them to lazily say refs/$whatever). (It even saves them
from a potential bug if their $whatever happens to start with "notes/",
in which case the current code already forces them to fully qualify...)

I realize this is a backwards-incompatible change in behavior, but I
don't think it'll matter much in practice. Given e.g.

  git notes --ref refs/foo list

when refs/foo and refs/notes/refs/foo is both missing:
  Current behavior: refs/notes/refs/foo lookup fails.
    Treat like empty notes tree; no output, exit code 0
  Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
    lookup fails. Same behavior as current.

when refs/notes/refs/foo exists:
  Current behavior: refs/notes/refs/foo lookup succeeds.
    Shows notes in that tree
  Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
    lookup succeeds. Same as current.

when refs/foo exists:
  Current behavior: refs/notes/refs/foo lookup fails. Treat like empty
    notes tree; no output, exit code 0
  Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
    probably empty, hence no output, exit code 0

when both refs/foo and refs/notes/refs/foo exist:
  Current behavior: refs/notes/refs/foo lookup succeeds. Shows notes
    in that tree
  Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
    probably empty, hence no output, exit code 0

In other words, this change requires both refs/foo and
refs/notes/refs/foo to be present in order to cause any real confusion.
And in that case, the proposed behavior forces you to use fully-
qualified refs (which will be interpreted as such) whereas the current
behavior takes what looks like a fully-qualified ref (refs/foo) and
interprets it like a notes-shorthand (-> refs/notes/refs/foo), which
I argue is probably more confusing to most users.

The current solution is to try to do a normal lookup
first and only use the notes DWIM after we fail a lookup, which I
think is what the above patch attempts to do. This seems ok enough to
me.

Yes, given $whatever, we should first lookup $whatever, and only
failing that, we should try refs/notes/$whatever. Maybe it's also
worth trying refs/$whatever (before refs/notes/$whatever), since that
would be consistent with what's currently done for other refs (e.g.
try "git log heads/master" or "git log tags/v2.6.3" in git.git).


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net