{"thread":{"id":"27611","subject":"Commit notes workflow","startedAt":"2011-06-13T07:09:40Z","lastAt":"2011-06-21T19:39:52Z","messageCount":27,"participants":["Yann Dirson","Johan Herland","ydirson@free.fr","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"169922","messageId":"20110613090940.664b1b97@chalon.bertin.fr","threadId":"27611","inReplyTo":null,"subject":"Commit notes workflow","fromName":"Yann Dirson","fromEmail":"dirson@bertin.fr","sentAt":"2011-06-13T07:09:40Z","receivedAt":"2011-06-13T07:09:40Z","isPatch":false,"sender":{"key":"dirson@bertin.fr","avatar":null},"body":"We have notes merge support since a couple of releases now, but no real example\nin the docs of how best to use that. That is, no suggested mapping of remote notes,\nlet alone automatic setup of refspecs at clone time.\n\nTrying to setup such refspecs, I find myself puzzled:\n\n* if I store remote notes under refs/notes (eg. refs/notes/*:refs/notes/origin/* as fetch\n refspec), then a refs/notes/*:refs/notes/origin/* push refspec will include\n refs/notes/origin/*, which we obviously don't want\n\n* if I store them outside of refs/notes (eg. refs/notes/*:refs/remote-notes/origin/* ),\n then \"git notes\" silently ignores them: no output nor any error message from \"notes list\"\n or \"notes merge\".\n\nDo we really want to \"git notes\" to ignore everything not in refs/notes/ ? I can think of\n2 possibilities out of this situation:\n\n* remove that limitation\n* decide on a naming convention for remote notes, and teach \"git notes\" not to ignore it\n\nA (minor) problem with the second possibility is that this naming convention could evolve,\neg. if we end up with something like was proposed in [1] for 1.8.0. Is there any real drawback\nwith the first suggestion ?\n\n[1] http://marc.info/?l=git&m=129661334011986&w=4\n-- \nYann Dirson - Bertin Technologies\n"},{"id":"169991","messageId":"201106141215.50689.johan@herland.net","threadId":"27611","inReplyTo":"20110613090940.664b1b97@chalon.bertin.fr","subject":"Re: Commit notes workflow","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-14T10:15:50Z","receivedAt":"2011-06-14T10:15:50Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 13 June 2011, Yann Dirson wrote:\n> We have notes merge support since a couple of releases now, but no real\n> example in the docs of how best to use that. That is, no suggested\n> mapping of remote notes, let alone automatic setup of refspecs at clone\n> time.\n\nTrue. I think this has been held up, partly because I (or anyone else) \nhaven't found the time to work on this, and partly because we want to add \nsome kind of default refspec to easily share notes between repos; the latter \nhas been caught up in the discussion you refer to in [1].\n\n> Trying to setup such refspecs, I find myself puzzled:\n> \n> * if I store remote notes under refs/notes (eg.\n> refs/notes/*:refs/notes/origin/* as fetch refspec), then a\n> refs/notes/*:refs/notes/origin/* push refspec will include\n> refs/notes/origin/*, which we obviously don't want\n> \n> * if I store them outside of refs/notes (eg.\n> refs/notes/*:refs/remote-notes/origin/* ), then \"git notes\" silently\n> ignores them: no output nor any error message from \"notes list\" or\n> \"notes merge\".\n> \n> Do we really want to \"git notes\" to ignore everything not in refs/notes/\n> ? I can think of 2 possibilities out of this situation:\n> \n> * remove that limitation\n> * decide on a naming convention for remote notes, and teach \"git notes\"\n> not to ignore it\n\nThe naming convention I have proposed (in the discussion for [1]) is \n\n refs/notes/*:refs/remotes/$remote/notes/*\n\n(but it obviously depends on reorganizing the entire remote refs hierarchy)\n\n> A (minor) problem with the second possibility is that this naming\n> convention could evolve, eg. if we end up with something like was\n> proposed in [1] for 1.8.0. Is there any real drawback with the first\n> suggestion ?\n> \n> [1] http://marc.info/?l=git&m=129661334011986&w=4\n\nMy gut feeling is to keep some sort of limit notes refs, and if/when we get \naround to implementing my proposal in [1] (or some variation thereof), we \nwill of course extend the limit to put \"refs/remotes/$remote/notes/*\" (or \nwhatever is decided) in the same category as \"refs/notes/*\".\n\nIn the meantime, I'm unsure if it's a good idea to remove the limitation \naltogether (allowing notes refs everywhere), since re-introducing a limit at \na later point will then be MUCH harder...\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170006","messageId":"201106141641.14257.johan@herland.net","threadId":"27611","inReplyTo":"f81891b81d39.4df76a5c@bertin.fr","subject":"Re: Commit notes workflow","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-14T14:41:13Z","receivedAt":"2011-06-14T14:41:13Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 14. June 2011, dirson@bertin.fr wrote:\n> > > Do we really want to \"git notes\" to ignore everything not in\n> > > refs/notes/ ? I can think of 2 possibilities out of this\n> > > situation:\n> > > \n> > > * remove that limitation\n> > > * decide on a naming convention for remote notes, and teach \"git\n> > > notes\" not to ignore it\n> > \n> > The naming convention I have proposed (in the discussion for\n> > [1]) is\n> > \n> > refs/notes/*:refs/remotes/$remote/notes/*\n> > \n> > (but it obviously depends on reorganizing the entire remote refs\n> > hierarchy)\n> > \n> > > A (minor) problem with the second possibility is that this naming\n> > > convention could evolve, eg. if we end up with something like was\n> > > proposed in [1] for 1.8.0. Is there any real drawback with the\n> > > first suggestion ?\n> > > \n> > > [1] http://marc.info/?l=git&m=129661334011986&w=4\n> > \n> > My gut feeling is to keep some sort of limit notes refs, and\n> > if/when we get around to implementing my proposal in [1] (or some\n> > variation thereof), we will of course extend the limit to put\n> > \"refs/remotes/$remote/notes/*\" (or whatever is decided) in the\n> > same category as \"refs/notes/*\".\n> > \n> > In the meantime, I'm unsure if it's a good idea to remove the\n> > limitation altogether (allowing notes refs everywhere), since\n> > re- introducing a limit at a later point will then be MUCH\n> > harder...\n> \n> So we could introduce something like refs/remote-notes//*\n> today to start working, and eventually phase it out when\n> refs/remotes/ gets restructured.\n\nYes, if you can't wait for the refs/remotes/ restructuring, then I guess \nyou'll have to do that.\n\n> Then the next point will be how best to provide git-pull-like support\n> for notes refs. We have a number of alternatives, like:\n> \n> * having \"git pull\" run \"git notes merge\" on all notes refs with a\n> tracking-branch set to the repo from which we pull\n> * do the same for a configured set of notes refs only\n> * only have \"git pull\" and \"git status\" notify about notes refs being\n> not uptodate, and add an explicit \"git notes pull\" command of some\n> sort (maybe just \"git notes merge\" without an argument, which would\n> be consistent with latest \"git merge\") * surely others\n\nI guess there are a lot of different possibilities here, and there will \nprobably be disagreement on what's the best default, so I'd suggest the \nfollowing guidelines:\n\n* make it as configurable as possible.\n\n* follow the existing conventions of pull/merge w.r.t. branches, but \nonly so far as it makes sense for notes.\n\n* leave the defaults conservative (e.g. don't do any merging by default, \nbut make pull/status notify about update-able notes refs).\n\n\nMy idea so far, is to model the notes configuration on the current \nbranch configuration, e.g. something like this:\n\n [remote \"origin\"]\n ...\n fetch = +refs/notes/*:refs/remotes/origin/notes/*\n ...\n\n [notes \"commits\"]\n remote = origin\n merge = refs/notes/commits\n\n [notes \"bugs\"]\n remote = origin\n merge = refs/notes/bugs\n mergeoptions = --strategy=cat_sort_uniq\n automerge = true\n\nExcept for the \"automerge\" option, everything is analogous to current \nbranch..* options. The above configuration sets up a default \ntracking ref for \"refs/notes/commits\", making\n\n git notes --ref commits merge\n\nequivalent to\n\n git notes --ref commits merge refs/remotes/origin/notes/commits\n\nThis notes merge would not happen automatically.\n\nThe last section, however, would presumably trigger an automatic notes \nmerge (on fetch? pull?) because of notes.bugs.automerge being enabled. \nIn this case, the\n\n git notes --ref bugs merge\n\ncommand would be issued, which would be equivalent to\n\n git notes --ref bugs merge --strategy=cat_sort_uniq \\\n refs/remotes/origin/notes/bugs\n\nThis is just a suggestion, and we might want to impose additional \nrestrictions not mentioned above. For example, enabling \"automerge\" \nwithout enabling a non-\"manual\" notes merge strategy is probably unwise, \nbecause it can force the user to resolve conflicts from a notes merge \nthat the user did not explicitly initiate.\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170045","messageId":"243233943.1008861308129613120.JavaMail.root@zimbra44-e7.priv.proxad.net","threadId":"27611","inReplyTo":"201106141215.50689.johan@herland.net","subject":"Re: Commit notes workflow","fromName":"","fromEmail":"ydirson@free.fr","sentAt":"2011-06-15T09:20:13Z","receivedAt":"2011-06-15T09:20:13Z","isPatch":false,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"> > A (minor) problem with the second possibility is that this naming\n> > convention could evolve, eg. if we end up with something like was\n> > proposed in [1] for 1.8.0. Is there any real drawback with the\n> first\n> > suggestion ?\n> > \n> > [1] http://marc.info/?l=git&m=129661334011986&w=4\n> \n> My gut feeling is to keep some sort of limit notes refs, and if/when we get \n> around to implementing my proposal in [1] (or some variation thereof), we \n> will of course extend the limit to put \"refs/remotes/$remote/notes/*\" (or \n> whatever is decided) in the same category as \"refs/notes/*\".\n> \n> In the meantime, I'm unsure if it's a good idea to remove the limitation \n> altogether (allowing notes refs everywhere), since re-introducing a limit at \n> a later point will then be MUCH harder...\n\nI'm still unsure what that limitation brings to us. OTOH, it has at least one\nfunny downside: when someone tries to refer to some forbidden ref using --ref, it\ngets silently requalified:\n\n$ git notes --ref=refs/remote-notes/foo add\n$ find .git/refs/notes/ -type f\n.git/refs/notes/refs/remote-notes/foo\n$\n\nIt just seems so wrong... Surely we can mitigate it by considering a ref starting\nwith \"refs/\" to be absolute, and thus never prepend \"refs/notes/\" to it, but it rather\nsounds to me a symptom that we may not want to filter things anyway.\n"},{"id":"170048","messageId":"201106151137.43231.johan@herland.net","threadId":"27611","inReplyTo":"243233943.1008861308129613120.JavaMail.root@zimbra44-e7.priv.proxad.net","subject":"Re: Commit notes workflow","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-15T09:37:43Z","receivedAt":"2011-06-15T09:37:43Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wednesday 15. June 2011, ydirson@free.fr wrote:\n> > > A (minor) problem with the second possibility is that this naming\n> > > convention could evolve, eg. if we end up with something like was\n> > > proposed in [1] for 1.8.0. Is there any real drawback with the\n> > \n> > first\n> > \n> > > suggestion ?\n> > > \n> > > [1] http://marc.info/?l=git&m=129661334011986&w=4\n> > \n> > My gut feeling is to keep some sort of limit notes refs, and\n> > if/when we get around to implementing my proposal in [1] (or some\n> > variation thereof), we will of course extend the limit to put\n> > \"refs/remotes/$remote/notes/*\" (or whatever is decided) in the\n> > same category as \"refs/notes/*\".\n> > \n> > In the meantime, I'm unsure if it's a good idea to remove the\n> > limitation altogether (allowing notes refs everywhere), since\n> > re-introducing a limit at a later point will then be MUCH\n> > harder...\n> \n> I'm still unsure what that limitation brings to us. OTOH, it has at\n> least one funny downside: when someone tries to refer to some\n> forbidden ref using --ref, it gets silently requalified:\n> \n> $ git notes --ref=refs/remote-notes/foo add\n> $ find .git/refs/notes/ -type f\n> .git/refs/notes/refs/remote-notes/foo\n> $\n> \n> It just seems so wrong... Surely we can mitigate it by considering a\n> ref starting with \"refs/\" to be absolute, and thus never prepend\n> \"refs/notes/\" to it, but it rather sounds to me a symptom that we\n> may not want to filter things anyway.\n\nThe reason we put the limitation there, is to prevent the notes code \nfrom screwing with non-notes trees. The notes code reorganizes the notes \ntree depending on the number of tree entries, in order to achieve \nacceptable performance for notes trees of all sizes. Therefore, you \ndefinitely DON'T want the notes code rummaging around in non-notes trees \n(especially if some of your tree entries can be mistaken for strings of \nhex digits).\n\nThat said, the example you give above (\"refs/remote-notes/foo\" -> \n\"refs/notes/refs/remote-notes/foo\" is obviously a stupid failure, and \nshould be fixed. Considering \"refs/*\" to be absolute seems safe to me. \n(Obviously we loose the \"refs/notes/refs/*\" namespace, but I can live \nwith that.)\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170051","messageId":"450570199.1013051308131850322.JavaMail.root@zimbra44-e7.priv.proxad.net","threadId":"27611","inReplyTo":"243233943.1008861308129613120.JavaMail.root@zimbra44-e7.priv.proxad.net","subject":"Re: Commit notes workflow","fromName":"","fromEmail":"ydirson@free.fr","sentAt":"2011-06-15T09:57:30Z","receivedAt":"2011-06-15T09:57:30Z","isPatch":false,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"\n> I'm still unsure what that limitation brings to us. OTOH, it has at least one\n> funny downside: when someone tries to refer to some forbidden ref using --ref, it\n> gets silently requalified:\n> \n> $ git notes --ref=refs/remote-notes/foo add\n> $ find .git/refs/notes/ -type f\n> .git/refs/notes/refs/remote-notes/foo\n> $\n> \n> It just seems so wrong... Surely we can mitigate it by considering a ref starting\n> with \"refs/\" to be absolute, and thus never prepend \"refs/notes/\" to it, but it rather\n> sounds to me a symptom that we may not want to filter things anyway.\n\nWhile playing with this, I realized that when editing the template\ndoes not name the notes ref being edited. When looking at the code,\nI notice that, contrarily to commit.c which uses stdio, notes.c uses\nwrite_or_die(), which is a bit less flexible for formatting.\n\nI'd think we could me things more consistent - is there any objection\nto switch notes.c to using stdio for this ?\n"},{"id":"170054","messageId":"201106151253.57908.johan@herland.net","threadId":"27611","inReplyTo":"450570199.1013051308131850322.JavaMail.root@zimbra44-e7.priv.proxad.net","subject":"Re: Commit notes workflow","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-15T10:53:57Z","receivedAt":"2011-06-15T10:53:57Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wednesday 15. June 2011, ydirson@free.fr wrote:\n> > I'm still unsure what that limitation brings to us. OTOH, it has\n> > at least one funny downside: when someone tries to refer to some\n> > forbidden ref using --ref, it gets silently requalified:\n> > \n> > $ git notes --ref=refs/remote-notes/foo add\n> > $ find .git/refs/notes/ -type f\n> > .git/refs/notes/refs/remote-notes/foo\n> > $\n> > \n> > It just seems so wrong... Surely we can mitigate it by considering\n> > a ref starting with \"refs/\" to be absolute, and thus never prepend\n> > \"refs/notes/\" to it, but it rather sounds to me a symptom that we\n> > may not want to filter things anyway.\n> \n> While playing with this, I realized that when editing the template\n> does not name the notes ref being edited. When looking at the code,\n> I notice that, contrarily to commit.c which uses stdio, notes.c uses\n> write_or_die(), which is a bit less flexible for formatting.\n> \n> I'd think we could me things more consistent - is there any objection\n> to switch notes.c to using stdio for this ?\n\nGo ahead, Doing things in line with commit.c seems good to me.\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170213","messageId":"1308431208-13353-1-git-send-email-ydirson@free.fr","threadId":"27611","inReplyTo":"201106151253.57908.johan@herland.net","subject":"[PATCH 0/6] Small notes usability improvements","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-18T21:06:42Z","receivedAt":"2011-06-18T21:06:42Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"Patches 1 and 2 are just preparing things for patch 3.\n\nPatch 4 is a (hopefully) temporary measure, to be able to implement a\nreal-life until notes workflow without having to wait for\nrefs/remotes/ to get its due restructuring,\n\nPatch 5 addresses the anomaly reported earlier this week.\n\nPatch 6 is a proposal to make \"notes merge\" more similar to \"merge\"\n"},{"id":"170214","messageId":"1308431208-13353-2-git-send-email-ydirson@free.fr","threadId":"27611","inReplyTo":"1308431208-13353-1-git-send-email-ydirson@free.fr","subject":"[PATCH 1/6] Bring notes.c template handling in line with commit.c.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-18T21:06:43Z","receivedAt":"2011-06-18T21:06:43Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"Signed-off-by: Yann Dirson \n---\n builtin/notes.c | 30 +++++++++++++++---------------\n 1 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f8e437d..bd342ac 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -108,19 +108,19 @@ static int list_each_note(const unsigned char *object_sha1,\n \treturn 0;\n }\n \n-static void write_note_data(int fd, const unsigned char *sha1)\n+static void write_note_data(FILE *fp, const unsigned char *sha1)\n {\n \tunsigned long size;\n \tenum object_type type;\n \tchar *buf = read_sha1_file(sha1, &type, &size);\n \tif (buf) {\n \t\tif (size)\n-\t\t\twrite_or_die(fd, buf, size);\n+\t\t\tfwrite(buf, 1, size, fp);\n \t\tfree(buf);\n \t}\n }\n \n-static void write_commented_object(int fd, const unsigned char *object)\n+static void write_commented_object(FILE *fp, const unsigned char *object)\n {\n \tconst char *show_args[5] =\n \t\t{\"show\", \"--stat\", \"--no-notes\", sha1_to_hex(object), NULL};\n@@ -144,11 +144,11 @@ static void write_commented_object(int fd, const unsigned char *object)\n \tif (show_out == NULL)\n \t\tdie_errno(_(\"can't fdopen 'show' output fd\"));\n \n-\t/* Prepend \"# \" to each output line and write result to 'fd' */\n+\t/* Prepend \"# \" to each output line and write result to 'fp' */\n \twhile (strbuf_getline(&buf, show_out, '\\n') != EOF) {\n-\t\twrite_or_die(fd, \"# \", 2);\n-\t\twrite_or_die(fd, buf.buf, buf.len);\n-\t\twrite_or_die(fd, \"\\n\", 1);\n+\t\tfwrite(\"# \", 1, 2, fp);\n+\t\tfwrite(buf.buf, 1, buf.len, fp);\n+\t\tfwrite(\"\\n\", 1, 1, fp);\n \t}\n \tstrbuf_release(&buf);\n \tif (fclose(show_out))\n@@ -166,23 +166,23 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n \tchar *path = NULL;\n \n \tif (msg->use_editor || !msg->given) {\n-\t\tint fd;\n+\t\tFILE *fp;\n \n \t\t/* write the template message before editing: */\n \t\tpath = git_pathdup(\"NOTES_EDITMSG\");\n-\t\tfd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n-\t\tif (fd < 0)\n+\t\tfp = fopen(path, \"w\");\n+\t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not create file '%s'\"), path);\n \n \t\tif (msg->given)\n-\t\t\twrite_or_die(fd, msg->buf.buf, msg->buf.len);\n+\t\t\tfwrite(msg->buf.buf, 1, msg->buf.len, fp);\n \t\telse if (prev && !append_only)\n-\t\t\twrite_note_data(fd, prev);\n-\t\twrite_or_die(fd, note_template, strlen(note_template));\n+\t\t\twrite_note_data(fp, prev);\n+\t\tfwrite(note_template, 1, strlen(note_template), fp);\n \n-\t\twrite_commented_object(fd, object);\n+\t\twrite_commented_object(fp, object);\n \n-\t\tclose(fd);\n+\t\tfclose(fp);\n \t\tstrbuf_reset(&(msg->buf));\n \n \t\tif (launch_editor(path, &(msg->buf), NULL)) {\n-- \n1.7.5.3\n"},{"id":"170217","messageId":"1308431208-13353-3-git-send-email-ydirson@free.fr","threadId":"27611","inReplyTo":"1308431208-13353-1-git-send-email-ydirson@free.fr","subject":"[PATCH 2/6] Factorize shortening of notes refname for display.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-18T21:06:44Z","receivedAt":"2011-06-18T21:06:44Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"Signed-off-by: Yann Dirson \n---\n notes.c | 24 ++++++++++++++++--------\n notes.h | 7 +++++++\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex f6ce848..1a5676a 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1235,16 +1235,11 @@ void format_note(struct notes_tree *t, const unsigned char *object_sha1,\n \t\tmsglen--;\n \n \tif (flags & NOTES_SHOW_HEADER) {\n-\t\tconst char *ref = t->ref;\n-\t\tif (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {\n+\t\tconst char *ref = notes_ref_shortname(t->ref);\n+\t\tif (!ref)\n \t\t\tstrbuf_addstr(sb, \"\\nNotes:\\n\");\n-\t\t} else {\n-\t\t\tif (!prefixcmp(ref, \"refs/\"))\n-\t\t\t\tref += 5;\n-\t\t\tif (!prefixcmp(ref, \"notes/\"))\n-\t\t\t\tref += 6;\n+\t\telse\n \t\t\tstrbuf_addf(sb, \"\\nNotes (%s):\\n\", ref);\n-\t\t}\n \t}\n \n \tfor (msg_p = msg; msg_p < msg + msglen; msg_p += linelen + 1) {\n@@ -1296,3 +1291,16 @@ void expand_notes_ref(struct strbuf *sb)\n \telse\n \t\tstrbuf_insert(sb, 0, \"refs/notes/\", 11);\n }\n+\n+const char *notes_ref_shortname(const char *ref)\n+{\n+\tif (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF))\n+\t\treturn NULL;\n+\telse {\n+\t\tif (!prefixcmp(ref, \"refs/\"))\n+\t\t\tref += 5;\n+\t\tif (!prefixcmp(ref, \"notes/\"))\n+\t\t\tref += 6;\n+\t\treturn ref;\n+\t}\n+}\ndiff --git a/notes.h b/notes.h\nindex c716694..d8ae29d 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -64,6 +64,13 @@ extern struct notes_tree {\n const char *default_notes_ref(void);\n \n /*\n+ * Return a short name for a notes ref, suitable for display to the user.\n+ *\n+ * No copy is done, the return value is a pointer into the original string.\n+ */\n+const char *notes_ref_shortname(const char *ref);\n+\n+/*\n * Flags controlling behaviour of notes tree initialization\n *\n * Default behaviour is to initialize the notes tree from the tree object\n-- \n1.7.5.3\n"},{"id":"170215","messageId":"1308431208-13353-4-git-send-email-ydirson@free.fr","threadId":"27611","inReplyTo":"1308431208-13353-1-git-send-email-ydirson@free.fr","subject":"[PATCH 3/6] Include name of notes ref in template when creating/editing notes.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-18T21:06:45Z","receivedAt":"2011-06-18T21:06:45Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"This will still show for the default \"commits\" notes:\n\n # Write/edit notes for the following object:\n\nFor other notes refs it will show:\n\n # Write/edit \"foo\" notes for the following object:\n\nSigned-off-by: Yann Dirson \n---\n builtin/notes.c | 8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex bd342ac..ae89d38 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -91,7 +91,7 @@ static const char * const git_notes_get_ref_usage[] = {\n static const char note_template[] =\n \t\"\\n\"\n \t\"#\\n\"\n-\t\"# Write/edit the notes for the following object:\\n\"\n+\t\"# Write/edit %s%s%snotes for the following object:\\n\"\n \t\"#\\n\";\n \n struct msg_arg {\n@@ -167,6 +167,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n \n \tif (msg->use_editor || !msg->given) {\n \t\tFILE *fp;\n+\t\tconst char *ref = notes_ref_shortname(default_notes_tree.ref);\n \n \t\t/* write the template message before editing: */\n \t\tpath = git_pathdup(\"NOTES_EDITMSG\");\n@@ -178,7 +179,10 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n \t\t\tfwrite(msg->buf.buf, 1, msg->buf.len, fp);\n \t\telse if (prev && !append_only)\n \t\t\twrite_note_data(fp, prev);\n-\t\tfwrite(note_template, 1, strlen(note_template), fp);\n+\t\tif (!ref)\n+\t\t\tfprintf(fp, note_template, \"\", \"\", \"\");\n+\t\telse\n+\t\t\tfprintf(fp, note_template, \"\\\"\", ref, \"\\\" \");\n \n \t\twrite_commented_object(fp, object);\n \n-- \n1.7.5.3\n"},{"id":"170216","messageId":"1308431208-13353-5-git-send-email-ydirson@free.fr","threadId":"27611","inReplyTo":"1308431208-13353-1-git-send-email-ydirson@free.fr","subject":"[PATCH 4/6] Allow \"git notes merge\" to use refs/remote-notes/ as a source.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-18T21:06:46Z","receivedAt":"2011-06-18T21:06:46Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"Signed-off-by: Yann Dirson \n---\n Documentation/git-notes.txt | 5 +++++\n builtin/notes.c | 4 ++--\n notes.c | 5 +++--\n notes.h | 2 +-\n revision.c | 2 +-\n 5 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex 6a187f2..7ce8a24 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -104,6 +104,11 @@ and instructs the user to manually resolve the conflicts there.\n When done, the user can either finalize the merge with\n 'git notes merge --commit', or abort the merge with\n 'git notes merge --abort'.\n++\n+In addition to `refs/notes/`, the remote notes ref is accepted\n+from the `refs/remote-notes/` namespace. This is intended to\n+provide notes with support for a workflow similar to the one used\n+for heads references.\n \n remove::\n \tRemove the notes for given objects (defaults to HEAD). When\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex ae89d38..6bff44f 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -905,7 +905,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \n \to.local_ref = default_notes_ref();\n \tstrbuf_addstr(&remote_ref, argv[0]);\n-\texpand_notes_ref(&remote_ref);\n+\texpand_notes_ref(&remote_ref, 1);\n \to.remote_ref = remote_ref.buf;\n \n \tif (strategy) {\n@@ -1075,7 +1075,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)\n \tif (override_notes_ref) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, override_notes_ref);\n-\t\texpand_notes_ref(&sb);\n+\t\texpand_notes_ref(&sb, 0);\n \t\tsetenv(\"GIT_NOTES_REF\", sb.buf, 1);\n \t\tstrbuf_release(&sb);\n \t}\ndiff --git a/notes.c b/notes.c\nindex 1a5676a..12afc02 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1282,9 +1282,10 @@ int copy_note(struct notes_tree *t,\n \treturn 0;\n }\n \n-void expand_notes_ref(struct strbuf *sb)\n+void expand_notes_ref(struct strbuf *sb, int allow_remotes)\n {\n-\tif (!prefixcmp(sb->buf, \"refs/notes/\"))\n+\tif (!prefixcmp(sb->buf, \"refs/notes/\") ||\n+\t (allow_remotes && !prefixcmp(sb->buf, \"refs/remote-notes/\")))\n \t\treturn; /* we're happy */\n \telse if (!prefixcmp(sb->buf, \"notes/\"))\n \t\tstrbuf_insert(sb, 0, \"refs/\", 5);\ndiff --git a/notes.h b/notes.h\nindex d8ae29d..80219ec 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -317,6 +317,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,\n \t\t\t\t\t const char *globs);\n \n /* Expand inplace a note ref like \"foo\" or \"notes/foo\" into \"refs/notes/foo\" */\n-void expand_notes_ref(struct strbuf *sb);\n+void expand_notes_ref(struct strbuf *sb, int allow_remotes);\n \n #endif\ndiff --git a/revision.c b/revision.c\nindex c46cfaa..b482314 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1393,7 +1393,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t}\n \t\telse\n \t\t\tstrbuf_addstr(&buf, arg+8);\n-\t\texpand_notes_ref(&buf);\n+\t\texpand_notes_ref(&buf, 1);\n \t\tstring_list_append(&revs->notes_opt.extra_notes_refs,\n \t\t\t\t strbuf_detach(&buf, NULL));\n \t} else if (!strcmp(arg, \"--no-notes\")) {\n-- \n1.7.5.3\n"},{"id":"170218","messageId":"1308431208-13353-6-git-send-email-ydirson@free.fr","threadId":"27611","inReplyTo":"1308431208-13353-1-git-send-email-ydirson@free.fr","subject":"[PATCH 5/6] Assume a note ref starting with refs must not be prepended refs/notes/.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-18T21:06:47Z","receivedAt":"2011-06-18T21:06:47Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"This caused strange behaviour when \"git notes\" was asked to manipulate\nrefs/ outside of refs/notes/: it was attempting to use\nrefs/notes/refs/.\n\nDying early this way should avoid the need to check for the\nrefs/notes/ prefix in several places.\n\nSigned-off-by: Yann Dirson \n---\n notes.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 12afc02..c6a82da 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1289,6 +1289,8 @@ void expand_notes_ref(struct strbuf *sb, int allow_remotes)\n \t\treturn; /* we're happy */\n \telse if (!prefixcmp(sb->buf, \"notes/\"))\n \t\tstrbuf_insert(sb, 0, \"refs/\", 5);\n+\telse if (!prefixcmp(sb->buf, \"refs/\"))\n+\t\tdie(_(\"Not a notes reference: %s\"), sb->buf);\n \telse\n \t\tstrbuf_insert(sb, 0, \"refs/notes/\", 11);\n }\n-- \n1.7.5.3\n"},{"id":"170219","messageId":"1308431208-13353-7-git-send-email-ydirson@free.fr","threadId":"27611","inReplyTo":"1308431208-13353-1-git-send-email-ydirson@free.fr","subject":"[PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-18T21:06:48Z","receivedAt":"2011-06-18T21:06:48Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"This causes the \"merge empty notes ref (z => y)\" test in t3308-notes-merge.sh\nto fail - obviously, it is removing the functionnality that is tested for.\n\nIs there any real use for this ? It just seems so different from\n\"git merge\", which errors out in the similar situation:\n\n$ git merge foo\nfatal: 'foo' does not point to a commit\n\nSigned-off-by: Yann Dirson \n---\n builtin/notes.c | 3 +++\n t/t3308-notes-merge.sh | 6 ------\n 2 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 6bff44f..058b14d 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -908,6 +908,9 @@ static int merge(int argc, const char **argv, const char *prefix)\n \texpand_notes_ref(&remote_ref, 1);\n \to.remote_ref = remote_ref.buf;\n \n+\tif (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))\n+\t\tdie(\"'%s' does not point to a commit\", o.remote_ref);\n+\n \tif (strategy) {\n \t\tif (!strcmp(strategy, \"manual\"))\n \t\t\to.strategy = NOTES_MERGE_RESOLVE_MANUAL;\ndiff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh\nindex 24d82b4..2dcc1db 100755\n--- a/t/t3308-notes-merge.sh\n+++ b/t/t3308-notes-merge.sh\n@@ -104,12 +104,6 @@ test_expect_success 'merge notes into empty notes ref (x => y)' '\n \ttest \"$(git rev-parse refs/notes/x)\" = \"$(git rev-parse refs/notes/y)\"\n '\n \n-test_expect_success 'merge empty notes ref (z => y)' '\n-\tgit notes merge z &&\n-\t# y should not change (still == x)\n-\ttest \"$(git rev-parse refs/notes/x)\" = \"$(git rev-parse refs/notes/y)\"\n-'\n-\n test_expect_success 'change notes on other notes ref (y)' '\n \t# Not touching notes to 1st commit\n \tgit notes remove 2nd &&\n-- \n1.7.5.3\n"},{"id":"170259","messageId":"201106192323.09511.johan@herland.net","threadId":"27611","inReplyTo":"1308431208-13353-2-git-send-email-ydirson@free.fr","subject":"Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-19T21:23:09Z","receivedAt":"2011-06-19T21:23:09Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 18 June 2011, Yann Dirson wrote:\n> Signed-off-by: Yann Dirson \n\nPlease mention in the commit message that the commit merely replaces \nwrite_or_die()/int fd with the corresponding stdio functionality, and that \nthere is no (intended) change in behavior. It was not apparent from your \ncommit message that you had not made any other changes.\n\nOtherwise the patch looks OK.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170260","messageId":"201106192325.00667.johan@herland.net","threadId":"27611","inReplyTo":"1308431208-13353-3-git-send-email-ydirson@free.fr","subject":"Re: [PATCH 2/6] Factorize shortening of notes refname for display.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-19T21:25:00Z","receivedAt":"2011-06-19T21:25:00Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 18 June 2011, Yann Dirson wrote:\n> Signed-off-by: Yann Dirson \n> ---\n> notes.c | 24 ++++++++++++++++--------\n> notes.h | 7 +++++++\n> 2 files changed, 23 insertions(+), 8 deletions(-)\n> \n> [...]\n> \n> /*\n> + * Return a short name for a notes ref, suitable for display to the user.\n> + *\n> + * No copy is done, the return value is a pointer into the original string.\n> + */\n> +const char *notes_ref_shortname(const char *ref);\n> +\n> +/*\n\nPlease include in the documentation what a NULL return means.\n\nOtherwise the patch looks OK.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170261","messageId":"201106192345.14879.johan@herland.net","threadId":"27611","inReplyTo":"1308431208-13353-5-git-send-email-ydirson@free.fr","subject":"Re: [PATCH 4/6] Allow \"git notes merge\" to use refs/remote-notes/ as a source.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-19T21:45:14Z","receivedAt":"2011-06-19T21:45:14Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 18 June 2011, Yann Dirson wrote:\n> Signed-off-by: Yann Dirson \n> ---\n> Documentation/git-notes.txt | 5 +++++\n> builtin/notes.c | 4 ++--\n> notes.c | 5 +++--\n> notes.h | 2 +-\n> revision.c | 2 +-\n> 5 files changed, 12 insertions(+), 6 deletions(-)\n> \n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index 6a187f2..7ce8a24 100644\n> --- a/Documentation/git-notes.txt\n> +++ b/Documentation/git-notes.txt\n> @@ -104,6 +104,11 @@ and instructs the user to manually resolve the\n> conflicts there. When done, the user can either finalize the merge with\n> 'git notes merge --commit', or abort the merge with\n> 'git notes merge --abort'.\n> ++\n> +In addition to `refs/notes/`, the remote notes ref is accepted\n> +from the `refs/remote-notes/` namespace. This is intended to\n> +provide notes with support for a workflow similar to the one used\n> +for heads references.\n\nI would rephrase this as:\n\n In addition to `refs/notes/*`, the remote notes ref can also be\n from within `refs/remote-notes/*`. This allows the user to set up\n fetch refspecs that transfers notes refs from a remote repo into\n `refs/remote-notes/*`, and then merge those remote notes refs into\n the corresponding local notes refs.\n\nAlso, AFAICS you're adding the possibility to read notes from refs/remote-\nnotes/*, but not WRITE to those notes using \"git notes\" (obviously, \"git \nfetch\" and other tools can be used to manipulate them). Please add some \nselftests verifying that \"git notes\" is still unable to manipulate notes in \nrefs/remote-notes/*.\n\nOtherwise the patch looks good to me.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170263","messageId":"201106200003.46490.johan@herland.net","threadId":"27611","inReplyTo":"1308431208-13353-7-git-send-email-ydirson@free.fr","subject":"Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-19T22:03:46Z","receivedAt":"2011-06-19T22:03:46Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 18 June 2011, Yann Dirson wrote:\n> This causes the \"merge empty notes ref (z => y)\" test in\n> t3308-notes-merge.sh to fail - obviously, it is removing the\n> functionnality that is tested for.\n> \n> Is there any real use for this ? It just seems so different from\n> \"git merge\", which errors out in the similar situation:\n> \n> $ git merge foo\n> fatal: 'foo' does not point to a commit\n\nI understand your reasoning, and I don't have a problem with changing this \nbehavior to be in line with \"git merge\".\n\n> Signed-off-by: Yann Dirson \n> ---\n> builtin/notes.c | 3 +++\n> t/t3308-notes-merge.sh | 6 ------\n> 2 files changed, 3 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 6bff44f..058b14d 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -908,6 +908,9 @@ static int merge(int argc, const char **argv, const\n> char *prefix) expand_notes_ref(&remote_ref, 1);\n> \to.remote_ref = remote_ref.buf;\n> \n> +\tif (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))\n> +\t\tdie(\"'%s' does not point to a commit\", o.remote_ref);\n\nHmm. I'm not sure requiring the remote ref to always point to a _commit_ is \nthe right solution here. In previous discussions on the notes topic, some \npeople (Peff?) expressed a need/interest for history-less notes refs (i.e. a \nnotes tree where we don't keep track of its development, but only refer to \nthe latest/current version). Obviously, there are two ways to implement \nhistory-less notes refs: (a) making the notes ref point to a notes commit \nwithout any parents (i.e. each notes commit is a root commit), or (b) making \nthe notes ref point directly at the notes _tree_ object (i.e. no commit \nobject at all).\n\nI can't remember off the top of my head whether our earlier discussions on \nthis topic resulted in us excluding support for option (b), but if we \ndidn't, it should be possible to merge notes refs where one or both refs \npoint directly at a tree object, and your above line would break this.\n\n> [...]\n> \n> diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh\n> index 24d82b4..2dcc1db 100755\n> --- a/t/t3308-notes-merge.sh\n> +++ b/t/t3308-notes-merge.sh\n> @@ -104,12 +104,6 @@ test_expect_success 'merge notes into empty notes\n> ref (x => y)' ' test \"$(git rev-parse refs/notes/x)\" = \"$(git rev-parse\n> refs/notes/y)\" '\n> \n> -test_expect_success 'merge empty notes ref (z => y)' '\n> -\tgit notes merge z &&\n> -\t# y should not change (still == x)\n> -\ttest \"$(git rev-parse refs/notes/x)\" = \"$(git rev-parse refs/notes/y)\"\n> -'\n\nInstead of removing the test, please change it into verifying the _new_ \nexpected behavior (that we fail with an appropriate error message when asked \nto merge a non-existent notes ref).\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170262","messageId":"201106200006.03945.johan@herland.net","threadId":"27611","inReplyTo":"1308431208-13353-1-git-send-email-ydirson@free.fr","subject":"Re: [PATCH 0/6] Small notes usability improvements","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-19T22:06:03Z","receivedAt":"2011-06-19T22:06:03Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 18 June 2011, Yann Dirson wrote:\n> Patches 1 and 2 are just preparing things for patch 3.\n> \n> Patch 4 is a (hopefully) temporary measure, to be able to implement a\n> real-life until notes workflow without having to wait for\n> refs/remotes/ to get its due restructuring,\n> \n> Patch 5 addresses the anomaly reported earlier this week.\n> \n> Patch 6 is a proposal to make \"notes merge\" more similar to \"merge\"\n\nThis series looks good to me, unless otherwise noted in my replies to \nindividual patches.\n\n\nThanks for your work!\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170266","messageId":"7vpqm9e8rn.fsf@alter.siamese.dyndns.org","threadId":"27611","inReplyTo":"201106192323.09511.johan@herland.net","subject":"Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-19T22:50:04Z","receivedAt":"2011-06-19T22:50:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Saturday 18 June 2011, Yann Dirson wrote:\n>> Signed-off-by: Yann Dirson \n>\n> Please mention in the commit message that the commit merely replaces \n> write_or_die()/int fd with the corresponding stdio functionality, and that \n> there is no (intended) change in behavior. It was not apparent from your \n> commit message that you had not made any other changes.\n>\n> Otherwise the patch looks OK.\n\nI had an impression that you would lose a lot of error checking, unless\nyou are careful, if you go from write_or_die() to stdio.\n"},{"id":"170267","messageId":"7vliwxe8p5.fsf@alter.siamese.dyndns.org","threadId":"27611","inReplyTo":"201106192325.00667.johan@herland.net","subject":"Re: [PATCH 2/6] Factorize shortening of notes refname for display.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-19T22:51:34Z","receivedAt":"2011-06-19T22:51:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Saturday 18 June 2011, Yann Dirson wrote:\n>> Signed-off-by: Yann Dirson \n>> ---\n>> notes.c | 24 ++++++++++++++++--------\n>> notes.h | 7 +++++++\n>> 2 files changed, 23 insertions(+), 8 deletions(-)\n>> \n>> [...]\n>> \n>> /*\n>> + * Return a short name for a notes ref, suitable for display to the user.\n>> + *\n>> + * No copy is done, the return value is a pointer into the original string.\n>> + */\n>> +const char *notes_ref_shortname(const char *ref);\n>> +\n>> +/*\n>\n> Please include in the documentation what a NULL return means.\n>\n> Otherwise the patch looks OK.\n\nIt may be just me, but every time somebody says Factorize, I find myself\nlooking for math textbook from middle school days, 12 = 2 * 2 * 3, etc.\n\nCould we say Refactor instead pretty please?\n"},{"id":"170282","messageId":"20110620071607.GB15246@sigill.intra.peff.net","threadId":"27611","inReplyTo":"201106200003.46490.johan@herland.net","subject":"Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-20T07:16:07Z","receivedAt":"2011-06-20T07:16:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 20, 2011 at 12:03:46AM +0200, Johan Herland wrote:\n\n> > +\tif (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))\n> > +\t\tdie(\"'%s' does not point to a commit\", o.remote_ref);\n> \n> Hmm. I'm not sure requiring the remote ref to always point to a _commit_ is \n> the right solution here. In previous discussions on the notes topic, some \n> people (Peff?) expressed a need/interest for history-less notes refs (i.e. a \n> notes tree where we don't keep track of its development, but only refer to \n> the latest/current version). Obviously, there are two ways to implement \n> history-less notes refs: (a) making the notes ref point to a notes commit \n> without any parents (i.e. each notes commit is a root commit), or (b) making \n> the notes ref point directly at the notes _tree_ object (i.e. no commit \n> object at all).\n> \n> I can't remember off the top of my head whether our earlier discussions on \n> this topic resulted in us excluding support for option (b), but if we \n> didn't, it should be possible to merge notes refs where one or both refs \n> point directly at a tree object, and your above line would break this.\n\nThe notes-cache.[ch] implementation uses history-less notes for textconv\ncaching. Since it's just a cache, we don't care about history or\nmerging. And keeping a history would just mean useless old versions of\nthe cache are kept longer than necessary.\n\nI ended up using a commit with no parents to store the cache. I don't\nrecall offhand whether there were any complications with using a raw\ntree, but I realized that I needed some place to put extra metadata like\nthe cache validity. Wrapping the tree object in a commit provided that\nplace.\n\nI don't think there is any real reason for somebody to need a bare tree\nof notes. There is a certain elegance that refs can point directly to\ntrees in git, but the overhead of a single commit object to wrap it is\njust not a big deal[1].\n\nI didn't test, but I doubt that \"git merge\" will handle bare trees; this\nwould provide analagous behavior for notes-merging. But maybe I'm\nwrong.\n\n-Peff\n\n[1] The only other time I recall seeing a bare tree is linux-2.6's\nv2.6.11 tag. And even there it is wrapped by a tag object, so that Linus\ncould include metadata (a comment and a GPG signature). There's really\nno reason that couldn't have had a commit, except that doing it as a\ntree shows off how cool git is. :)\n"},{"id":"170283","messageId":"201106200929.53566.johan@herland.net","threadId":"27611","inReplyTo":"20110620071607.GB15246@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-20T07:29:53Z","receivedAt":"2011-06-20T07:29:53Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 20 June 2011, Jeff King wrote:\n> On Mon, Jun 20, 2011 at 12:03:46AM +0200, Johan Herland wrote:\n> > > +\tif (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))\n> > > +\t\tdie(\"'%s' does not point to a commit\", o.remote_ref);\n> > \n> > Hmm. I'm not sure requiring the remote ref to always point to a\n> > _commit_ is the right solution here. In previous discussions on the\n> > notes topic, some people (Peff?) expressed a need/interest for\n> > history-less notes refs (i.e. a notes tree where we don't keep track\n> > of its development, but only refer to the latest/current version).\n> > Obviously, there are two ways to implement history-less notes refs:\n> > (a) making the notes ref point to a notes commit without any parents\n> > (i.e. each notes commit is a root commit), or (b) making the notes ref\n> > point directly at the notes _tree_ object (i.e. no commit object at\n> > all).\n> > \n> > I can't remember off the top of my head whether our earlier discussions\n> > on this topic resulted in us excluding support for option (b), but if\n> > we didn't, it should be possible to merge notes refs where one or both\n> > refs point directly at a tree object, and your above line would break\n> > this.\n> \n> [...]\n> \n> I don't think there is any real reason for somebody to need a bare tree\n> of notes. There is a certain elegance that refs can point directly to\n> trees in git, but the overhead of a single commit object to wrap it is\n> just not a big deal.\n> \n> I didn't test, but I doubt that \"git merge\" will handle bare trees; this\n> would provide analagous behavior for notes-merging. But maybe I'm\n> wrong.\n\nYou're not wrong. \"git merge\" when trying to merge a tree object:\n\n $ git merge ee314a3\n error: ee314a3: expected commit type, but the object dereferences to tree type\n fatal: 'ee314a3' does not point to a commit\n\nSo I guess there's no reason to allow notes trees with no commit object.\n\nYann: Please disregard my complaint on the above two lines.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170285","messageId":"201106200941.54883.johan@herland.net","threadId":"27611","inReplyTo":"7vpqm9e8rn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-06-20T07:41:54Z","receivedAt":"2011-06-20T07:41:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 20 June 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > On Saturday 18 June 2011, Yann Dirson wrote:\n> >> Signed-off-by: Yann Dirson \n> > \n> > Please mention in the commit message that the commit merely replaces\n> > write_or_die()/int fd with the corresponding stdio functionality, and\n> > that there is no (intended) change in behavior. It was not apparent\n> > from your commit message that you had not made any other changes.\n> > \n> > Otherwise the patch looks OK.\n> \n> I had an impression that you would lose a lot of error checking, unless\n> you are careful, if you go from write_or_die() to stdio.\n\nYeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one \nneeds to check the return value, and handle errors accordingly.\n\nAn alternative solution would be to drop this patch, and instead use \nstrbuf_addf() to get the format printing functionality needed in PATCH 3/6.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"170313","messageId":"20110620184842.GN2921@home.lan","threadId":"27611","inReplyTo":"201106200941.54883.johan@herland.net","subject":"Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-20T18:48:42Z","receivedAt":"2011-06-20T18:48:42Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:\n> On Monday 20 June 2011, Junio C Hamano wrote:\n> > Johan Herland writes:\n> > > On Saturday 18 June 2011, Yann Dirson wrote:\n> > >> Signed-off-by: Yann Dirson \n> > > \n> > > Please mention in the commit message that the commit merely replaces\n> > > write_or_die()/int fd with the corresponding stdio functionality, and\n> > > that there is no (intended) change in behavior. It was not apparent\n> > > from your commit message that you had not made any other changes.\n> > > \n> > > Otherwise the patch looks OK.\n> > \n> > I had an impression that you would lose a lot of error checking, unless\n> > you are careful, if you go from write_or_die() to stdio.\n> \n> Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one \n> needs to check the return value, and handle errors accordingly.\n\nIt appears I based my code on buildin/commit.c from 1.7.4.1 - I just\ndid not realize that this part changed much in between with 098d0e0e.\nI'll look into that.\n\n> An alternative solution would be to drop this patch, and instead use \n> strbuf_addf() to get the format printing functionality needed in PATCH 3/6.\n\nI have thought about that, but that will make the i18n process for the\ntemplate much more awkward - and we probably don't want to reimplement\nstdio formatting for strbuf.\n\n-- \nYann.\n"},{"id":"170314","messageId":"20110620184953.GO2921@home.lan","threadId":"27611","inReplyTo":"7vliwxe8p5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/6] Factorize shortening of notes refname for display.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-20T18:49:53Z","receivedAt":"2011-06-20T18:49:53Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Sun, Jun 19, 2011 at 03:51:34PM -0700, Junio C Hamano wrote:\n> Johan Herland writes:\n> \n> > On Saturday 18 June 2011, Yann Dirson wrote:\n> >> Signed-off-by: Yann Dirson \n> >> ---\n> >> notes.c | 24 ++++++++++++++++--------\n> >> notes.h | 7 +++++++\n> >> 2 files changed, 23 insertions(+), 8 deletions(-)\n> >> \n> >> [...]\n> >> \n> >> /*\n> >> + * Return a short name for a notes ref, suitable for display to the user.\n> >> + *\n> >> + * No copy is done, the return value is a pointer into the original string.\n> >> + */\n> >> +const char *notes_ref_shortname(const char *ref);\n> >> +\n> >> +/*\n> >\n> > Please include in the documentation what a NULL return means.\n> >\n> > Otherwise the patch looks OK.\n> \n> It may be just me, but every time somebody says Factorize, I find myself\n> looking for math textbook from middle school days, 12 = 2 * 2 * 3, etc.\n> \n> Could we say Refactor instead pretty please?\n\nWell, factorizing is just one kind of code refactoring - but OK, I'll\ndo that ;)\n"},{"id":"170417","messageId":"20110621193952.GP2921@home.lan","threadId":"27611","inReplyTo":"20110620184842.GN2921@home.lan","subject":"Re: [PATCH 1/6] Bring notes.c template handling in line with commit.c.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2011-06-21T19:39:52Z","receivedAt":"2011-06-21T19:39:52Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Jun 20, 2011 at 08:48:42PM +0200, Yann Dirson wrote:\n> On Mon, Jun 20, 2011 at 09:41:54AM +0200, Johan Herland wrote:\n> > On Monday 20 June 2011, Junio C Hamano wrote:\n> > > Johan Herland writes:\n> > > > On Saturday 18 June 2011, Yann Dirson wrote:\n> > > >> Signed-off-by: Yann Dirson \n> > > > \n> > > > Please mention in the commit message that the commit merely replaces\n> > > > write_or_die()/int fd with the corresponding stdio functionality, and\n> > > > that there is no (intended) change in behavior. It was not apparent\n> > > > from your commit message that you had not made any other changes.\n> > > > \n> > > > Otherwise the patch looks OK.\n> > > \n> > > I had an impression that you would lose a lot of error checking, unless\n> > > you are careful, if you go from write_or_die() to stdio.\n> > \n> > Yeah, write_or_die() dies on failure, while with fwrite/fprintf I guess one \n> > needs to check the return value, and handle errors accordingly.\n> \n> It appears I based my code on buildin/commit.c from 1.7.4.1 - I just\n> did not realize that this part changed much in between with 098d0e0e.\n> I'll look into that.\n\nHm. So now builtin/commit.c heavily relies on status_printf_*. Those\ndo not do much more return-checking on fprintf() than the previous\ncode - but at least they provide a single point where such\nreturn-checking can be inserted, which is already better.\n\nNow, those require a wt_status struct... but AFAICT, it only uses the\nFILE* inside. This seems a bit annoying for the purpose of reusing\nthe #-prefixing and line-folding mechanism in builtin/notes.c. Would\nreplacing those funcs with FILE*-based ones - let's say,\nstatus_printf_*_fp() - and wrappers with the current names in\nwt-status.h be seen as a good idea ?\n\n\n> > An alternative solution would be to drop this patch, and instead use \n> > strbuf_addf() to get the format printing functionality needed in PATCH 3/6.\n> \n> I have thought about that, but that will make the i18n process for the\n> template much more awkward - and we probably don't want to reimplement\n> stdio formatting for strbuf.\n\n... and for that matter, it looks like status_printf* provide us with\nall that's needed.\n\n-- \nYann\n"}]}