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

[Jeff King <peff@peff.net>]

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"). However, I'd argue
that's a good thing, as it allows more flexibility in note storage. The
downside is that if you have a notes ref like
"refs/notes/refs/heads/master", you can no longer refer to it as
"refs/heads/master" (you have to use the fully qualified name to get the
note). But:

  1. This makes the notes resolution a lot more like regular ref
     resolution (i.e., we now allow fully qualified refs, and you can
     store remote notes outside of refs/notes if you want to).

  2. There are already a bunch of names that have the same problem. You
     cannot refer to "refs/notes/notes/foo" as "notes/foo", nor
     "refs/notes/refs/notes/foo" as "refs/notes/foo". Yes, these are
     silly names, so is the example above.

So it's backwards incompatible with the current behavior, but I think in
a good way.

> ---
>  notes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I think you need to adjust t3308 (and you should probably add a new test
exercising your case; this is exactly the sort of thing that it's easy
to accidentally regress later).

-Peff