patchnotes: do not accept non-blobs as new notes

While at it, check if current notes are blobs before using them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c  |    9 ++++++++-
 t/t3301-notes.sh |    4 ++++
 2 files changed, 12 insertions(+), 1 deletions(-)

On Tue, May 08, 2012 at 08:11:32PM +0700, Nguyen Thai Ngoc Duy wrote:

Hmm. There has been discussion in the past on whether trees could be stored in notes. Here's one such thread:

  http://thread.gmane.org/gmane.comp.version-control.git/139165

There didn't seem to be any consensus. It might be a useful concept, but it might introduce some complexity. That discussion is two years old now, and notes are even older. So I don't know that there is some pressing use case that is cut off by disallowing non-blob notes.

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?

-Peff

Jeff King <peff@peff.net> writes:

You said everything I wanted to say and asked the right questions I wanted to ask. Thanks.

On Tue, May 8, 2012 at 11:03 PM, Jeff King <peff@peff.net> wrote:

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.

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

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

Jeff King <peff@peff.net> writes:

It is fair to restrict anything that involves an editor or end user interaction with the terminal output for sanity, and raw tree object data is on the other side of the border line that defines what is sane, so I wouldn't have much problem with a new restriction on the command line interface, except for "-C". Advertising that we store arbitrary binary out of an existing blob as-is and then starting to refuse taking it sounds like a non-starter.

On Wed, May 09, 2012 at 10:52:56AM -0700, Junio C Hamano wrote:

Fair enough. I was willing to accept the "-C $tree" case as collateral damage under the assumption that nobody is using it. But you're right, the real issue is restricting the "-c" case, as well as the "show" case when reading notes. The right patch would restrict those and leave "-C" alone.

-Peff

Thanks Jeff and Junio for all the comments. This is basically what you (and I) agree in v1's thread. The first patch is a new one, to avoid git-notes from converting everything to blobs. I don't restrict the "show" case either because git-notes uses "git show", which is capable of displaying all kinds of objects.

Nguyễn Thái Ngọc Duy (4):
  notes: preserve object type given by "add -C"
  notes: "add -c" refuses to open an editor with non-blobs
  notes: refuse to edit non-blobs
  notes: refuse to append to non-blob notes

 builtin/notes.c  |   21 +++++++++++++++------
 t/t3301-notes.sh |   22 ++++++++++++++++++++++
 2 files changed, 37 insertions(+), 6 deletions(-)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c  |    8 +++++---
 t/t3301-notes.sh |   10 ++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c  |    2 ++
 t/t3301-notes.sh |    4 ++++
 2 files changed, 6 insertions(+), 0 deletions(-)

Am 5/10/2012 16:04, schrieb Nguyễn Thái Ngọc Duy:

	(
		test_set_editor cat &&
		test_must_fail git notes add -f -c HEAD^{tree}
	)

Ditto in 3/4 and 4/4.

(The problem is not that test_set_editor must be used, but that VAR=value in front of a shell function invocation does not do what you want it to do unless your shell is buggy.)

-- Hannes

On Thu, May 10, 2012 at 10:26 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:

thanks. I looked again and t3301 already exports GIT_EDITOR, so we can skip setting EDITOR again.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c  |    6 ++++--
 t/t3301-notes.sh |    4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c  |    5 ++++-
 t/t3301-notes.sh |    4 ++++
 2 files changed, 8 insertions(+), 1 deletions(-)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Thu, May 10, 2012 at 9:29 PM, Jeff King <peff@peff.net> wrote:
 > It's a little odd that we rewrite the object in the "-C" case at all; we
 > should never even need to open the object. I guess it is just to make
 > the code paths between "-c" and "-C" simpler.

 Yeah. It made me look again to see if that was true, and I found that
 my last patch was flawed. Reading object content in memory in "add
 -C" is nonsense, not so much in "append -C".

 builtin/notes.c  |   18 +++++++++++++++++-
 t/t3301-notes.sh |    6 ++++++
 2 files changed, 23 insertions(+), 1 deletions(-)

On Thu, May 10, 2012 at 09:43:35PM +0700, Nguyen Thai Ngoc Duy wrote:

Yeah. Although you would not want to "append -C" anything but blobs. While the tree syntax concatenates, I believe the entries are supposed to be in sorted order, no? And you would not want to concatenate commits at all.

Don't we have object.c:typename for this?

I think this is wrong for the reasons above. You would not want to concatenate a tree to a tree.

-Peff

On Thu, May 10, 2012 at 10:19 PM, Jeff King <peff@peff.net> wrote:

Exactly. Even for blobs, there are non-safe cases (e.g. binaries) but that's out of our control (or my attention).

The key difference here is _() with an article. It's i18n friendly. I wanted to make 15 combinations (blob/blob cannot happen) of "cannot append %s to %s", which is best for translators but probably too much for C developers.

Hmm.. that would become "if (tree != blob || tree != blob) die();", exactly your point.

On Thu, May 10, 2012 at 10:31:23PM +0700, Nguyen Thai Ngoc Duy wrote:

I do not pay much attention to the translation details, but I would think that we would keep terms like "tree" and "blob" universal, as they are technical terms. IOW, you would not expect the "blob" in "git cat-file blob $sha1" to be internationalized, and this seems like the same level of technical detail.

Oh, I totally misread this as "type != msg->type" for some reason. Sorry.

I think the behavior is correct, but the message confused me. If I see "cannot append a foo to a bar", it implies to me that it is the combination of the elements that is the limiting factor. But it is not. It is that one (or more) of the elements is not a blob, regardless of what the other element is. So I think this would be more accurate:

  if (type != OBJ_BLOB)
          die(_("cannot append to a non-blob note"));
  if (msg->type != OBJ_BLOB)
          die(_("cannot append a non-blob object to a note"));

-Peff

Jeff King <peff@peff.net> writes:

It certainly looks easier to grok.

On Thu, May 10, 2012 at 09:04:57PM +0700, Nguyen Thai Ngoc Duy wrote:

Nice, this is much better than the previous round.

I was surprised by the "git show" thing you mentioned, because I checked before that "git notes add -C HEAD^{tree}; git notes show" generated garbage. But the problem is that "-C $tree" was more broken than we realized. It actually copied the tree contents into a blob object, giving it a new sha1:

  $ git rev-parse HEAD^{tree}
  f799843170cdddbbdfec446e934bbccf94e0d2a7
  $ git notes add -C HEAD^{tree}
  $ git notes list HEAD
  587521811b7a5b84ed7cb48d11f9321ddcefd337

So it really was totally useless, and nobody could possibly have been using it to store a real tree object, experimental or otherwise. But I still like these patches better than just disallowing it, because they allow people to experiment with the idea.

It's a little odd that we rewrite the object in the "-C" case at all; we should never even need to open the object. I guess it is just to make the code paths between "-c" and "-C" simpler.

-Peff

 - stop one-time env setting on a shell function call
 - reword error messages when appending non-blobs

Nguyễn Thái Ngọc Duy (4):
  notes: preserve object type given by "add -C"
  notes: "add -c" refuses to open an editor with non-blobs
  notes: refuse to edit non-blobs
  notes: only allow to append a blob to a blob

 builtin/notes.c  |   23 +++++++++++++++++------
 t/t3301-notes.sh |   24 ++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 6 deletions(-)

_Automatically_ converting a non-blob object to a blob is wrong. Either this way, or reject non-blob objects upfront.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c  |    8 +++++---
 t/t3301-notes.sh |   10 ++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

But wouldn't it be even _more_ wrong to stuff a non-blob object at the leaf level of the notes tree?

It is not automatically "converting"; as far as the notes subsystem is concerned, the data you throw at it to be associated with an object the note annotates has always been uninterpreted stream of bytes. If an application wants to store the raw representation of a commit object including the log message and its header, it has every right to expect that it can use "git cat-file commit $argument_to_the_C_option" as the source of that uninterpreted stream of bytes, doesn't it?

On Sat, May 12, 2012 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:

If people deliberately want to do that and notes subsystem knows not to mess with non-blob objects, I think it's ok. Though if users want to add some text, but put non-blob by mistake, then it's wrong to accept it.

Some part of git-notes expects this stream of bytes to be textual, human readable. In that case, "git notes add -C commit/tag"'s stuffing a blob with the given commit/tag content to notes tree may make sense. Personally I'd rather stuff the commit/tag object instead so no new blobs are created. The end result is the same: read_sha1_file() always return correct text, so does "git notes show".

The difference after this series is "notes add -C commit; notes append -m foo" now results in an error message while it case goes smoothly without the series. Perhaps we can be more flexible here on appending and allow to append non-tree objects to non-tree objects, resulting in a blob note.

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

No, the end result is definitely not the same.

There are two important characteristics of "uninterpreted byte stream" the above thinking is not taking into consideration:

 (1) we do not interpret what the application stores; and
 (2) the application is *not* limited by our type system.

Suppose the application happens to want to stuff the contents it took from a commit object, and "add -C $objecname" is a convenient way to do so. We have recorded it as "blob" because it is uninterpreted stream of bytes. If you record that as a leaf note in the note tree, does that mean the note tree now suddenly have a submodule? Hell, no.

What if the application wanted to record the contents of a tree object instead? How would that affect the fan-out mechanism the note subsystem uses to hash the 40-hexadecimal object names? After descending the notes tree to consume the object name to reach the leaf node, it still finds even more level hanging below. Not very careful "list all object names that have notes attached in this note tree" implementation may end up descending into the tree object, because of this patch. Another bad implication of such a change is that suddenly we would start including all the subobjects in that tree object when computing the reachability of the notes tree.

The application needs to have a way to tell what is in the data it stores anyway, because it is not necessarily "enhancing git" kind of application that talks about relationships between git objects. And it may do so either by convention (e.g. my "notes/amlog" notes tree only records a single-line text that is a Message-Id header of the original patch e-mail commits came from) or by having its own way to identify the application specific data type (e.g. json, pickle, protobuf, etc.). It is pointless to say "Git object types can be stored natively, but there is only one type of blob so the application needs to find a way to coax the types of data it wants to store itself." It needs to do so anyway, and having native and standardized way only for git object types does not improve the system. It only ties our hands going forward because we need to define what it _means_ to store non-blob types in the notes tree, and support that forever.

So this 1/4 patch is _not_ a bugfix at all. It breaks perfectly good current storage semantics without no good reason.

For that matter, as long as $EDITOR is set to something appropriate for the application specific data, there is no reason to forbid editing, either.

The only sensible safety against "oops, I forgot that this notes tree stores binary gunk" I can think of offhand that won't cripple sensible use case is to sample the data to see if it is binary and ask confirmation, similar to how "less" asks "frotz may be a binary file. See it anyway?", and do so only when we are spewing it to the terminal.

On Sat, May 12, 2012 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:

Hmm.. you are right. Consider this series dropped.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c  |    2 ++
 t/t3301-notes.sh |    4 ++++
 2 files changed, 6 insertions(+), 0 deletions(-)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c  |    6 ++++--
 t/t3301-notes.sh |    4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Thu, May 10, 2012 at 10:45 PM, Jeff King <peff@peff.net> wrote:
 > On Thu, May 10, 2012 at 10:31:23PM +0700, Nguyen Thai Ngoc Duy wrote:
 >
 >> >> +static const char *type_name(enum object_type type)
 >> >> +{
 >> >> +     switch (type) {
 >> >> +     case OBJ_BLOB: return _("a blob");
 >> >> +     case OBJ_TAG: return _("a tag");
 >> >> +     case OBJ_COMMIT: return _("a commit");
 >> >> +     case OBJ_TREE: return _("a tree");
 >> >> +     default:
 >> >> +             die("BUG: put a new string for type %d here", type);
 >> >> +     }
 >> >> +}
 >> >
 >> > Don't we have object.c:typename for this
 >>
 >> The key difference here is _() with an article. It's i18n friendly. I
 >> wanted to make 15 combinations (blob/blob cannot happen) of "cannot
 >> append %s to %s", which is best for translators but probably too much
 >> for C developers.
 >
 > I do not pay much attention to the translation details, but I would
 > think that we would keep terms like "tree" and "blob" universal, as they
 > are technical terms. IOW, you would not expect the "blob" in "git
 > cat-file blob $sha1" to be internationalized, and this seems like the
 > same level of technical detail.

 It's the article that's important here. Putting "appending a %s to a
 %s" may be safe in this case because no object type begins with a
 vowel (and we would need to turn "a" to "an"). But it raises a
 question: does any other language have different forms of article
 depending on the main noun?

 But this is for the sake of discussion. The new patch (with your
 suggestion) does not have this problem any more.

 builtin/notes.c  |    7 ++++++-
 t/t3301-notes.sh |    6 ++++++
 2 files changed, 12 insertions(+), 1 deletions(-)