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

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