Re: [PATCH] Remove restriction on notes ref base

From: Shawn Pearce <spearce@spearce.org>
Date: Nov 2, 2010, 14:11 UTC
Message-ID: <AANLkTinN1UXSmkxOg59pT_xVd2eWS0Ms2sgAweLv7hbg@mail.gmail.com>
In-Reply-To: <201011020948.22677.johan@herland.net>

On Tue, Nov 2, 2010 at 1:48 AM, Johan Herland <johan@herland.net> wrote:

Show 12 quoted lines

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

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.

Show 7 quoted lines

>> 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. :-)

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".

-- 
Shawn.