Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

From: Jacob Keller <jacob.keller@gmail.com>
Date: Sep 22, 2015, 18:48 UTC
Message-ID: <CA+P7+xqDs4OokEeNhsvYeP8dDRhC1jRh2hLMBqc1v8D+ABCnmg@mail.gmail.com>
In-Reply-To: <xmqqa8se47wa.fsf@gitster.mtv.corp.google.com>

On Tue, Sep 22, 2015 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> Jacob Keller <jacob.keller@gmail.com> 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.

Show 31 quoted lines

>
>> 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.

Show 8 quoted lines

> 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