Commit notes workflow

We have notes merge support since a couple of releases now, but no real example
in the docs of how best to use that.  That is, no suggested mapping of remote notes,
let alone automatic setup of refspecs at clone time.

Trying to setup such refspecs, I find myself puzzled:

* if I store remote notes under refs/notes (eg. refs/notes/*:refs/notes/origin/* as fetch
  refspec), then a refs/notes/*:refs/notes/origin/* push refspec will include
  refs/notes/origin/*, which we obviously don't want

* if I store them outside of refs/notes (eg. refs/notes/*:refs/remote-notes/origin/* ),
  then "git notes" silently ignores them: no output nor any error message from "notes list"
  or "notes merge".

Do we really want to "git notes" to ignore everything not in refs/notes/ ?  I can think of
2 possibilities out of this situation:

* remove that limitation
* decide on a naming convention for remote notes, and teach "git notes" not to ignore it

A (minor) problem with the second possibility is that this naming convention could evolve,
eg. if we end up with something like was proposed in [1] for 1.8.0.  Is there any real drawback
with the first suggestion ?

[1] http://marc.info/?l=git&m=129661334011986&w=4
-- 
Yann Dirson - Bertin Technologies

On Monday 13 June 2011, Yann Dirson wrote:

We have notes merge support since a couple of releases now, but no real
example in the docs of how best to use that.  That is, no suggested
mapping of remote notes, let alone automatic setup of refspecs at clone
time.

True. I think this has been held up, partly because I (or anyone else) 
haven't found the time to work on this, and partly because we want to add 
some kind of default refspec to easily share notes between repos; the latter 
has been caught up in the discussion you refer to in [1].

Trying to setup such refspecs, I find myself puzzled:

* if I store remote notes under refs/notes (eg.
refs/notes/*:refs/notes/origin/* as fetch refspec), then a
refs/notes/*:refs/notes/origin/* push refspec will include
refs/notes/origin/*, which we obviously don't want

* if I store them outside of refs/notes (eg.
refs/notes/*:refs/remote-notes/origin/* ), then "git notes" silently
ignores them: no output nor any error message from "notes list" or
"notes merge".

Do we really want to "git notes" to ignore everything not in refs/notes/
?  I can think of 2 possibilities out of this situation:

* remove that limitation
* decide on a naming convention for remote notes, and teach "git notes"
not to ignore it

The naming convention I have proposed (in the discussion for [1]) is 

  refs/notes/*:refs/remotes/$remote/notes/*

(but it obviously depends on reorganizing the entire remote refs hierarchy)

A (minor) problem with the second possibility is that this naming
convention could evolve, eg. if we end up with something like was
proposed in [1] for 1.8.0.  Is there any real drawback with the first
suggestion ?

[1] http://marc.info/?l=git&m=129661334011986&w=4

My gut feeling is to keep some sort of limit notes refs, and if/when we get 
around to implementing my proposal in [1] (or some variation thereof), we 
will of course extend the limit to put "refs/remotes/$remote/notes/*" (or 
whatever is decided) in the same category as "refs/notes/*".

In the meantime, I'm unsure if it's a good idea to remove the limitation 
altogether (allowing notes refs everywhere), since re-introducing a limit at 
a later point will then be MUCH harder...


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

quoted

A (minor) problem with the second possibility is that this naming
convention could evolve, eg. if we end up with something like was
proposed in [1] for 1.8.0.  Is there any real drawback with the

first

quoted

suggestion ?

[1] http://marc.info/?l=git&m=129661334011986&w=4

My gut feeling is to keep some sort of limit notes refs, and if/when we get 
around to implementing my proposal in [1] (or some variation thereof), we 
will of course extend the limit to put "refs/remotes/$remote/notes/*" (or 
whatever is decided) in the same category as "refs/notes/*".

In the meantime, I'm unsure if it's a good idea to remove the limitation 
altogether (allowing notes refs everywhere), since re-introducing a limit at 
a later point will then be MUCH harder...

I'm still unsure what that limitation brings to us.  OTOH, it has at least one
funny downside: when someone tries to refer to some forbidden ref using --ref, it
gets silently requalified:

$ git notes --ref=refs/remote-notes/foo add
$ find .git/refs/notes/ -type f
.git/refs/notes/refs/remote-notes/foo
$

It just seems so wrong...  Surely we can mitigate it by considering a ref starting
with "refs/" to be absolute, and thus never prepend "refs/notes/" to it, but it rather
sounds to me a symptom that we may not want to filter things anyway.

On Wednesday 15. June 2011, ydirson@free.fr wrote:

quoted

quoted

A (minor) problem with the second possibility is that this naming
convention could evolve, eg. if we end up with something like was
proposed in [1] for 1.8.0.  Is there any real drawback with the

first

quoted

suggestion ?

[1] http://marc.info/?l=git&m=129661334011986&w=4

My gut feeling is to keep some sort of limit notes refs, and
if/when we get around to implementing my proposal in [1] (or some
variation thereof), we will of course extend the limit to put
"refs/remotes/$remote/notes/*" (or whatever is decided) in the
same category as "refs/notes/*".

In the meantime, I'm unsure if it's a good idea to remove the
limitation altogether (allowing notes refs everywhere), since
re-introducing a limit at a later point will then be MUCH
harder...

I'm still unsure what that limitation brings to us.  OTOH, it has at
least one funny downside: when someone tries to refer to some
forbidden ref using --ref, it gets silently requalified:

$ git notes --ref=refs/remote-notes/foo add
$ find .git/refs/notes/ -type f
.git/refs/notes/refs/remote-notes/foo
$

It just seems so wrong...  Surely we can mitigate it by considering a
ref starting with "refs/" to be absolute, and thus never prepend
"refs/notes/" to it, but it rather sounds to me a symptom that we
may not want to filter things anyway.

The reason we put the limitation there, is to prevent the notes code 
from screwing with non-notes trees. The notes code reorganizes the notes 
tree depending on the number of tree entries, in order to achieve 
acceptable performance for notes trees of all sizes. Therefore, you 
definitely DON'T want the notes code rummaging around in non-notes trees 
(especially if some of your tree entries can be mistaken for strings of 
hex digits).

That said, the example you give above ("refs/remote-notes/foo" -> 
"refs/notes/refs/remote-notes/foo" is obviously a stupid failure, and 
should be fixed. Considering "refs/*" to be absolute seems safe to me. 
(Obviously we loose the "refs/notes/refs/*" namespace, but I can live 
with that.)


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

I'm still unsure what that limitation brings to us.  OTOH, it has at least one
funny downside: when someone tries to refer to some forbidden ref using --ref, it
gets silently requalified:

$ git notes --ref=refs/remote-notes/foo add
$ find .git/refs/notes/ -type f
.git/refs/notes/refs/remote-notes/foo
$

It just seems so wrong...  Surely we can mitigate it by considering a ref starting
with "refs/" to be absolute, and thus never prepend "refs/notes/" to it, but it rather
sounds to me a symptom that we may not want to filter things anyway.

While playing with this, I realized that when editing the template
does not name the notes ref being edited.  When looking at the code,
I notice that, contrarily to commit.c which uses stdio, notes.c uses
write_or_die(), which is a bit less flexible for formatting.

I'd think we could me things more consistent - is there any objection
to switch notes.c to using stdio for this ?

On Wednesday 15. June 2011, ydirson@free.fr wrote:

quoted

I'm still unsure what that limitation brings to us.  OTOH, it has
at least one funny downside: when someone tries to refer to some
forbidden ref using --ref, it gets silently requalified:

$ git notes --ref=refs/remote-notes/foo add
$ find .git/refs/notes/ -type f
.git/refs/notes/refs/remote-notes/foo
$

It just seems so wrong...  Surely we can mitigate it by considering
a ref starting with "refs/" to be absolute, and thus never prepend
"refs/notes/" to it, but it rather sounds to me a symptom that we
may not want to filter things anyway.

While playing with this, I realized that when editing the template
does not name the notes ref being edited.  When looking at the code,
I notice that, contrarily to commit.c which uses stdio, notes.c uses
write_or_die(), which is a bit less flexible for formatting.

I'd think we could me things more consistent - is there any objection
to switch notes.c to using stdio for this ?

Go ahead, Doing things in line with commit.c seems good to me.


Have fun! :)

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Patches 1 and 2 are just preparing things for patch 3.

Patch 4 is a (hopefully) temporary measure, to be able to implement a
real-life until notes workflow without having to wait for
refs/remotes/ to get its due restructuring,

Patch 5 addresses the anomaly reported earlier this week.

Patch 6 is a proposal to make "notes merge" more similar to "merge"

Signed-off-by: Yann Dirson <redacted>
---
 builtin/notes.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..bd342ac 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c

@@ -108,19 +108,19 @@ 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_data(FILE *fp, const unsigned char *sha1)
 {
 	unsigned long size;
 	enum object_type type;
 	char *buf = read_sha1_file(sha1, &type, &size);
 	if (buf) {
 		if (size)
-			write_or_die(fd, buf, size);
+			fwrite(buf, 1, size, fp);
 		free(buf);
 	}
 }
 
-static void write_commented_object(int fd, const unsigned char *object)
+static void write_commented_object(FILE *fp, const unsigned char *object)
 {
 	const char *show_args[5] =
 		{"show", "--stat", "--no-notes", sha1_to_hex(object), NULL};

@@ -144,11 +144,11 @@ static void write_commented_object(int fd, const unsigned char *object)
 	if (show_out == NULL)
 		die_errno(_("can't fdopen 'show' output fd"));
 
-	/* Prepend "# " to each output line and write result to 'fd' */
+	/* Prepend "# " to each output line and write result to 'fp' */
 	while (strbuf_getline(&buf, show_out, '\n') != EOF) {
-		write_or_die(fd, "# ", 2);
-		write_or_die(fd, buf.buf, buf.len);
-		write_or_die(fd, "\n", 1);
+		fwrite("# ", 1, 2, fp);
+		fwrite(buf.buf, 1, buf.len, fp);
+		fwrite("\n", 1, 1, fp);
 	}
 	strbuf_release(&buf);
 	if (fclose(show_out))

@@ -166,23 +166,23 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 	char *path = NULL;
 
 	if (msg->use_editor || !msg->given) {
-		int fd;
+		FILE *fp;
 
 		/* write the template message before editing: */
 		path = git_pathdup("NOTES_EDITMSG");
-		fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
-		if (fd < 0)
+		fp = fopen(path, "w");
+		if (fp == NULL)
 			die_errno(_("could not create file '%s'"), path);
 
 		if (msg->given)
-			write_or_die(fd, msg->buf.buf, msg->buf.len);
+			fwrite(msg->buf.buf, 1, msg->buf.len, fp);
 		else if (prev && !append_only)
-			write_note_data(fd, prev);
-		write_or_die(fd, note_template, strlen(note_template));
+			write_note_data(fp, prev);
+		fwrite(note_template, 1, strlen(note_template), fp);
 
-		write_commented_object(fd, object);
+		write_commented_object(fp, object);
 
-		close(fd);
+		fclose(fp);
 		strbuf_reset(&(msg->buf));
 
 		if (launch_editor(path, &(msg->buf), NULL)) {

-- 
1.7.5.3

This will still show for the default "commits" notes:

 # Write/edit notes for the following object:

For other notes refs it will show:

 # Write/edit "foo" notes for the following object:

Signed-off-by: Yann Dirson <redacted>
---
 builtin/notes.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index bd342ac..ae89d38 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c

@@ -91,7 +91,7 @@ static const char * const git_notes_get_ref_usage[] = {
 static const char note_template[] =
 	"\n"
 	"#\n"
-	"# Write/edit the notes for the following object:\n"
+	"# Write/edit %s%s%snotes for the following object:\n"
 	"#\n";
 
 struct msg_arg {

@@ -167,6 +167,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 
 	if (msg->use_editor || !msg->given) {
 		FILE *fp;
+		const char *ref = notes_ref_shortname(default_notes_tree.ref);
 
 		/* write the template message before editing: */
 		path = git_pathdup("NOTES_EDITMSG");

@@ -178,7 +179,10 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 			fwrite(msg->buf.buf, 1, msg->buf.len, fp);
 		else if (prev && !append_only)
 			write_note_data(fp, prev);
-		fwrite(note_template, 1, strlen(note_template), fp);
+		if (!ref)
+			fprintf(fp, note_template, "", "", "");
+		else
+			fprintf(fp, note_template, "\"", ref, "\" ");
 
 		write_commented_object(fp, object);
 

-- 
1.7.5.3

Signed-off-by: Yann Dirson <redacted>
---
 Documentation/git-notes.txt |    5 +++++
 builtin/notes.c             |    4 ++--
 notes.c                     |    5 +++--
 notes.h                     |    2 +-
 revision.c                  |    2 +-
 5 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 6a187f2..7ce8a24 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt

@@ -104,6 +104,11 @@ and instructs the user to manually resolve the conflicts there.
 When done, the user can either finalize the merge with
 'git notes merge --commit', or abort the merge with
 'git notes merge --abort'.
++
+In addition to `refs/notes/`, the remote notes ref is accepted
+from the `refs/remote-notes/` namespace.  This is intended to
+provide notes with support for a workflow similar to the one used
+for heads references.
 
 remove::
 	Remove the notes for given objects (defaults to HEAD). When

diff --git a/builtin/notes.c b/builtin/notes.c
index ae89d38..6bff44f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c

@@ -905,7 +905,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 
 	o.local_ref = default_notes_ref();
 	strbuf_addstr(&remote_ref, argv[0]);
-	expand_notes_ref(&remote_ref);
+	expand_notes_ref(&remote_ref, 1);
 	o.remote_ref = remote_ref.buf;
 
 	if (strategy) {

@@ -1075,7 +1075,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 	if (override_notes_ref) {
 		struct strbuf sb = STRBUF_INIT;
 		strbuf_addstr(&sb, override_notes_ref);
-		expand_notes_ref(&sb);
+		expand_notes_ref(&sb, 0);
 		setenv("GIT_NOTES_REF", sb.buf, 1);
 		strbuf_release(&sb);
 	}

diff --git a/notes.c b/notes.c
index 1a5676a..12afc02 100644
--- a/notes.c
+++ b/notes.c

@@ -1282,9 +1282,10 @@ int copy_note(struct notes_tree *t,
 	return 0;
 }
 
-void expand_notes_ref(struct strbuf *sb)
+void expand_notes_ref(struct strbuf *sb, int allow_remotes)
 {
-	if (!prefixcmp(sb->buf, "refs/notes/"))
+	if (!prefixcmp(sb->buf, "refs/notes/") ||
+	    (allow_remotes && !prefixcmp(sb->buf, "refs/remote-notes/")))
 		return; /* we're happy */
 	else if (!prefixcmp(sb->buf, "notes/"))
 		strbuf_insert(sb, 0, "refs/", 5);

diff --git a/notes.h b/notes.h
index d8ae29d..80219ec 100644
--- a/notes.h
+++ b/notes.h

@@ -317,6 +317,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 					 const char *globs);
 
 /* Expand inplace a note ref like "foo" or "notes/foo" into "refs/notes/foo" */
-void expand_notes_ref(struct strbuf *sb);
+void expand_notes_ref(struct strbuf *sb, int allow_remotes);
 
 #endif

diff --git a/revision.c b/revision.c
index c46cfaa..b482314 100644
--- a/revision.c
+++ b/revision.c

@@ -1393,7 +1393,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		}
 		else
 			strbuf_addstr(&buf, arg+8);
-		expand_notes_ref(&buf);
+		expand_notes_ref(&buf, 1);
 		string_list_append(&revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));
 	} else if (!strcmp(arg, "--no-notes")) {

-- 
1.7.5.3

Signed-off-by: Yann Dirson <redacted>
---
 notes.c |   24 ++++++++++++++++--------
 notes.h |    7 +++++++
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/notes.c b/notes.c
index f6ce848..1a5676a 100644
--- a/notes.c
+++ b/notes.c

@@ -1235,16 +1235,11 @@ void format_note(struct notes_tree *t, const unsigned char *object_sha1,
 		msglen--;
 
 	if (flags & NOTES_SHOW_HEADER) {
-		const char *ref = t->ref;
-		if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
+		const char *ref = notes_ref_shortname(t->ref);
+		if (!ref)
 			strbuf_addstr(sb, "\nNotes:\n");
-		} else {
-			if (!prefixcmp(ref, "refs/"))
-				ref += 5;
-			if (!prefixcmp(ref, "notes/"))
-				ref += 6;
+		else
 			strbuf_addf(sb, "\nNotes (%s):\n", ref);
-		}
 	}
 
 	for (msg_p = msg; msg_p < msg + msglen; msg_p += linelen + 1) {

@@ -1296,3 +1291,16 @@ void expand_notes_ref(struct strbuf *sb)
 	else
 		strbuf_insert(sb, 0, "refs/notes/", 11);
 }
+
+const char *notes_ref_shortname(const char *ref)
+{
+	if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF))
+		return NULL;
+	else {
+		if (!prefixcmp(ref, "refs/"))
+			ref += 5;
+		if (!prefixcmp(ref, "notes/"))
+			ref += 6;
+		return ref;
+	}
+}

diff --git a/notes.h b/notes.h
index c716694..d8ae29d 100644
--- a/notes.h
+++ b/notes.h

@@ -64,6 +64,13 @@ extern struct notes_tree {
 const char *default_notes_ref(void);
 
 /*
+ * Return a short name for a notes ref, suitable for display to the user.
+ *
+ * No copy is done, the return value is a pointer into the original string.
+ */
+const char *notes_ref_shortname(const char *ref);
+
+/*
  * Flags controlling behaviour of notes tree initialization
  *
  * Default behaviour is to initialize the notes tree from the tree object

-- 
1.7.5.3

This caused strange behaviour when "git notes" was asked to manipulate
refs/<anything> outside of refs/notes/: it was attempting to use
refs/notes/refs/<anything>.

Dying early this way should avoid the need to check for the
refs/notes/ prefix in several places.

Signed-off-by: Yann Dirson <redacted>
---
 notes.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/notes.c b/notes.c
index 12afc02..c6a82da 100644
--- a/notes.c
+++ b/notes.c

@@ -1289,6 +1289,8 @@ void expand_notes_ref(struct strbuf *sb, int allow_remotes)
 		return; /* we're happy */
 	else if (!prefixcmp(sb->buf, "notes/"))
 		strbuf_insert(sb, 0, "refs/", 5);
+	else if (!prefixcmp(sb->buf, "refs/"))
+		die(_("Not a notes reference: %s"), sb->buf);
 	else
 		strbuf_insert(sb, 0, "refs/notes/", 11);
 }

-- 
1.7.5.3

This causes the "merge empty notes ref (z => y)" test in t3308-notes-merge.sh
to fail - obviously, it is removing the functionnality that is tested for.

Is there any real use for this ?  It just seems so different from
"git merge", which errors out in the similar situation:

$ git merge foo
fatal: 'foo' does not point to a commit

Signed-off-by: Yann Dirson <redacted>
---
 builtin/notes.c        |    3 +++
 t/t3308-notes-merge.sh |    6 ------
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 6bff44f..058b14d 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c

@@ -908,6 +908,9 @@ static int merge(int argc, const char **argv, const char *prefix)
 	expand_notes_ref(&remote_ref, 1);
 	o.remote_ref = remote_ref.buf;
 
+	if (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))
+		die("'%s' does not point to a commit", o.remote_ref);
+
 	if (strategy) {
 		if (!strcmp(strategy, "manual"))
 			o.strategy = NOTES_MERGE_RESOLVE_MANUAL;

diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b4..2dcc1db 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh

@@ -104,12 +104,6 @@ test_expect_success 'merge notes into empty notes ref (x => y)' '
 	test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)"
 '
 
-test_expect_success 'merge empty notes ref (z => y)' '
-	git notes merge z &&
-	# y should not change (still == x)
-	test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)"
-'
-
 test_expect_success 'change notes on other notes ref (y)' '
 	# Not touching notes to 1st commit
 	git notes remove 2nd &&

-- 
1.7.5.3

On Saturday 18 June 2011, Yann Dirson wrote:

Signed-off-by: Yann Dirson <redacted>

Please mention in the commit message that the commit merely replaces 
write_or_die()/int fd with the corresponding stdio functionality, and that 
there is no (intended) change in behavior. It was not apparent from your 
commit message that you had not made any other changes.

Otherwise the patch looks OK.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Saturday 18 June 2011, Yann Dirson wrote:

Signed-off-by: Yann Dirson <redacted>
---
 notes.c |   24 ++++++++++++++++--------
 notes.h |    7 +++++++
 2 files changed, 23 insertions(+), 8 deletions(-)

[...]

 /*
+ * Return a short name for a notes ref, suitable for display to the user.
+ *
+ * No copy is done, the return value is a pointer into the original string.
+ */
+const char *notes_ref_shortname(const char *ref);
+
+/*

Please include in the documentation what a NULL return means.

Otherwise the patch looks OK.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Saturday 18 June 2011, Yann Dirson wrote:

I would rephrase this as:

  In addition to `refs/notes/*`, the remote notes ref can also be
  from within `refs/remote-notes/*`. This allows the user to set up
  fetch refspecs that transfers notes refs from a remote repo into
  `refs/remote-notes/*`, and then merge those remote notes refs into
  the corresponding local notes refs.

Also, AFAICS you're adding the possibility to read notes from refs/remote-
notes/*, but not WRITE to those notes using "git notes" (obviously, "git 
fetch" and other tools can be used to manipulate them). Please add some 
selftests verifying that "git notes" is still unable to manipulate notes in 
refs/remote-notes/*.

Otherwise the patch looks good to me.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Saturday 18 June 2011, Yann Dirson wrote:

Patches 1 and 2 are just preparing things for patch 3.

Patch 4 is a (hopefully) temporary measure, to be able to implement a
real-life until notes workflow without having to wait for
refs/remotes/ to get its due restructuring,

Patch 5 addresses the anomaly reported earlier this week.

Patch 6 is a proposal to make "notes merge" more similar to "merge"

This series looks good to me, unless otherwise noted in my replies to 
individual patches.


Thanks for your work!

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Saturday 18 June 2011, Yann Dirson wrote:

This causes the "merge empty notes ref (z => y)" test in
t3308-notes-merge.sh to fail - obviously, it is removing the
functionnality that is tested for.

Is there any real use for this ?  It just seems so different from
"git merge", which errors out in the similar situation:

$ git merge foo
fatal: 'foo' does not point to a commit

I understand your reasoning, and I don't have a problem with changing this 
behavior to be in line with "git merge".

Hmm. I'm not sure requiring the remote ref to always point to a _commit_ is 
the right solution here. In previous discussions on the notes topic, some 
people (Peff?) expressed a need/interest for history-less notes refs (i.e. a 
notes tree where we don't keep track of its development, but only refer to 
the latest/current version). Obviously, there are two ways to implement 
history-less notes refs: (a) making the notes ref point to a notes commit 
without any parents (i.e. each notes commit is a root commit), or (b) making 
the notes ref point directly at the notes _tree_ object (i.e. no commit 
object at all).

I can't remember off the top of my head whether our earlier discussions on 
this topic resulted in us excluding support for option (b), but if we 
didn't, it should be possible to merge notes refs where one or both refs 
point directly at a tree object, and your above line would break this.

Instead of removing the test, please change it into verifying the _new_ 
expected behavior (that we fail with an appropriate error message when asked 
to merge a non-existent notes ref).


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Mon, Jun 20, 2011 at 12:03:46AM +0200, Johan Herland wrote:

quoted

+	if (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))
+		die("'%s' does not point to a commit", o.remote_ref);

Hmm. I'm not sure requiring the remote ref to always point to a _commit_ is 
the right solution here. In previous discussions on the notes topic, some 
people (Peff?) expressed a need/interest for history-less notes refs (i.e. a 
notes tree where we don't keep track of its development, but only refer to 
the latest/current version). Obviously, there are two ways to implement 
history-less notes refs: (a) making the notes ref point to a notes commit 
without any parents (i.e. each notes commit is a root commit), or (b) making 
the notes ref point directly at the notes _tree_ object (i.e. no commit 
object at all).

I can't remember off the top of my head whether our earlier discussions on 
this topic resulted in us excluding support for option (b), but if we 
didn't, it should be possible to merge notes refs where one or both refs 
point directly at a tree object, and your above line would break this.

The notes-cache.[ch] implementation uses history-less notes for textconv
caching. Since it's just a cache, we don't care about history or
merging. And keeping a history would just mean useless old versions of
the cache are kept longer than necessary.

I ended up using a commit with no parents to store the cache. I don't
recall offhand whether there were any complications with using a raw
tree, but I realized that I needed some place to put extra metadata like
the cache validity. Wrapping the tree object in a commit provided that
place.

I don't think there is any real reason for somebody to need a bare tree
of notes. There is a certain elegance that refs can point directly to
trees in git, but the overhead of a single commit object to wrap it is
just not a big deal[1].

I didn't test, but I doubt that "git merge" will handle bare trees; this
would provide analagous behavior for notes-merging.  But maybe I'm
wrong.

-Peff

[1] The only other time I recall seeing a bare tree is linux-2.6's
v2.6.11 tag. And even there it is wrapped by a tag object, so that Linus
could include metadata (a comment and a GPG signature). There's really
no reason that couldn't have had a commit, except that doing it as a
tree shows off how cool git is. :)

On Monday 20 June 2011, Jeff King wrote:

On Mon, Jun 20, 2011 at 12:03:46AM +0200, Johan Herland wrote:

quoted

quoted

+	if (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))
+		die("'%s' does not point to a commit", o.remote_ref);

Hmm. I'm not sure requiring the remote ref to always point to a
_commit_ is the right solution here. In previous discussions on the
notes topic, some people (Peff?) expressed a need/interest for
history-less notes refs (i.e. a notes tree where we don't keep track
of its development, but only refer to the latest/current version).
Obviously, there are two ways to implement history-less notes refs:
(a) making the notes ref point to a notes commit without any parents
(i.e. each notes commit is a root commit), or (b) making the notes ref
point directly at the notes _tree_ object (i.e. no commit object at
all).

I can't remember off the top of my head whether our earlier discussions
on this topic resulted in us excluding support for option (b), but if
we didn't, it should be possible to merge notes refs where one or both
refs point directly at a tree object, and your above line would break
this.

[...]

I don't think there is any real reason for somebody to need a bare tree
of notes. There is a certain elegance that refs can point directly to
trees in git, but the overhead of a single commit object to wrap it is
just not a big deal.

I didn't test, but I doubt that "git merge" will handle bare trees; this
would provide analagous behavior for notes-merging.  But maybe I'm
wrong.

You're not wrong. "git merge" when trying to merge a tree object:

  $ git merge ee314a3
  error: ee314a3: expected commit type, but the object dereferences to tree type
  fatal: 'ee314a3' does not point to a commit

So I guess there's no reason to allow notes trees with no commit object.

Yann: Please disregard my complaint on the above two lines.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net