Commit notes workflow

Commit notes workflow

[Yann Dirson]

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

Re: Commit notes workflow

[Johan Herland]

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, <johan@herland.net>
www.herland.net

Re: Commit notes workflow

[Johan Herland]

On Tuesday 14. June 2011, dirson@bertin.fr wrote:
> > > 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...
> 
> So we could introduce something like refs/remote-notes/<remote>/*
> today to start working, and eventually phase it out when
> refs/remotes/ gets restructured.

Yes, if you can't wait for the refs/remotes/ restructuring, then I guess 
you'll have to do that.

> Then the next point will be how best to provide git-pull-like support
> for notes refs. We have a number of alternatives, like:
> 
> * having "git pull" run "git notes merge" on all notes refs with a
> tracking-branch set to the repo from which we pull
> * do the same for a configured set of notes refs only
> * only have "git pull" and "git status" notify about notes refs being
> not uptodate, and add an explicit "git notes pull" command of some
> sort (maybe just "git notes merge" without an argument, which would
> be consistent with latest "git merge") * surely others

I guess there are a lot of different possibilities here, and there will 
probably be disagreement on what's the best default, so I'd suggest the 
following guidelines:

* make it as configurable as possible.

* follow the existing conventions of pull/merge w.r.t. branches, but 
only so far as it makes sense for notes.

* leave the defaults conservative (e.g. don't do any merging by default, 
but make pull/status notify about update-able notes refs).


My idea so far, is to model the notes configuration on the current 
branch configuration, e.g. something like this:

  [remote "origin"]
      ...
      fetch = +refs/notes/*:refs/remotes/origin/notes/*
      ...

  [notes "commits"]
      remote = origin
      merge = refs/notes/commits

  [notes "bugs"]
      remote = origin
      merge = refs/notes/bugs
      mergeoptions = --strategy=cat_sort_uniq
      automerge = true

Except for the "automerge" option, everything is analogous to current 
branch.<name>.* options. The above configuration sets up a default 
tracking ref for "refs/notes/commits", making

  git notes --ref commits merge

equivalent to

  git notes --ref commits merge refs/remotes/origin/notes/commits

This notes merge would not happen automatically.

The last section, however, would presumably trigger an automatic notes 
merge (on fetch? pull?) because of notes.bugs.automerge being enabled. 
In this case, the

  git notes --ref bugs merge

command would be issued, which would be equivalent to

  git notes --ref bugs merge --strategy=cat_sort_uniq \
      refs/remotes/origin/notes/bugs

This is just a suggestion, and we might want to impose additional 
restrictions not mentioned above. For example, enabling "automerge" 
without enabling a non-"manual" notes merge strategy is probably unwise, 
because it can force the user to resolve conflicts from a notes merge 
that the user did not explicitly initiate.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: Commit notes workflow

[ydirson]

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

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.

Re: Commit notes workflow

[Johan Herland]

On Wednesday 15. June 2011, ydirson@free.fr wrote:
> > > 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...
> 
> 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, <johan@herland.net>
www.herland.net

Re: Commit notes workflow

[ydirson]

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

Re: Commit notes workflow

[Johan Herland]

On Wednesday 15. June 2011, ydirson@free.fr wrote:
> > 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, <johan@herland.net>
www.herland.net

[PATCH 0/6] Small notes usability improvements

[Yann Dirson]

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"

[PATCH 1/6] Bring notes.c template handling in line with commit.c.

[Yann Dirson]

Signed-off-by: Yann Dirson <ydirson@free.fr>
---
 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

[PATCH 2/6] Factorize shortening of notes refname for display.

[Yann Dirson]

Signed-off-by: Yann Dirson <ydirson@free.fr>
---
 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

[PATCH 3/6] Include name of notes ref in template when creating/editing notes.

[Yann Dirson]

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 <ydirson@free.fr>
---
 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

[PATCH 4/6] Allow "git notes merge" to use refs/remote-notes/ as a source.

[Yann Dirson]

Signed-off-by: Yann Dirson <ydirson@free.fr>
---
 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

[PATCH 5/6] Assume a note ref starting with refs must not be prepended refs/notes/.

[Yann Dirson]

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 <ydirson@free.fr>
---
 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

[PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.

[Yann Dirson]

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 <ydirson@free.fr>
---
 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

Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.

[Johan Herland]

On Saturday 18 June 2011, Yann Dirson wrote:
> Signed-off-by: Yann Dirson <ydirson@free.fr>

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, <johan@herland.net>
www.herland.net

Re: [PATCH 2/6] Factorize shortening of notes refname for display.

[Johan Herland]

On Saturday 18 June 2011, Yann Dirson wrote:
> Signed-off-by: Yann Dirson <ydirson@free.fr>
> ---
>  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, <johan@herland.net>
www.herland.net

Re: [PATCH 4/6] Allow "git notes merge" to use refs/remote-notes/ as a source.

[Johan Herland]

On Saturday 18 June 2011, Yann Dirson wrote:
> Signed-off-by: Yann Dirson <ydirson@free.fr>
> ---
>  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.

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, <johan@herland.net>
www.herland.net

Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.

[Johan Herland]

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

> Signed-off-by: Yann Dirson <ydirson@free.fr>
> ---
>  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);

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.

> [...]
> 
> 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)"
> -'

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, <johan@herland.net>
www.herland.net

Re: [PATCH 0/6] Small notes usability improvements

[Johan Herland]

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, <johan@herland.net>
www.herland.net

Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Saturday 18 June 2011, Yann Dirson wrote:
>> Signed-off-by: Yann Dirson <ydirson@free.fr>
>
> 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.

I had an impression that you would lose a lot of error checking, unless
you are careful, if you go from write_or_die() to stdio.

Re: [PATCH 2/6] Factorize shortening of notes refname for display.

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Saturday 18 June 2011, Yann Dirson wrote:
>> Signed-off-by: Yann Dirson <ydirson@free.fr>
>> ---
>>  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.

It may be just me, but every time somebody says Factorize, I find myself
looking for math textbook from middle school days, 12 = 2 * 2 * 3, etc.

Could we say Refactor instead pretty please?

Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.

[Jeff King]

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

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

Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.

[Johan Herland]

On Monday 20 June 2011, Jeff King wrote:
> On Mon, Jun 20, 2011 at 12:03:46AM +0200, Johan Herland wrote:
> > > +	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, <johan@herland.net>
www.herland.net

Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.

[Johan Herland]

On Monday 20 June 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > On Saturday 18 June 2011, Yann Dirson wrote:
> >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> > 
> > 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.
> 
> I had an impression that you would lose a lot of error checking, unless
> you are careful, if you go from write_or_die() to stdio.

Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one 
needs to check the return value, and handle errors accordingly.

An alternative solution would be to drop this patch, and instead use 
strbuf_addf() to get the format printing functionality needed in PATCH 3/6.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.

[Yann Dirson]

On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:
> On Monday 20 June 2011, Junio C Hamano wrote:
> > Johan Herland <johan@herland.net> writes:
> > > On Saturday 18 June 2011, Yann Dirson wrote:
> > >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> > > 
> > > 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.
> > 
> > I had an impression that you would lose a lot of error checking, unless
> > you are careful, if you go from write_or_die() to stdio.
> 
> Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one 
> needs to check the return value, and handle errors accordingly.

It appears I based my code on buildin/commit.c from 1.7.4.1 - I just
did not realize that this part changed much in between with 098d0e0e.
I'll look into that.

> An alternative solution would be to drop this patch, and instead use 
> strbuf_addf() to get the format printing functionality needed in PATCH 3/6.

I have thought about that, but that will make the i18n process for the
template much more awkward - and we probably don't want to reimplement
stdio formatting for strbuf.

-- 
Yann.

Re: [PATCH 2/6] Factorize shortening of notes refname for display.

[Yann Dirson]

On Sun, Jun 19, 2011 at 03:51:34PM -0700, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> 
> > On Saturday 18 June 2011, Yann Dirson wrote:
> >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> >> ---
> >>  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.
> 
> It may be just me, but every time somebody says Factorize, I find myself
> looking for math textbook from middle school days, 12 = 2 * 2 * 3, etc.
> 
> Could we say Refactor instead pretty please?

Well, factorizing is just one kind of code refactoring - but OK, I'll
do that ;)

Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.

[Yann Dirson]

On Mon, Jun 20, 2011 at 08:48:42PM +0200, Yann Dirson wrote:
> On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:
> > On Monday 20 June 2011, Junio C Hamano wrote:
> > > Johan Herland <johan@herland.net> writes:
> > > > On Saturday 18 June 2011, Yann Dirson wrote:
> > > >> Signed-off-by: Yann Dirson <ydirson@free.fr>
> > > > 
> > > > 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.
> > > 
> > > I had an impression that you would lose a lot of error checking, unless
> > > you are careful, if you go from write_or_die() to stdio.
> > 
> > Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one 
> > needs to check the return value, and handle errors accordingly.
> 
> It appears I based my code on buildin/commit.c from 1.7.4.1 - I just
> did not realize that this part changed much in between with 098d0e0e.
> I'll look into that.

Hm.  So now builtin/commit.c heavily relies on status_printf_*.  Those
do not do much more return-checking on fprintf() than the previous
code - but at least they provide a single point where such
return-checking can be inserted, which is already better.

Now, those require a wt_status struct... but AFAICT, it only uses the
FILE* inside.  This seems a bit annoying for the purpose of reusing
the #-prefixing and line-folding mechanism in builtin/notes.c.  Would
replacing those funcs with FILE*-based ones - let's say,
status_printf_*_fp() - and wrappers with the current names in
wt-status.h be seen as a good idea ?


> > An alternative solution would be to drop this patch, and instead use 
> > strbuf_addf() to get the format printing functionality needed in PATCH 3/6.
> 
> I have thought about that, but that will make the i18n process for the
> template much more awkward - and we probably don't want to reimplement
> stdio formatting for strbuf.

... and for that matter, it looks like status_printf* provide us with
all that's needed.

-- 
Yann