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

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

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 3644d14..5f276fc 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -114,6 +114,8 @@ static void write_note_data(int fd, const unsigned char *sha1)
 	enum object_type type;
 	char *buf = read_sha1_file(sha1, &type, &size);
 	if (buf) {
+		if (type != OBJ_BLOB)
+			die(_("note %s is not a blob"), sha1_to_hex(sha1));
 		if (size)
 			write_or_die(fd, buf, size);
 		free(buf);
@@ -201,8 +203,11 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 		strbuf_grow(&(msg->buf), size + 1);
 		if (msg->buf.len && prev_buf && size)
 			strbuf_insert(&(msg->buf), 0, "\n", 1);
-		if (prev_buf && size)
+		if (prev_buf && size) {
+			if (type != OBJ_BLOB)
+				die(_("note %s is not a blob"), sha1_to_hex(prev));
 			strbuf_insert(&(msg->buf), 0, prev_buf, size);
+		}
 		free(prev_buf);
 	}
 
@@ -274,6 +279,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 		free(buf);
 		die(_("Failed to read object '%s'."), arg);;
 	}
+	if (type != OBJ_BLOB)
+		die(_("%s is not a blob, invalid for notes"), sha1_to_hex(object));
 	strbuf_add(&(msg->buf), buf, len);
 	free(buf);
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 16de05a..8c72755 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1222,4 +1222,8 @@ test_expect_success 'git notes get-ref (--ref)' '
 	test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = "refs/notes/baz"
 '
 
+test_expect_success 'non-blobs cannot be notes' '
+	test_must_fail git notes add -f -C HEAD^{tree}
+'
+
 test_done
-- 
1.7.8.36.g69ee2

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Nguyen Thai Ngoc Duy]

On Tue, May 8, 2012 at 11:03 PM, Jeff King <peff@peff.net> wrote:
> On Tue, May 08, 2012 at 08:11:32PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> While at it, check if current notes are blobs before using them.
>
> 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?

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

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

[Jeff King]

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

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

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > 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).
> 
> 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.

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

[PATCH 0/4] git-notes ui fixes regarding non-blobs notes

[Nguyễn Thái Ngọc Duy]

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(-)

-- 
1.7.8.36.g69ee2

[PATCH 1/4] notes: preserve object type given by "add -C"

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 3644d14..9840269 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -98,6 +98,7 @@ struct msg_arg {
 	int given;
 	int use_editor;
 	struct strbuf buf;
+	enum object_type type;
 };
 
 static int list_each_note(const unsigned char *object_sha1,
@@ -211,7 +212,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 			sha1_to_hex(object));
 		hashclr(result);
 	} else {
-		if (write_sha1_file(msg->buf.buf, msg->buf.len, blob_type, result)) {
+		if (write_sha1_file(msg->buf.buf, msg->buf.len, typename(msg->type), result)) {
 			error(_("unable to write note object"));
 			if (path)
 				error(_("The note contents has been left in %s"),
@@ -278,6 +279,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 	free(buf);
 
 	msg->given = 1;
+	msg->type = type;
 	return 0;
 }
 
@@ -529,7 +531,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	unsigned char object[20], new_note[20];
 	char logmsg[100];
 	const unsigned char *note;
-	struct msg_arg msg = { 0, 0, STRBUF_INIT };
+	struct msg_arg msg = { 0, 0, STRBUF_INIT, OBJ_BLOB };
 	struct option options[] = {
 		{ OPTION_CALLBACK, 'm', "message", &msg, "msg",
 			"note contents as a string", PARSE_OPT_NONEG,
@@ -686,7 +688,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	const unsigned char *note;
 	char logmsg[100];
 	const char * const *usage;
-	struct msg_arg msg = { 0, 0, STRBUF_INIT };
+	struct msg_arg msg = { 0, 0, STRBUF_INIT, OBJ_BLOB };
 	struct option options[] = {
 		{ OPTION_CALLBACK, 'm', "message", &msg, "msg",
 			"note contents as a string", PARSE_OPT_NONEG,
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 16de05a..d3fd341 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1222,4 +1222,14 @@ test_expect_success 'git notes get-ref (--ref)' '
 	test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = "refs/notes/baz"
 '
 
+test_expect_success 'add -C happily takes object of any kind' '
+	git notes add -f -C HEAD^{tree}
+'
+
+test_expect_success 'non-blobs notes are shown in human-readable form' '
+	git notes show HEAD >actual &&
+	git show `git rev-parse HEAD^{tree}` >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.7.8.36.g69ee2

[PATCH 2/4] notes: "add -c" refuses to open an editor with non-blobs

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 9840269..2960535 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -275,6 +275,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 		free(buf);
 		die(_("Failed to read object '%s'."), arg);;
 	}
+	if (msg->use_editor && type != OBJ_BLOB)
+		die(_("%s is not a blob, cannot be edited manually"), sha1_to_hex(object));
 	strbuf_add(&(msg->buf), buf, len);
 	free(buf);
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index d3fd341..add13bc 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1222,6 +1222,10 @@ test_expect_success 'git notes get-ref (--ref)' '
 	test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = "refs/notes/baz"
 '
 
+test_expect_success 'non-blobs cannot be edited using editor' '
+	EDITOR=cat test_must_fail git notes add -f -c HEAD^{tree}
+'
+
 test_expect_success 'add -C happily takes object of any kind' '
 	git notes add -f -C HEAD^{tree}
 '
-- 
1.7.8.36.g69ee2

[PATCH 3/4] notes: refuse to edit non-blobs

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 2960535..44fb8b6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -109,12 +109,14 @@ static int list_each_note(const unsigned char *object_sha1,
 	return 0;
 }
 
-static void write_note_data(int fd, const unsigned char *sha1)
+static void write_note_blob(int fd, const unsigned char *sha1)
 {
 	unsigned long size;
 	enum object_type type;
 	char *buf = read_sha1_file(sha1, &type, &size);
 	if (buf) {
+		if (type != OBJ_BLOB)
+			die(_("note %s is not a blob"), sha1_to_hex(sha1));
 		if (size)
 			write_or_die(fd, buf, size);
 		free(buf);
@@ -178,7 +180,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 		if (msg->given)
 			write_or_die(fd, msg->buf.buf, msg->buf.len);
 		else if (prev && !append_only)
-			write_note_data(fd, prev);
+			write_note_blob(fd, prev);
 		write_or_die(fd, note_template, strlen(note_template));
 
 		write_commented_object(fd, object);
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index add13bc..9104bf0 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1236,4 +1236,8 @@ test_expect_success 'non-blobs notes are shown in human-readable form' '
 	test_cmp expected actual
 '
 
+test_expect_success 'cannot edit non-blob notes' '
+	EDITOR=cat test_must_fail git notes edit
+'
+
 test_done
-- 
1.7.8.36.g69ee2

[PATCH 4/4] notes: refuse to append to non-blob notes

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 44fb8b6..d958618 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -204,8 +204,11 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 		strbuf_grow(&(msg->buf), size + 1);
 		if (msg->buf.len && prev_buf && size)
 			strbuf_insert(&(msg->buf), 0, "\n", 1);
-		if (prev_buf && size)
+		if (prev_buf && size) {
+			if (type != OBJ_BLOB)
+				die(_("note %s is not a blob"), sha1_to_hex(prev));
 			strbuf_insert(&(msg->buf), 0, prev_buf, size);
+		}
 		free(prev_buf);
 	}
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 9104bf0..0dac1e9 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1240,4 +1240,8 @@ test_expect_success 'cannot edit non-blob notes' '
 	EDITOR=cat test_must_fail git notes edit
 '
 
+test_expect_success 'refuse to launch editor with existing non-blob notes' '
+	EDITOR=cat test_must_fail git notes append -m foo
+'
+
 test_done
-- 
1.7.8.36.g69ee2

Re: [PATCH 0/4] git-notes ui fixes regarding non-blobs notes

[Jeff King]

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

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

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

[PATCH 4/4] notes: only append a blob to a blob

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 44fb8b6..595bcc8 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -162,6 +162,18 @@ static void write_commented_object(int fd, const unsigned char *object)
 		    sha1_to_hex(object));
 }
 
+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);
+	}
+}
+
 static void create_note(const unsigned char *object, struct msg_arg *msg,
 			int append_only, const unsigned char *prev,
 			unsigned char *result)
@@ -204,8 +216,12 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 		strbuf_grow(&(msg->buf), size + 1);
 		if (msg->buf.len && prev_buf && size)
 			strbuf_insert(&(msg->buf), 0, "\n", 1);
-		if (prev_buf && size)
+		if (prev_buf && size) {
+			if (type != OBJ_BLOB || msg->type != OBJ_BLOB)
+				die(_("cannot append %s to %s"),
+				    type_name(type), type_name(msg->type));
 			strbuf_insert(&(msg->buf), 0, prev_buf, size);
+		}
 		free(prev_buf);
 	}
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 9104bf0..9b17e56 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1240,4 +1240,10 @@ test_expect_success 'cannot edit non-blob notes' '
 	EDITOR=cat test_must_fail git notes edit
 '
 
+test_expect_success 'refuse to concatenate two notes of different type' '
+	EDITOR=cat test_must_fail git notes append -m foo &&
+	git notes add -f -m foo &&
+	EDITOR=cat test_must_fail git notes append -C HEAD^{tree}
+'
+
 test_done
-- 
1.7.8.36.g69ee2

Re: [PATCH 4/4] notes: only append a blob to a blob

[Jeff King]

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

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

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.

> +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?

> @@ -204,8 +216,12 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
>  		strbuf_grow(&(msg->buf), size + 1);
>  		if (msg->buf.len && prev_buf && size)
>  			strbuf_insert(&(msg->buf), 0, "\n", 1);
> -		if (prev_buf && size)
> +		if (prev_buf && size) {
> +			if (type != OBJ_BLOB || msg->type != OBJ_BLOB)
> +				die(_("cannot append %s to %s"),
> +				    type_name(type), type_name(msg->type));
>  			strbuf_insert(&(msg->buf), 0, prev_buf, size);
> +		}

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

-Peff

Re: [PATCH 2/4] notes: "add -c" refuses to open an editor with non-blobs

[Johannes Sixt]

Am 5/10/2012 16:04, schrieb Nguyễn Thái Ngọc Duy:
> +test_expect_success 'non-blobs cannot be edited using editor' '
> +	EDITOR=cat test_must_fail git notes add -f -c HEAD^{tree}

	(
		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

Re: [PATCH 4/4] notes: only append a blob to a blob

[Nguyen Thai Ngoc Duy]

On Thu, May 10, 2012 at 10:19 PM, Jeff King <peff@peff.net> wrote:
> On Thu, May 10, 2012 at 09:43:35PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>>  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".
>
> 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.

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

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

>> @@ -204,8 +216,12 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
>>               strbuf_grow(&(msg->buf), size + 1);
>>               if (msg->buf.len && prev_buf && size)
>>                       strbuf_insert(&(msg->buf), 0, "\n", 1);
>> -             if (prev_buf && size)
>> +             if (prev_buf && size) {
>> +                     if (type != OBJ_BLOB || msg->type != OBJ_BLOB)
>> +                             die(_("cannot append %s to %s"),
>> +                                 type_name(type), type_name(msg->type));
>>                       strbuf_insert(&(msg->buf), 0, prev_buf, size);
>> +             }
>
> I think this is wrong for the reasons above. You would not want to
> concatenate a tree to a tree.

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

Re: [PATCH 4/4] notes: only append a blob to a blob

[Jeff King]

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.

> >> @@ -204,8 +216,12 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
> >>               strbuf_grow(&(msg->buf), size + 1);
> >>               if (msg->buf.len && prev_buf && size)
> >>                       strbuf_insert(&(msg->buf), 0, "\n", 1);
> >> -             if (prev_buf && size)
> >> +             if (prev_buf && size) {
> >> +                     if (type != OBJ_BLOB || msg->type != OBJ_BLOB)
> >> +                             die(_("cannot append %s to %s"),
> >> +                                 type_name(type), type_name(msg->type));
> >>                       strbuf_insert(&(msg->buf), 0, prev_buf, size);
> >> +             }
> >
> > I think this is wrong for the reasons above. You would not want to
> > concatenate a tree to a tree.
> 
> Hmm.. that would become "if (tree != blob || tree != blob) die();",
> exactly your point.

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

Re: [PATCH 2/4] notes: "add -c" refuses to open an editor with non-blobs

[Nguyen Thai Ngoc Duy]

On Thu, May 10, 2012 at 10:26 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 5/10/2012 16:04, schrieb Nguyễn Thái Ngọc Duy:
>> +test_expect_success 'non-blobs cannot be edited using editor' '
>> +     EDITOR=cat test_must_fail git notes add -f -c HEAD^{tree}
>
>        (
>                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.)

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

>
>> +'
>
> -- Hannes



-- 
Duy

[PATCH v2 0/4] non-blob notes fixes

[Nguyễn Thái Ngọc Duy]

 - 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(-)

-- 
1.7.3.1.256.g2539c.dirty

[PATCH v2 1/4] notes: preserve object type given by "add -C"

[Nguyễn Thái Ngọc Duy]

_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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 3644d14..9840269 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -98,6 +98,7 @@ struct msg_arg {
 	int given;
 	int use_editor;
 	struct strbuf buf;
+	enum object_type type;
 };
 
 static int list_each_note(const unsigned char *object_sha1,
@@ -211,7 +212,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 			sha1_to_hex(object));
 		hashclr(result);
 	} else {
-		if (write_sha1_file(msg->buf.buf, msg->buf.len, blob_type, result)) {
+		if (write_sha1_file(msg->buf.buf, msg->buf.len, typename(msg->type), result)) {
 			error(_("unable to write note object"));
 			if (path)
 				error(_("The note contents has been left in %s"),
@@ -278,6 +279,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 	free(buf);
 
 	msg->given = 1;
+	msg->type = type;
 	return 0;
 }
 
@@ -529,7 +531,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	unsigned char object[20], new_note[20];
 	char logmsg[100];
 	const unsigned char *note;
-	struct msg_arg msg = { 0, 0, STRBUF_INIT };
+	struct msg_arg msg = { 0, 0, STRBUF_INIT, OBJ_BLOB };
 	struct option options[] = {
 		{ OPTION_CALLBACK, 'm', "message", &msg, "msg",
 			"note contents as a string", PARSE_OPT_NONEG,
@@ -686,7 +688,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	const unsigned char *note;
 	char logmsg[100];
 	const char * const *usage;
-	struct msg_arg msg = { 0, 0, STRBUF_INIT };
+	struct msg_arg msg = { 0, 0, STRBUF_INIT, OBJ_BLOB };
 	struct option options[] = {
 		{ OPTION_CALLBACK, 'm', "message", &msg, "msg",
 			"note contents as a string", PARSE_OPT_NONEG,
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 16de05a..d3fd341 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1222,4 +1222,14 @@ test_expect_success 'git notes get-ref (--ref)' '
 	test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = "refs/notes/baz"
 '
 
+test_expect_success 'add -C happily takes object of any kind' '
+	git notes add -f -C HEAD^{tree}
+'
+
+test_expect_success 'non-blobs notes are shown in human-readable form' '
+	git notes show HEAD >actual &&
+	git show `git rev-parse HEAD^{tree}` >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.7.3.1.256.g2539c.dirty

[PATCH v2 2/4] notes: "add -c" refuses to open an editor with non-blobs

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 9840269..2960535 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -275,6 +275,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 		free(buf);
 		die(_("Failed to read object '%s'."), arg);;
 	}
+	if (msg->use_editor && type != OBJ_BLOB)
+		die(_("%s is not a blob, cannot be edited manually"), sha1_to_hex(object));
 	strbuf_add(&(msg->buf), buf, len);
 	free(buf);
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index d3fd341..dd8675f 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1222,6 +1222,10 @@ test_expect_success 'git notes get-ref (--ref)' '
 	test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = "refs/notes/baz"
 '
 
+test_expect_success 'non-blobs cannot be edited using editor' '
+	test_must_fail git notes add -f -c HEAD^{tree}
+'
+
 test_expect_success 'add -C happily takes object of any kind' '
 	git notes add -f -C HEAD^{tree}
 '
-- 
1.7.3.1.256.g2539c.dirty

[PATCH v2 3/4] notes: refuse to edit non-blobs

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 2960535..44fb8b6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -109,12 +109,14 @@ static int list_each_note(const unsigned char *object_sha1,
 	return 0;
 }
 
-static void write_note_data(int fd, const unsigned char *sha1)
+static void write_note_blob(int fd, const unsigned char *sha1)
 {
 	unsigned long size;
 	enum object_type type;
 	char *buf = read_sha1_file(sha1, &type, &size);
 	if (buf) {
+		if (type != OBJ_BLOB)
+			die(_("note %s is not a blob"), sha1_to_hex(sha1));
 		if (size)
 			write_or_die(fd, buf, size);
 		free(buf);
@@ -178,7 +180,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 		if (msg->given)
 			write_or_die(fd, msg->buf.buf, msg->buf.len);
 		else if (prev && !append_only)
-			write_note_data(fd, prev);
+			write_note_blob(fd, prev);
 		write_or_die(fd, note_template, strlen(note_template));
 
 		write_commented_object(fd, object);
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index dd8675f..66cc872 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1236,4 +1236,8 @@ test_expect_success 'non-blobs notes are shown in human-readable form' '
 	test_cmp expected actual
 '
 
+test_expect_success 'cannot edit non-blob notes' '
+	test_must_fail git notes edit
+'
+
 test_done
-- 
1.7.3.1.256.g2539c.dirty

[PATCH v2 4/4] notes: only allow to append a blob to a blob

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 44fb8b6..a90d9b6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -204,8 +204,13 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 		strbuf_grow(&(msg->buf), size + 1);
 		if (msg->buf.len && prev_buf && size)
 			strbuf_insert(&(msg->buf), 0, "\n", 1);
-		if (prev_buf && size)
+		if (prev_buf && size) {
+			if (type != OBJ_BLOB)
+				die(_("cannot append to a non-blob note"));
+			if (msg->type != OBJ_BLOB)
+				die(_("cannot append non-blob object to a note"));
 			strbuf_insert(&(msg->buf), 0, prev_buf, size);
+		}
 		free(prev_buf);
 	}
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 66cc872..7a682fe 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1240,4 +1240,10 @@ test_expect_success 'cannot edit non-blob notes' '
 	test_must_fail git notes edit
 '
 
+test_expect_success 'refuse to concatenate two notes of different type' '
+	test_must_fail git notes append -m foo &&
+	git notes add -f -m foo &&
+	test_must_fail git notes append -C HEAD^{tree}
+'
+
 test_done
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH 4/4] notes: only append a blob to a blob

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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"));

It certainly looks easier to grok.

Re: [PATCH v2 1/4] notes: preserve object type given by "add -C"

[Junio C Hamano]

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

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

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?

Re: [PATCH v2 1/4] notes: preserve object type given by "add -C"

[Nguyen Thai Ngoc Duy]

On Sat, May 12, 2012 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> _Automatically_ converting a non-blob object to a blob is
>> wrong. Either this way, or reject non-blob objects upfront.
>
> But wouldn't it be even _more_ wrong to stuff a non-blob object at the
> leaf level of the notes tree?

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.

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

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

Re: [PATCH v2 1/4] notes: preserve object type given by "add -C"

[Junio C Hamano]

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

> On Sat, May 12, 2012 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> 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?
>
> 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".

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.

Re: [PATCH v2 1/4] notes: preserve object type given by "add -C"

[Nguyen Thai Ngoc Duy]

On Sat, May 12, 2012 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Sat, May 12, 2012 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> 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?
>>
>> 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".
>
> 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.

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

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



-- 
Duy