Re: [PATCH] notes: do not accept non-blobs as new notes

From: Jeff King <peff@peff.net>
Date: May 9, 2012, 17:37 UTC
Message-ID: <20120509173701.GB30487@sigill.intra.peff.net>
In-Reply-To: <CACsJy8DHotHhF0ADDwjUZx5m8SGjXFuXV9fY=mfHmswyZxzeSQ@mail.gmail.com>

On Wed, May 09, 2012 at 03:19:14PM +0700, Nguyen Thai Ngoc Duy wrote:

Show 13 quoted lines

> > At the same time, is there any reason not to allow experimentation in
> > this area? We don't know what other people might be putting in their
> > private notes trees, and the current interface does allow this.
> >
> > Is this fixing some important problem that justifies making such
> > experimentation harder?
> 
> I was actually thinking about tree notes when I made this patch. I
> decided that if new git supports tree notes while old git does not,
> the old git should refuse to operate on tree notes so it won't cause
> any unintentional damages (e.g. appending a blob note to a tree note).
> It's too late to fix released git though, I don't know what to do in
> that case.

Hmm. I was thinking that we supported arbitrary sha1s as note values via the command-line interface. But we don't; that is only the internal C API, and it is quite difficult to do anything useful with non-blob notes via the command-line interface. As you noticed, you can trick it into doing so with "-C", but even "-c" has disastrous results (unless you like dumping the binary tree object into your editor).

So the support from the command-line tool is pretty awful. And your patches affected only that, not the internal API, which I find a more likely candidate for people experimenting. So I take back my original questions; I think using the command-line tool as-is on non-blob notes is just crazy, and we are better to rigorously enforce that instead of dumping binary junk on the user's terminal.

The slim possibility that somebody is using "git notes add -C" in conjunction with parsing the output of "git notes list" to store trees is probably not worth worrying about (it took me a long time to even figure out if you _could_ have a usable workflow).

-Peff