Re: [PATCH] Remove restriction on notes ref base

From: Johan Herland <johan@herland.net>
Date: Nov 2, 2010, 08:48 UTC
Message-ID: <201011020948.22677.johan@herland.net>
In-Reply-To: <20101102065208.GA4280@burratino>

On Tuesday 02 November 2010, Jonathan Nieder wrote:

Show 8 quoted lines

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

Show 17 quoted lines

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