Re: [PATCH] Remove restriction on notes ref base

[Johan Herland <johan@herland.net>]

On Tuesday 02 November 2010, Shawn Pearce wrote:
> On Tue, Nov 2, 2010 at 1:48 AM, Johan Herland wrote:
> > 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?
>
> I wanted Kenny to create a notes branch called refs/meta/bad-commits,
> and put that inside of one of our Gerrit Code Review server
> repositories.  We want to extend Gerrit Code Review to check to see
> if any commit in the incoming pack appears in the bad-commits with a
> note.  If it does, it will reject the push.  This allows a repository
> owner to ban certain commits from re-entering a repository once they
> have done a filter-branch or rebase to rewrite a particular item out
> of history.

Ah. Nice feature. Yet another use case for 'git notes' to add to my 
list... :)

> I didn't want to use refs/notes/bad-commits because its not really an
> annotation you would be looking at with git log. But we do want to
> have a log of who banned particular SHA-1s from entering the
> repository, and being able to push that branch from a workstation to
> the server is a convenient way to edit that list of banned SHA-1s.
> During prototyping Kenny discovered you can't use `git note --ref
> refs/meta/bad-commits`.  Which means a server administrator wouldn't
> be able to edit the list directly in the repository.  Hence this
> patch.

Ok, so you're using notes to annotate commits that should NOT exist in 
your repo. In other words (from the code's POV) you are annotating SHA1 
sums that (hopefully) don't exist as objects in your repo.

The notes code will happily do this (and I don't plan on changing this 
behaviour), but you should still know that this is more a side-effect 
of the current implementation than part of the design specification. 
Other notes implementations (jgit?) will need to keep this extra 
requirement in mind.

Also, if you run 'git notes prune' on your notes ref, it will (as 
specified) happily purge all notes to non-existing objects.

> >> 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.
>
> I guess I can see some logic in this.  But the documentation says
> --ref accepts a qualified ref name, and then proceeds to assume that
> a string that starts with "refs/" (like "refs/meta/bad-commits")
> should actually be "refs/notes/refs/meta/bad-commits".  That's a bug
> in the UI and/or documentation.  If we are given a string starting
> with "refs/" and its documented as taking a qualified name, that's
> qualified and should either be rejected, or should be taken as-is.
>
> I think the docs are correct, and the code is buggy.  If the user
> asked us to edit refs/meta/bad-commits, we should.  If the user asked
> us to edit refs/heads/my-branch... well, they asked us to edit it.
>
> :-)

Ok, we've probably been too conservative by trying to restrict where 
users put notes. As I said in the earlier email, we need to loosen this 
up in any case, in order to share notes with remote repos.

> A better safety measure might be to sniff the ref's contents and see
> what it is.  If the top level directory has a number of non-note like
> entries, we should abort editing the branch.  Its not common for
> users to name their directories "02" and "fe".

Good idea. We should probably add something like this to the bottom of 
notes.c:init_notes() (UNTESTED - not even compiled):

  unsigned int i, empty_tree = 1;

  for (i = 0; i < 16; i++) {
  	if (t->root[i]) {
  		empty_tree = 0;
  		break;
  	}
  }

  /* If no notes and >0 non-notes, this is probably not a notes tree */
  if (empty_tree && t->first_non_note)
  	die("'%s' doesn't look like a valid notes tree", t->ref);


Have fun! :)

...Johan

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