On Tue, Sep 22, 2015 at 11:37 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Jacob Keller <jacob.keller@xxxxxxxxx> 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. I think that's more confusing than helpful, but we really shouldn't change behavior here. I think we need to update documentation to clearly indicate the current behavior, as it was not obvious at least to me. I can submit a patch for this at least. > >> 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. > Ok. > 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. > I think the easiest way would be to have --ref check ahead of time which commands are read or write, and perform the init_notes_check code there instead of inside each command. I'll look at this again when I have time in the next few days. Regards, Jake -- 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