Re: [PATCH] Remove restriction on notes ref base

[Johan Herland <johan@herland.net>]

On Tuesday 02 November 2010, Jonathan Nieder wrote:
> (+cc: Johan, Thomas)
> 
> Kenny Root wrote:
> > Git notes were restricted to refs/notes/* in the command line
> > utilities, but setting things like GIT_NOTES_REF to something outside
> > of that structure would work.
> > 
> > This removes the restrictions on the git notes command line interface.

Why do you need to set GIT_NOTES_REF to something outside refs/notes/ at 
all?

> Could you explain what those restrictions are (perhaps through an
> example in the form of a patch to the t/ subdirectory)?

Yes, please do. As it stands, I'm pretty sure the patch below breaks at 
least TCs t3301.4 and t3301.5.

> Cc-ing some people more knowledgeable about notes than I am; maybe
> they can give more information about what this notes.rewriteref
> protection and other check are about.

Well, I guess we originally decided to limit notes refs to within 
refs/notes/ in order to clearly separate notes from non-notes, and to 
prevent notes code from accidentally messing up non-notes refs.

Later, we have discovered that in order to work with remote notes refs, we 
probably have to lift this restriction, and allow (at least) notes refs 
somewhere within refs/remotes/, e.g. refs/remotes/*/notes/.

http://thread.gmane.org/gmane.comp.version-control.git/159469/focus=159703 
and its sub-thread contains more discussion on sharing notes with remote 
repos.

So, in conclusion, this patch is a move in the right direction, but in some 
respects it might be going a bit _too_ far: Maybe we should restrict notes 
to refs/notes/ and refs/remotes/*/notes/? ...or maybe I'm too paranoid, and 
allowing notes "everywhere" is ok. Then again, in other respects this patch 
isn't going far enough, since it doesn't deal with setting up the refspecs 
needed to make notes sharing easy and straighforward for end users.

> [patch follows for their convenience.]
> [...]
> > @@ -844,7 +837,7 @@ int cmd_notes(int argc, const char **argv, const
> > char *prefix)
> > 
> >  	if (override_notes_ref) {
> >  	
> >  		struct strbuf sb = STRBUF_INIT;
> > 
> > -		if (!prefixcmp(override_notes_ref, "refs/notes/"))
> > +		if (!prefixcmp(override_notes_ref, "refs/"))
> > 
> >  			/* we're happy */;
> >  		
> >  		else if (!prefixcmp(override_notes_ref, "notes/"))
> >  		
> >  			strbuf_addstr(&sb, "refs/");

FTR, this hunk will surely conflict with the introduction of 
expand_notes_ref() which is in patch 09/23 in the 'git notes merge' series 
in 'pu'.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net