Re: [PATCH] notes: accept any ref for merge

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2014, 18:22 UTC
Message-ID: <xmqqoaubmpvh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CALKQrgc4nZdaXM-Ooh1pP4x4nZRLexJzLyaBmrgn+qVaQGCg+g@mail.gmail.com>

Johan Herland <johan@herland.net> writes:

Show 24 quoted lines

> On Fri, Sep 19, 2014 at 11:39 AM, Jeff King <peff@peff.net> wrote:
>> On Fri, Sep 19, 2014 at 09:39:45AM +0200, Scott Chacon wrote:
>>> Currently if you try to merge notes, the notes code ensures that the
>>> reference is under the 'refs/notes' namespace. In order to do any sort
>>> of collaborative workflow, this doesn't work well as you can't easily
>>> have local notes refs seperate from remote notes refs.
>>>
>>> This patch changes the expand_notes_ref function to check for simply a
>>> leading refs/ instead of refs/notes to check if we're being passed an
>>> expanded notes reference. This would allow us to set up
>>> refs/remotes-notes or otherwise keep mergeable notes references outside
>>> of what would be contained in the notes push refspec.
>>
>> I think this change affects not just "git notes merge", but all of the
>> notes lookups (including just "git notes show")....
> ...
> Additionally, I suggest adding another test demonstrating your use
> case as well. Something like setting up a small scenario for notes
> collaboration, and walking through the various steps:
>
>  - Creating a couple of repos where notes are added/edited
>  - Setting up config to allow pushing and/or fetching notes
>  - Performing the push/fetch
>  - Merging with the corresponding local notes ref

Is it our future direction to set up refs/remote-notes/<remote>/ namespace? If so, let's not do it piecemeail in an unorganized guerrilla fashion by starting with a stealth enabler with an associated test. We risk not following through and leave the resulting user experience more puzzling if we go that way.

By "stealth enabler" I mean the removal of refs/notes/ restriction that was originally done as a safety measure to avoid mistakes of storing notes outside. The refs/remote-notes/ future direction declares that it is no longer a mistake to store notes outside refs/notes/, but that does not necessarily have to mean that anywhere under refs/ is fine. It may make more sense to be explicit with the code touched here to allow traditional refs/notes/ and the new hierarchy only. That way, we will still keep the "avoid mistakes" safety and enable the new layout at the same time.

The most important first step for that to happen is to make sure we are on the same page on that future direction. I personally think refs/remote-notes/<remote> that runs parallel to the remote tracking branch hierarchy refs/remotes/<remote> is a reasonable way to do this, but my words are no way final.

Assuming that this is we all agree to go in that direction, let's make a list of things to be done to codify it, and do them. For a starter, I think these are needed, perhaps?

 - This patch (or an enhancement to keep some safety)

 - Documentation updates to "git notes"

 - Documentation updates to Documentation/gitrepository-layout.txt

 - Update to "git clone" and "git remote add" to add a fetch refspec
   refs/notes:refs/remote-refs/<remote>/*

 - New tests you suggest