threads / discuss / 27611

Commit notes workflow

Subject: Commit notes workflow

## tl;dr

27 messages between Jun 13, 2011 and Jun 21, 2011.

replies: 26people: 5as markdown or json

Yann Dirson· Jun 13, 2011, 07:09 UTC · lore

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
Johan Herland· Jun 14, 2011, 10:15 UTC · re: Yann Dirson · lore

Re: Commit notes workflow

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

Show 18 quoted lines
> 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)
Show 6 quoted lines
> 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
ydirson@free.fr· Jun 15, 2011, 09:20 UTC · re: Johan Herland · lore

Re: Commit notes workflow

Show 16 quoted lines
> > 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.

Johan Herland· Jun 15, 2011, 09:37 UTC · re: ydirson@free.fr · lore

Re: Commit notes workflow

On Wednesday 15. June 2011, ydirson@free.fr wrote:
Show 34 quoted lines
> > > 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
ydirson@free.fr· Jun 15, 2011, 09:57 UTC · re: ydirson@free.fr · lore

Re: Commit notes workflow

Show 12 quoted lines
> 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 ?

Johan Herland· Jun 15, 2011, 10:53 UTC · re: ydirson@free.fr · lore

Re: Commit notes workflow

On Wednesday 15. June 2011, ydirson@free.fr wrote:
Show 21 quoted lines
> > 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
Yann Dirson· Jun 18, 2011, 21:06 UTC · re: Johan Herland · lore

[PATCH 0/6] Small notes usability improvements

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"
Yann Dirson· Jun 18, 2011, 21:06 UTC · re: Yann Dirson · lore

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

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
Johan Herland· Jun 19, 2011, 21:23 UTC · re: Yann Dirson · lore

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

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
Junio C Hamano· Jun 19, 2011, 22:50 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 9 quoted lines
> 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.

Johan Herland· Jun 20, 2011, 07:41 UTC · re: Junio C Hamano · lore

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

On Monday 20 June 2011, Junio C Hamano wrote:
Show 13 quoted lines
> 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
Yann Dirson· Jun 20, 2011, 18:48 UTC · re: Johan Herland · lore

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

On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:
Show 17 quoted lines
> 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.
Yann Dirson· Jun 21, 2011, 19:39 UTC · re: Yann Dirson · lore

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

On Mon, Jun 20, 2011 at 08:48:42PM +0200, Yann Dirson wrote:
Show 22 quoted lines
> 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 ?

Show 6 quoted lines
> > 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
Yann Dirson· Jun 18, 2011, 21:06 UTC · re: Yann Dirson · lore

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

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
Johan Herland· Jun 19, 2011, 21:25 UTC · re: Yann Dirson · lore

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

On Saturday 18 June 2011, Yann Dirson wrote:
Show 16 quoted lines
> 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
Junio C Hamano· Jun 19, 2011, 22:51 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 21 quoted lines
> 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?
Yann Dirson· Jun 20, 2011, 18:49 UTC · re: Junio C Hamano · lore

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

On Sun, Jun 19, 2011 at 03:51:34PM -0700, Junio C Hamano wrote:
Show 28 quoted lines
> 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 ;)

Yann Dirson· Jun 18, 2011, 21:06 UTC · re: Yann Dirson · lore

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

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
Yann Dirson· Jun 18, 2011, 21:06 UTC · re: Yann Dirson · lore

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

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
Johan Herland· Jun 19, 2011, 21:45 UTC · re: Yann Dirson · lore

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

On Saturday 18 June 2011, Yann Dirson wrote:
Show 22 quoted lines
> 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
Yann Dirson· Jun 18, 2011, 21:06 UTC · re: Yann Dirson · lore

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

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
Yann Dirson· Jun 18, 2011, 21:06 UTC · re: Yann Dirson · lore

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

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
Johan Herland· Jun 19, 2011, 22:03 UTC · re: Yann Dirson · lore

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

On Saturday 18 June 2011, Yann Dirson wrote:
Show 9 quoted lines
> 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".

Show 16 quoted lines
> 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.

Show 15 quoted lines
> [...]
> 
> 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
Jeff King· Jun 20, 2011, 07:16 UTC · re: Johan Herland · lore

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

On Mon, Jun 20, 2011 at 12:03:46AM +0200, Johan Herland wrote:
Show 17 quoted lines
> > +	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. :)

Johan Herland· Jun 20, 2011, 07:29 UTC · re: Jeff King · lore

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

On Monday 20 June 2011, Jeff King wrote:
Show 31 quoted lines
> 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
Johan Herland· Jun 19, 2011, 22:06 UTC · re: Yann Dirson · lore

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

On Saturday 18 June 2011, Yann Dirson wrote:
Show 9 quoted lines
> 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
Johan Herland· Jun 14, 2011, 14:41 UTC · lore

Re: Commit notes workflow

On Tuesday 14. June 2011, dirson@bertin.fr wrote:
Show 37 quoted lines
> > > 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.

Show 10 quoted lines
> 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

← back to recent threads