On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller <jacob.keller@xxxxxxxxx> 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, <johan@xxxxxxxxxxx>
www.herland.net
--
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