On Wed, Nov 18, 2015 at 2:29 PM, Johan Herland <johan@xxxxxxxxxxx> wrote: > 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. > I agree, and I had a patch to change this behavior, but the main issue being I think it didn't fallback to the suggested proposal below. >> 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...) > The question is whether we do: a) check for refs/abc/xyz and fail if we can't find it or b) check for refs/abc/xyz and then expand to refs/notes/refs/abc/xyz if we can't? I think that the first is generally preferable but with read-only ops it's not a big deal since we won't be writing to notes trees. > 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). > > The biggest implementation issue here is that the notes code that does DWIM happens before lookup of whether the ref exists, and the code that does lookup of refs in the notes.c code won't fallback and try a different expansion. I think that is why my proposed change was dropped, if I remember correctly. I am in agreement with you, and think we should use the proposed behavior above, as it is very unlikely to cause any issues with current cases, especially since we already try not to allow operation on refs outside of the notes tree today. Regards, Jake > ...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