{"thread":{"id":"26601","subject":"[RFC/PATCH] commit notes workflow","startedAt":"2011-02-25T13:30:57Z","lastAt":"2011-03-09T08:13:07Z","messageCount":27,"participants":["Jeff King","Johan Herland","Junio C Hamano","Drew Northup","Michael J Gruber","Chris Packham","Piotr Krukowiecki","Sverre Rabbelier","Ian Ward Comfort","Michel Lespinasse","Yann Dirson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"162236","messageId":"20110225133056.GA1026@sigill.intra.peff.net","threadId":"26601","inReplyTo":null,"subject":"[RFC/PATCH] commit notes workflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-25T13:30:57Z","receivedAt":"2011-02-25T13:30:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was revising a long-ish series today, and I have been wanting to start\nusing \"git notes\" to store information on what's changed between\nversions (which will eventually go after the \"---\" in format-patch).\n\nSo my workflow was something like:\n\n 1. git rebase -i, mark one or more commits for edit\n\n 2. For each commit we stop at:\n\n a. Tweak the tree either with enhancements, or to resolve\n conflicts from tweaks to earlier patches.\n\n b. commit --amend, tweak commit message is needed\n\n c. git notes add, mention changes\n\n d. git rebase --continue\n\nTwo things annoyed me:\n\n 1. Editing the commit message and notes separately felt awkward. They\n are conceptually part of the same task to me.\n\n 2. In the conflict case, there is no opportunity to run \"git notes\n add\" because you fix up commits and directly run \"rebase\n --continue\".\n\nSo my solution was that \"git commit\" should be able to embed and extract\nnotes from the commit message itself. The patch below implements \"git\ncommit --notes\", which does two things:\n\n 1. If we are amending, it populates the commit message not just with\n the existing message, but also with a \"---\" divider and any notes on\n the commit.\n\n 2. After editing the commit message, it looks for the \"---\" divider\n and puts everything after it into a commit note (whether or not it\n put in a divider in step (1), so you can add new notes, too).\n\nSo your commit template looks like:\n\n subject\n\n commit message body\n ---\n notes data\n\n # usual template stuff\n\nI'm curious what people think. Do others find this useful? Does it seem\nharmful?\n\nIt's yet another magic format to worry about when writing a commit\nmessage. But you don't need to care unless you use \"--notes\" (and I\nwould probably add a config option, since I would always want this on\npersonally). And \"---\" is already something to be aware of, since \"am\"\ntreats it specially (technically, I could just drop \"notes\" entirely and\nuse \"---\" in my commit message; so perhaps this is just\noverengineering).\n\nMy initial attempt was to implement in terms of prepare-commit-msg,\ncommit-msg, and post-commit hooks. And it worked OK, but was foiled by\nrebase using \"git commit --no-verify\".\n\nThere are still a few questions to address.\n\nHow should this interact with --cleanup? Right now it splits everything\nafter the \"---\" into the notes part, including any \"#\" lines. Which\nshould be fine, I think, because they get pulled out by stripspace\nin either case. If you were using --cleanup=verbatim, then you'd have\ngotten rid of them manually anyway. And if you really want a literal\n\"---\", you would use \"git commit\" (or \"git commit --no-notes\" once there\nis a config option). So I think the behavior in this patch is sane.\n\nI only turn on --edit when we launch an editor. It seems somehow more\nconfusing to me that \"git commit -F file\" should split notes out (or\nworse, \"git commit -m\"). If you are doing things non-interactively, it's\nprobably not a big deal to just call \"git notes add\" separately. And I\nexpect \"-F\" is used by porcelains, or people wanting to do verbatim\nstuff.\n\nHow should this interact with the commit-msg hook? In my implementation,\nit sees the whole thing, message and notes. Should we be picking apart\nthe two bits after the editor and rewriting the COMMIT_EDITMSG before\nthe hook sees it?\n\nHow should this interact with the post-rewrite hook? I obviously need to\nset that up for my workflow, too, but I haven't yet. This patch does\nnothing, but I'm pretty sure it should turn of \"git commit --amend\"\ncalling the rewrite hook if we are using --notes (since the user has\nalready seen and edited the notes, and we've written them out).\n\nAnyway, here is the patch.\n\n---\n builtin/commit.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 64 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5295032..3e21e33 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -26,6 +26,7 @@\n #include \"unpack-trees.h\"\n #include \"quote.h\"\n #include \"submodule.h\"\n+#include \"blob.h\"\n \n static const char * const builtin_commit_usage[] = {\n \t\"git commit [options] [--] ...\",\n@@ -104,6 +105,7 @@ static char *cleanup_arg;\n static enum commit_whence whence;\n static int use_editor = 1, initial_commit, include_status = 1;\n static int show_ignored_in_status;\n+static int edit_notes;\n static const char *only_include_assumed;\n static struct strbuf message;\n \n@@ -146,6 +148,7 @@ static struct option builtin_commit_options[] = {\n \tOPT_BOOLEAN('e', \"edit\", &edit_flag, \"force edit of commit\"),\n \tOPT_STRING(0, \"cleanup\", &cleanup_arg, \"default\", \"how to strip spaces and #comments from message\"),\n \tOPT_BOOLEAN(0, \"status\", &include_status, \"include status in commit message template\"),\n+\tOPT_BOOLEAN(0, \"notes\", &edit_notes, \"edit notes interactively\"),\n \t/* end commit message options */\n \n \tOPT_GROUP(\"Commit contents options\"),\n@@ -603,6 +606,53 @@ static char *cut_ident_timestamp_part(char *string)\n \treturn ket;\n }\n \n+static void add_notes_from_commit(struct strbuf *out, const char *name)\n+{\n+\tstruct commit *commit;\n+\tstruct strbuf note = STRBUF_INIT;\n+\n+\tcommit = lookup_commit_reference_by_name(name);\n+\tif (!commit)\n+\t\tdie(\"could not lookup commit %s\", name);\n+\tformat_note(NULL, commit->object.sha1, ¬e,\n+\t\t get_commit_output_encoding(), 0);\n+\n+\tif (note.len) {\n+\t\tstrbuf_addstr(out, \"\\n---\\n\");\n+\t\tstrbuf_addbuf(out, ¬e);\n+\t}\n+\tstrbuf_release(¬e);\n+}\n+\n+static void extract_notes_from_message(struct strbuf *msg, struct strbuf *notes)\n+{\n+\tconst char *separator = strstr(msg->buf, \"\\n---\\n\");\n+\n+\tif (!separator)\n+\t\treturn;\n+\n+\tstrbuf_addstr(notes, separator + 5);\n+\tstrbuf_setlen(msg, separator - msg->buf + 1);\n+}\n+\n+static void update_notes_for_commit(struct strbuf *notes,\n+\t\t\t\t unsigned char *commit_sha1)\n+{\n+\tstripspace(notes, cleanup_mode == CLEANUP_ALL);\n+\n+\tif (!notes->len)\n+\t\tremove_note(NULL, commit_sha1);\n+\telse {\n+\t\tunsigned char blob_sha1[20];\n+\t\tif (write_sha1_file(notes->buf, notes->len,\n+\t\t\t\t blob_type, blob_sha1) < 0)\n+\t\t\tdie(\"unable to write note blob\");\n+\t\tadd_note(NULL, commit_sha1, blob_sha1,\n+\t\t\t combine_notes_overwrite);\n+\t}\n+\tcommit_notes(NULL, \"updated by commit --notes\");\n+}\n+\n static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t struct wt_status *s,\n \t\t\t struct strbuf *author_ident)\n@@ -730,6 +780,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstrbuf_release(&sob);\n \t}\n \n+\tif (edit_notes && amend)\n+\t\tadd_notes_from_commit(&sb, \"HEAD\");\n+\n \tif (fwrite(sb.buf, 1, sb.len, fp) < sb.len)\n \t\tdie_errno(\"could not write commit template\");\n \n@@ -997,8 +1050,10 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\tuse_editor = 0;\n \tif (edit_flag)\n \t\tuse_editor = 1;\n-\tif (!use_editor)\n+\tif (!use_editor) {\n \t\txsetenv(\"GIT_EDITOR\", \":\", 1);\n+\t\tedit_notes = 0;\n+\t}\n \n \tif (get_sha1(\"HEAD\", head_sha1))\n \t\tinitial_commit = 1;\n@@ -1357,6 +1412,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf author_ident = STRBUF_INIT;\n+\tstruct strbuf notes = STRBUF_INIT;\n \tconst char *index_file, *reflog_msg;\n \tchar *nl, *p;\n \tunsigned char commit_sha1[20];\n@@ -1458,6 +1514,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tstrbuf_setlen(&sb, p - sb.buf + 1);\n \t}\n \n+\tif (edit_notes)\n+\t\textract_notes_from_message(&sb, ¬es);\n+\n \tif (cleanup_mode != CLEANUP_NONE)\n \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (message_is_empty(&sb) && !allow_empty_message) {\n@@ -1473,6 +1532,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t}\n \tstrbuf_release(&author_ident);\n \n+\tif (edit_notes)\n+\t\tupdate_notes_for_commit(¬es, commit_sha1);\n+\tstrbuf_release(¬es);\n+\n \tref_lock = lock_any_ref_for_update(\"HEAD\",\n \t\t\t\t\t initial_commit ? NULL : head_sha1,\n \t\t\t\t\t 0);\n-- \n1.7.2.5.20.g7ba93.dirty\n"},{"id":"162244","messageId":"201102251658.22678.johan@herland.net","threadId":"26601","inReplyTo":"20110225133056.GA1026@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-02-25T15:58:22Z","receivedAt":"2011-02-25T15:58:22Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 25 February 2011, Jeff King wrote:\n> So my solution was that \"git commit\" should be able to embed and\n> extract notes from the commit message itself. The patch below\n> implements \"git commit --notes\", which does two things:\n>\n> 1. If we are amending, it populates the commit message not just\n> with the existing message, but also with a \"---\" divider and any\n> notes on the commit.\n>\n> 2. After editing the commit message, it looks for the \"---\" divider\n> and puts everything after it into a commit note (whether or not\n> it put in a divider in step (1), so you can add new notes, too).\n>\n> So your commit template looks like:\n>\n> subject\n>\n> commit message body\n> ---\n> notes data\n>\n> # usual template stuff\n>\n> I'm curious what people think. Do others find this useful? Does it\n> seem harmful?\n\nI _really_ like the idea. :)\n\n> It's yet another magic format to worry about when writing a commit\n> message. But you don't need to care unless you use \"--notes\" (and I\n> would probably add a config option, since I would always want this on\n> personally). And \"---\" is already something to be aware of, since\n> \"am\" treats it specially (technically, I could just drop \"notes\"\n> entirely and use \"---\" in my commit message; so perhaps this is just\n> overengineering).\n\nMaybe we should use a slightly more verbose separator (i.e. more \nunlikely to trigger false positives). As you say, we already have to \nwatch out for \"---\" because of \"am\", but that only applies to projects \nthat _use_ \"am\" (i.e. mailing-list-centric projects like git.git and \nthe Linux kernel). Other projects (e.g. github-centric projects or most \ncentralized \"$dayjob-style\" projects) seldom or never use \"am\" at all, \nso I wouldn't expect those developers think of \"---\" as \"special\" in \nany way.\n\nWhat about using something like \"--- Notes ---\" instead?\n\n> How should this interact with --cleanup? Right now it splits\n> everything after the \"---\" into the notes part, including any \"#\"\n> lines. Which should be fine, I think, because they get pulled out by\n> stripspace in either case. If you were using --cleanup=verbatim, then\n> you'd have gotten rid of them manually anyway. And if you really want\n> a literal \"---\", you would use \"git commit\" (or \"git commit\n> --no-notes\" once there is a config option). So I think the behavior\n> in this patch is sane.\n\nWhat if you combine --notes with --verbose (i.e. including the \ndiff-to-be-committed in the commit message template)?\n\nAFAICS, stripspace() doesn't know how to remove the diff (there's a \nseparate section in cmd_commit() discarding everything \nfollowing \"\\ndiff --git \").\n\n> I only turn on --edit when we launch an editor. It seems somehow more\n> confusing to me that \"git commit -F file\" should split notes out (or\n> worse, \"git commit -m\"). If you are doing things non-interactively,\n> it's probably not a big deal to just call \"git notes add\" separately.\n> And I expect \"-F\" is used by porcelains, or people wanting to do\n> verbatim stuff.\n\nAgreed.\n\n> How should this interact with the commit-msg hook? In my\n> implementation, it sees the whole thing, message and notes. Should we\n> be picking apart the two bits after the editor and rewriting the\n> COMMIT_EDITMSG before the hook sees it?\n\nI'm not sure about this, but I suspect we should follow the same \nbehaviour as --verbose (i.e. does the commit-msg hook see the entire \ndiff inline in the commit message?).\n\nA short look at builtin/commit.c indicates that we should leave \neverything in there for the commit-msg hook (AFAICS, the commit-msg \nhook is invoked from prepare_to_commit(), which is invoked from \ncmd_commit() _before_ the verbose diff part is removed.)\n\n> How should this interact with the post-rewrite hook? I obviously need\n> to set that up for my workflow, too, but I haven't yet. This patch\n> does nothing, but I'm pretty sure it should turn of \"git commit\n> --amend\" calling the rewrite hook if we are using --notes (since the\n> user has already seen and edited the notes, and we've written them\n> out).\n\nI don't see what this has to do with the post-rewrite hook. Currently, \nthe post-rewrite documentation (\"git help hooks\") states that it is run \n_after_ the automatic notes copying. AFAICS, your --notes simply \nreplaces the usual automatic notes copying with a \nsemi-automatic \"edit-and-copy\" instead. But this all happens before the \nport-rewrite hook is called, and thus shouldn't affect it.\n\n> @@ -730,6 +780,9 @@ static int prepare_to_commit(const char\n> *index_file, const char *prefix, strbuf_release(&sob);\n> \t}\n>\n> +\tif (edit_notes && amend)\n> +\t\tadd_notes_from_commit(&sb, \"HEAD\");\n\nI haven't read the sources closely enough to figure out when/where the \ncommit diff is added to the commit message (in case of --verbose), but \nI trust that it happens _after_ the above lines (so that the notes part \ndoesn't end up after the diff)\n\nOtherwise, this looks good to me from a precursory review.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"162250","messageId":"7v4o7saqj4.fsf@alter.siamese.dyndns.org","threadId":"26601","inReplyTo":"20110225133056.GA1026@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-25T18:59:59Z","receivedAt":"2011-02-25T18:59:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So your commit template looks like:\n>\n> subject\n>\n> commit message body\n> ---\n> notes data\n>\n> # usual template stuff\n>\n> I'm curious what people think. Do others find this useful? Does it seem\n> harmful?\n\nAs long as this is done only under \"commit --notes\", I don't think it\nshould hurt innocent bystanders.\n\n> It's yet another magic format to worry about when writing a commit\n> message. But you don't need to care unless you use \"--notes\" (and I\n> would probably add a config option, since I would always want this on\n> personally).\n\nThen --no-notes would also be necessary, but I think you would get it for\nfree these days ;-).\n\n> I only turn on --edit when we launch an editor. It seems somehow more\n> confusing to me that \"git commit -F file\" should split notes out (or\n> worse, \"git commit -m\").\n\nSo if you see -F -m and there is no --edit, you don't split out notes at\nthe divider? That sounds like a sensible thing to me.\n"},{"id":"162270","messageId":"1298665854.27129.25.camel@drew-northup.unet.maine.edu","threadId":"26601","inReplyTo":"20110225133056.GA1026@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-02-25T20:30:54Z","receivedAt":"2011-02-25T20:30:54Z","isPatch":true,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Fri, 2011-02-25 at 08:30 -0500, Jeff King wrote:\n> I was revising a long-ish series today, and I have been wanting to start\n> using \"git notes\" to store information on what's changed between\n> versions (which will eventually go after the \"---\" in format-patch).\n\n\n> 1. If we are amending, it populates the commit message not just with\n> the existing message, but also with a \"---\" divider and any notes on\n> the commit.\n> \n> 2. After editing the commit message, it looks for the \"---\" divider\n> and puts everything after it into a commit note (whether or not it\n> put in a divider in step (1), so you can add new notes, too).\n> \n> So your commit template looks like:\n> \n> subject\n> \n> commit message body\n> ---\n> notes data\n> \n> # usual template stuff\n> \n> I'm curious what people think. Do others find this useful? Does it seem\n> harmful?\n> \n\nI'm in agreement with the others that it doesn't seem like a bad idea,\nand likely a good one. Just one thing, can you add an end-of-note\ndelimiter (the same thing perhaps)? I didn't spend a long time looking\nat the code, but I can imagine more than a few ways for this to go wrong\nwithout one.\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"162362","messageId":"4D6A6056.20201@drmicha.warpmail.net","threadId":"26601","inReplyTo":"20110225133056.GA1026@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-02-27T14:31:50Z","receivedAt":"2011-02-27T14:31:50Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 25.02.2011 14:30:\n> I was revising a long-ish series today, and I have been wanting to start\n> using \"git notes\" to store information on what's changed between\n> versions (which will eventually go after the \"---\" in format-patch).\n> \n> So my workflow was something like:\n> \n> 1. git rebase -i, mark one or more commits for edit\n> \n> 2. For each commit we stop at:\n> \n> a. Tweak the tree either with enhancements, or to resolve\n> conflicts from tweaks to earlier patches.\n> \n> b. commit --amend, tweak commit message is needed\n> \n> c. git notes add, mention changes\n> \n> d. git rebase --continue\n> \n> Two things annoyed me:\n> \n> 1. Editing the commit message and notes separately felt awkward. They\n> are conceptually part of the same task to me.\n> \n> 2. In the conflict case, there is no opportunity to run \"git notes\n> add\" because you fix up commits and directly run \"rebase\n> --continue\".\n> \n> So my solution was that \"git commit\" should be able to embed and extract\n> notes from the commit message itself. The patch below implements \"git\n> commit --notes\", which does two things:\n> \n> 1. If we are amending, it populates the commit message not just with\n> the existing message, but also with a \"---\" divider and any notes on\n> the commit.\n> \n> 2. After editing the commit message, it looks for the \"---\" divider\n> and puts everything after it into a commit note (whether or not it\n> put in a divider in step (1), so you can add new notes, too).\n> \n> So your commit template looks like:\n> \n> subject\n> \n> commit message body\n> ---\n> notes data\n> \n> # usual template stuff\n> \n> I'm curious what people think. Do others find this useful? Does it seem\n> harmful?\n\nI can haz tis wiz \"format-patch --notes-behind-triple-dash\"?\n\nMichael\n"},{"id":"162589","messageId":"20110301215907.GA23945@sigill.intra.peff.net","threadId":"26601","inReplyTo":"201102251658.22678.johan@herland.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-01T21:59:07Z","receivedAt":"2011-03-01T21:59:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:\n\n> > I'm curious what people think. Do others find this useful? Does it\n> > seem harmful?\n> \n> I _really_ like the idea. :)\n\nThanks. Everybody seems to like it, so I'm going to polish it up and\nsubmit a nicer version.\n\n> Maybe we should use a slightly more verbose separator (i.e. more \n> unlikely to trigger false positives). As you say, we already have to \n> watch out for \"---\" because of \"am\", but that only applies to projects \n> that _use_ \"am\" (i.e. mailing-list-centric projects like git.git and \n> the Linux kernel). Other projects (e.g. github-centric projects or most \n> centralized \"$dayjob-style\" projects) seldom or never use \"am\" at all, \n> so I wouldn't expect those developers think of \"---\" as \"special\" in \n> any way.\n> \n> What about using something like \"--- Notes ---\" instead?\n\nYeah, it is true that many git users will never care about the\npatch-through-mail workflow. And I think these days that is OK, because\nrebase will take care to keep their commit message intact even if it\ndoesn't format well in a \"format-patch | am\" pipeline.\n\nI really wanted to keep it short and natural, though. Because eventually\nI'd like to have this on all the time via a config option, and I don't\nwant to see \"--- Notes ---\" in every commit that doesn't have notes. But\nI _do_ want to be able to quickly say \"oh, let me make a note on this\"\nand just add a quick separator.\n\nIt wouldn't be a regression if people had to opt into the feature using\nthe command-line or config option. So in theory they could learn about\n\"---\" then, unless we turn it on by default (but why would we? A user\nhas to know about this feature to use it, so they can easily turn on the\noption).\n\nOr maybe the divider should be configurable and default to something\nlong. But clueful people can set it to \"---\". That kind of seems like\noverkill, though.\n\n> What if you combine --notes with --verbose (i.e. including the \n> diff-to-be-committed in the commit message template)?\n> \n> AFAICS, stripspace() doesn't know how to remove the diff (there's a \n> separate section in cmd_commit() discarding everything \n> following \"\\ndiff --git \").\n\nUgh. Yeah, I looked at that in an earlier iteration but then forgot\nabout it in the final. We will end up with \"-v\" crap in the notes. I'll\nfix that in the next revision.\n\nI also think it will be worth making a nice test script of all of these\ndifferent cases.\n\n> > How should this interact with the commit-msg hook? In my\n> > implementation, it sees the whole thing, message and notes. Should we\n> > be picking apart the two bits after the editor and rewriting the\n> > COMMIT_EDITMSG before the hook sees it?\n> \n> I'm not sure about this, but I suspect we should follow the same \n> behaviour as --verbose (i.e. does the commit-msg hook see the entire \n> diff inline in the commit message?).\n> \n> A short look at builtin/commit.c indicates that we should leave \n> everything in there for the commit-msg hook (AFAICS, the commit-msg \n> hook is invoked from prepare_to_commit(), which is invoked from \n> cmd_commit() _before_ the verbose diff part is removed.)\n\nYeah, I think the commit-msg hook sees everything. Which is arguably not\nthe most convenient behavior, but it is the most flexible. Sort of. The\nhook doesn't actually know whether \"-v\" was supplied, so it has to guess\nat what is \"-v\" junk and what is not. I wonder if anyone actually uses\n\"-v\" these days. It seems like \"git add -p\" would have superseded it in\nmost workflows.\n\n> > How should this interact with the post-rewrite hook? I obviously need\n> > to set that up for my workflow, too, but I haven't yet. This patch\n> > does nothing, but I'm pretty sure it should turn of \"git commit\n> > --amend\" calling the rewrite hook if we are using --notes (since the\n> > user has already seen and edited the notes, and we've written them\n> > out).\n> \n> I don't see what this has to do with the post-rewrite hook. Currently, \n> the post-rewrite documentation (\"git help hooks\") states that it is run \n> _after_ the automatic notes copying. AFAICS, your --notes simply \n> replaces the usual automatic notes copying with a \n> semi-automatic \"edit-and-copy\" instead. But this all happens before the \n> port-rewrite hook is called, and thus shouldn't affect it.\n\nI think this was just me showing my cluelessness about how the notes\nrewriting code worked. I was thinking you needed to have a post-rewrite\nhook to make it work at all, but it looks like it does the rewrite and\nthen lets you tweak it. So my code doesn't turn off the existing copy,\nbut it probably should. Should the post-rewrite hook run after this? I'm\nnot really sure what people use post-rewrite hooks for, to be honest.\n\n> > @@ -730,6 +780,9 @@ static int prepare_to_commit(const char\n> > *index_file, const char *prefix, strbuf_release(&sob);\n> > \t}\n> >\n> > +\tif (edit_notes && amend)\n> > +\t\tadd_notes_from_commit(&sb, \"HEAD\");\n> \n> I haven't read the sources closely enough to figure out when/where the \n> commit diff is added to the commit message (in case of --verbose), but \n> I trust that it happens _after_ the above lines (so that the notes part \n> doesn't end up after the diff)\n\nI think so, but I'll double check. I agree that it's important for it to\ngo right after the commit message (and before the \"#\" comment lines, I\nthink).\n\n> Otherwise, this looks good to me from a precursory review.\n\nThanks. I'll work on some tests for the --cleanup and -v cases so we can\nbe sure that it's behaving as we want, and then hopefully submit a nicer\nversion.\n\n-Peff\n"},{"id":"162590","messageId":"20110301220053.GB23945@sigill.intra.peff.net","threadId":"26601","inReplyTo":"1298665854.27129.25.camel@drew-northup.unet.maine.edu","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-01T22:00:53Z","receivedAt":"2011-03-01T22:00:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 25, 2011 at 03:30:54PM -0500, Drew Northup wrote:\n\n> > So your commit template looks like:\n> > \n> > subject\n> > \n> > commit message body\n> > ---\n> > notes data\n> > \n> > # usual template stuff\n> > \n> > I'm curious what people think. Do others find this useful? Does it seem\n> > harmful?\n> > \n> \n> I'm in agreement with the others that it doesn't seem like a bad idea,\n> and likely a good one. Just one thing, can you add an end-of-note\n> delimiter (the same thing perhaps)? I didn't spend a long time looking\n> at the code, but I can imagine more than a few ways for this to go wrong\n> without one.\n\nWe could add one pretty easily, but I'm not sure what you would be\ndelimiting it from. Can you describe a case where it would be useful?\n\n-Peff\n"},{"id":"162591","messageId":"20110301220144.GC23945@sigill.intra.peff.net","threadId":"26601","inReplyTo":"4D6A6056.20201@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-01T22:01:45Z","receivedAt":"2011-03-01T22:01:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 27, 2011 at 03:31:50PM +0100, Michael J Gruber wrote:\n\n> > So your commit template looks like:\n> > \n> > subject\n> > \n> > commit message body\n> > ---\n> > notes data\n> > \n> > # usual template stuff\n> > \n> I can haz tis wiz \"format-patch --notes-behind-triple-dash\"?\n\nYeah, I think that would be a nice 2/2 to this series. From past\nthreads, it seems that it is not as trivial as one would like, but I can\ntake a look.\n\n-Peff\n"},{"id":"162593","messageId":"1299017913.14490.10.camel@drew-northup.unet.maine.edu","threadId":"26601","inReplyTo":"20110301220053.GB23945@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-03-01T22:18:33Z","receivedAt":"2011-03-01T22:18:33Z","isPatch":true,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Tue, 2011-03-01 at 17:00 -0500, Jeff King wrote:\n> On Fri, Feb 25, 2011 at 03:30:54PM -0500, Drew Northup wrote:\n> \n> > > So your commit template looks like:\n> > > \n> > > subject\n> > > \n> > > commit message body\n> > > ---\n> > > notes data\n> > > \n> > > # usual template stuff\n> > > \n> > > I'm curious what people think. Do others find this useful? Does it seem\n> > > harmful?\n> > > \n> > \n> > I'm in agreement with the others that it doesn't seem like a bad idea,\n> > and likely a good one. Just one thing, can you add an end-of-note\n> > delimiter (the same thing perhaps)? I didn't spend a long time looking\n> > at the code, but I can imagine more than a few ways for this to go wrong\n> > without one.\n> \n> We could add one pretty easily, but I'm not sure what you would be\n> delimiting it from. Can you describe a case where it would be useful?\n> \n> -Peff\n\nA notes message which contains \"the usual template stuff\" as means of\ndescribing a change to it, for starters...\n\nThere is likely good reason why the commit message already has an end\nmark, I suspect that also applies here. (Unless you count \"---\" between\nthe commit message and the patch as \"the usual template stuff\"--which\nwasn't clear at this keyboard anyway.) If that's already in there then\nplease forgive the noise--it didn't jump out at me, but I also spend way\ntoo much time programming in too many languages for that to be very\nlikely.\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"162595","messageId":"20110301222350.GA24215@sigill.intra.peff.net","threadId":"26601","inReplyTo":"1299017913.14490.10.camel@drew-northup.unet.maine.edu","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-01T22:23:51Z","receivedAt":"2011-03-01T22:23:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 01, 2011 at 05:18:33PM -0500, Drew Northup wrote:\n\n> A notes message which contains \"the usual template stuff\" as means of\n> describing a change to it, for starters...\n\nBut we strip that from the notes, unless you use --cleanup. But in that\ncase, you would have deleted the template cruft, since it pollutes your\nmessage.\n\n> There is likely good reason why the commit message already has an end\n> mark, I suspect that also applies here.\n\nIt doesn't have an end mark. The \"usual template stuff\" just happens to\nbe at the end. But any line starting with \"#\" will be removed unless you\nuse --cleanup, whether you use --notes or no. Similarly, unadorned lines\nafter the \"#\" lines will be counted as part of the message.\n\n> (Unless you count \"---\" between the commit message and the patch as\n> \"the usual template stuff\"--which wasn't clear at this keyboard\n> anyway.)\n\nNo, I meant the \"#\" lines. The \"---\" of format-patch isn't relevant\nhere, since we're just talking about commit messages inside the editor\nduring git-commit.\n\nThe really evil bit is \"-v\" which appends a giant diff with no real\nindication that it isn't part of the commit message. We already get rid\nof it with some heuristics (which I remember improving a while back).\nI don't think my RFC patch handles it very well, but that is something I\nwill be looking at for the next revision.\n\n-Peff\n"},{"id":"162596","messageId":"1299018395.14490.12.camel@drew-northup.unet.maine.edu","threadId":"26601","inReplyTo":"20110301222350.GA24215@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-03-01T22:26:35Z","receivedAt":"2011-03-01T22:26:35Z","isPatch":true,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Tue, 2011-03-01 at 17:23 -0500, Jeff King wrote:\n> On Tue, Mar 01, 2011 at 05:18:33PM -0500, Drew Northup wrote:\n\n> \n> No, I meant the \"#\" lines. The \"---\" of format-patch isn't relevant\n> here, since we're just talking about commit messages inside the editor\n> during git-commit.\n\nThat's the part I missed. I'll crawl back under my rock now...\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"162612","messageId":"201103020121.54690.johan@herland.net","threadId":"26601","inReplyTo":"20110301215907.GA23945@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-03-02T00:21:54Z","receivedAt":"2011-03-02T00:21:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 01 March 2011, Jeff King wrote:\n> On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:\n> > What about using something like \"--- Notes ---\" instead?\n> \n> Yeah, it is true that many git users will never care about the\n> patch-through-mail workflow. And I think these days that is OK, because\n> rebase will take care to keep their commit message intact even if it\n> doesn't format well in a \"format-patch | am\" pipeline.\n> \n> I really wanted to keep it short and natural, though. Because eventually\n> I'd like to have this on all the time via a config option, and I don't\n> want to see \"--- Notes ---\" in every commit that doesn't have notes. But\n> I _do_ want to be able to quickly say \"oh, let me make a note on this\"\n> and just add a quick separator.\n> \n> It wouldn't be a regression if people had to opt into the feature using\n> the command-line or config option. So in theory they could learn about\n> \"---\" then, unless we turn it on by default (but why would we? A user\n> has to know about this feature to use it, so they can easily turn on the\n> option).\n\nI'm not so sure. Requiring users to opt in might be enough protection \nagainst false \"---\" positives. I'm just paranoid about someone turning this \non in their global config, and then forgetting all about it when they format \na commit message like this:\n\n Subject line of the commit message\n\n What\n ----\n Lorem ipsum dolor sit amet.\n\n Why\n ---\n eggs spam bacon spam spam spam.\n\n(which will end the commit message after \"Why\", and add the last line as a \nnote)\n\nJust grepping through a \"git log\" from git.git master, I can find one \nalmost-false-positive in b6b84d1 (\"---\" appears slightly indented), and \ngrepping through linux-2.6 master, I find plenty potential for false \npositives:\n\n a2d49358ba9bc93204dc001d5568c5bdb299b77d (almost false positive)\n 20cbd3e120a0c20bebe420e1fed0e816730bb988 (almost false positive)\n 68845cb2c82275efd7390026bba70c320ca6ef86 (false positive)\n 5e553110f27ff77591ec7305c6216ad6949f7a95 (false positive)\n 9638d89a75776abc614c29cdeece0cc874ea2a4c (false positive)\n\nRemember that developers sometimes cut-n-paste output from other programs \n(debug sessions, performance benchmarks, etc.) into their commit message, \nand that makes a false positive a lot more likely to slip through.\n\n> Or maybe the divider should be configurable and default to something\n> long. But clueful people can set it to \"---\". That kind of seems like\n> overkill, though.\n\nNot sure that would help. I consider myself \"clueful\" enough that I'd likely \nset it to \"---\", but I also know myself well enough that if I pasted some \ndebug/performance output into a commit message, and that output happened to \ncontain a \"---\", it would likely slip through...\n\n> > > How should this interact with the commit-msg hook? In my\n> > > implementation, it sees the whole thing, message and notes. Should we\n> > > be picking apart the two bits after the editor and rewriting the\n> > > COMMIT_EDITMSG before the hook sees it?\n> > \n> > I'm not sure about this, but I suspect we should follow the same\n> > behaviour as --verbose (i.e. does the commit-msg hook see the entire\n> > diff inline in the commit message?).\n> > \n> > A short look at builtin/commit.c indicates that we should leave\n> > everything in there for the commit-msg hook (AFAICS, the commit-msg\n> > hook is invoked from prepare_to_commit(), which is invoked from\n> > cmd_commit() _before_ the verbose diff part is removed.)\n> \n> Yeah, I think the commit-msg hook sees everything. Which is arguably not\n> the most convenient behavior, but it is the most flexible. Sort of. The\n> hook doesn't actually know whether \"-v\" was supplied, so it has to guess\n> at what is \"-v\" junk and what is not.\n\nYeah, it might be messy today, but I don't think you can clean it up without \nchanging the commit-msg hook interface, which to me means that the cleanup \nshould probably happen in a separate series.\n\n> I wonder if anyone actually uses\n> \"-v\" these days. It seems like \"git add -p\" would have superseded it in\n> most workflows.\n\nI find myself using -v every now and then, to just have the diff handy while \nI construct the commit message. Makes it easier to refer to function names, \netc. in the commit message.\n\n> > > How should this interact with the post-rewrite hook? I obviously need\n> > > to set that up for my workflow, too, but I haven't yet. This patch\n> > > does nothing, but I'm pretty sure it should turn of \"git commit\n> > > --amend\" calling the rewrite hook if we are using --notes (since the\n> > > user has already seen and edited the notes, and we've written them\n> > > out).\n> > \n> > I don't see what this has to do with the post-rewrite hook. Currently,\n> > the post-rewrite documentation (\"git help hooks\") states that it is run\n> > _after_ the automatic notes copying. AFAICS, your --notes simply\n> > replaces the usual automatic notes copying with a\n> > semi-automatic \"edit-and-copy\" instead. But this all happens before the\n> > port-rewrite hook is called, and thus shouldn't affect it.\n> \n> I think this was just me showing my cluelessness about how the notes\n> rewriting code worked. I was thinking you needed to have a post-rewrite\n> hook to make it work at all, but it looks like it does the rewrite and\n> then lets you tweak it.\n\nIndeed, the notes rewrite does not depend on the post-rewrite hook at all.\n\n> So my code doesn't turn off the existing copy, but it probably should.\n\nYeah, if the user edits the note, you don't want the notes rewriting code \nclobbering the edited note by copying the original note on top of it.\n\n> Should the post-rewrite hook run after this? I'm not really sure what\n> people use post-rewrite hooks for, to be honest.\n\nMe neither, but from its name I gather that it should be run whenever a \ncommit is \"rewritten\" (amend, rebase, etc.). As such, it binds closer to the \ncommit rewrite itself, rather than to the accompanying notes copy (or \"edit-\nand-copy\" in your case). That's why I argue that --notes should not affect \nthe invocation of the post-rewrite hook.\n\n> > Otherwise, this looks good to me from a precursory review.\n> \n> Thanks. I'll work on some tests for the --cleanup and -v cases so we can\n> be sure that it's behaving as we want, and then hopefully submit a nicer\n> version.\n\nLooking forward to it. :)\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"162624","messageId":"4D6DEB64.1080003@gmail.com","threadId":"26601","inReplyTo":"20110301215907.GA23945@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2011-03-02T07:01:56Z","receivedAt":"2011-03-02T07:01:56Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 02/03/11 10:59, Jeff King wrote:\n> On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:\n>> Maybe we should use a slightly more verbose separator (i.e. more \n>> unlikely to trigger false positives). As you say, we already have to \n>> watch out for \"---\" because of \"am\", but that only applies to projects \n>> that _use_ \"am\" (i.e. mailing-list-centric projects like git.git and \n>> the Linux kernel). Other projects (e.g. github-centric projects or most \n>> centralized \"$dayjob-style\" projects) seldom or never use \"am\" at all, \n>> so I wouldn't expect those developers think of \"---\" as \"special\" in \n>> any way.\n>>\n>> What about using something like \"--- Notes ---\" instead?\n> \n> Yeah, it is true that many git users will never care about the\n> patch-through-mail workflow. And I think these days that is OK, because\n> rebase will take care to keep their commit message intact even if it\n> doesn't format well in a \"format-patch | am\" pipeline.\n> \n> I really wanted to keep it short and natural, though. Because eventually\n> I'd like to have this on all the time via a config option, and I don't\n> want to see \"--- Notes ---\" in every commit that doesn't have notes. But\n> I _do_ want to be able to quickly say \"oh, let me make a note on this\"\n> and just add a quick separator.\n\n\nWhat about \"#---\"? Satisfies the quick to type and is a lot less likely\nto appear in commit messages. Not sure about the implications of finding\nthat string before the commit message is stripped.\n\n"},{"id":"162632","messageId":"1299069921.17973.26.camel@drew-northup.unet.maine.edu","threadId":"26601","inReplyTo":"4D6DEB64.1080003@gmail.com","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-03-02T12:45:21Z","receivedAt":"2011-03-02T12:45:21Z","isPatch":true,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Wed, 2011-03-02 at 20:01 +1300, Chris Packham wrote:\n> On 02/03/11 10:59, Jeff King wrote:\n> > On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:\n> >> Maybe we should use a slightly more verbose separator (i.e. more \n> >> unlikely to trigger false positives). As you say, we already have to \n> >> watch out for \"---\" because of \"am\", but that only applies to projects \n> >> that _use_ \"am\" (i.e. mailing-list-centric projects like git.git and \n> >> the Linux kernel). Other projects (e.g. github-centric projects or most \n> >> centralized \"$dayjob-style\" projects) seldom or never use \"am\" at all, \n> >> so I wouldn't expect those developers think of \"---\" as \"special\" in \n> >> any way.\n> >>\n> >> What about using something like \"--- Notes ---\" instead?\n> > \n> > Yeah, it is true that many git users will never care about the\n> > patch-through-mail workflow. And I think these days that is OK, because\n> > rebase will take care to keep their commit message intact even if it\n> > doesn't format well in a \"format-patch | am\" pipeline.\n> > \n> > I really wanted to keep it short and natural, though. Because eventually\n> > I'd like to have this on all the time via a config option, and I don't\n> > want to see \"--- Notes ---\" in every commit that doesn't have notes. But\n> > I _do_ want to be able to quickly say \"oh, let me make a note on this\"\n> > and just add a quick separator.\n> \n> \n> What about \"#---\"? Satisfies the quick to type and is a lot less likely\n> to appear in commit messages. Not sure about the implications of finding\n> that string before the commit message is stripped.\n> \n\n\nTrue enough, but that would be seen as a comment and dropped outright\nthe way things are currently standing. If you want short, definitely\nrare, and most likely intentional you'd need something harder to\nremember like \"-!N\" as the tag. I don't know how well that'd go over\nwith people--it definitely isn't natural.\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"162650","messageId":"AANLkTimW-Cs5LVVOL9tpFiN6JsarWVo4Kua4ky7N1HB-@mail.gmail.com","threadId":"26601","inReplyTo":"4D6DEB64.1080003@gmail.com","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Piotr Krukowiecki","fromEmail":"piotr.krukowiecki@gmail.com","sentAt":"2011-03-02T16:24:13Z","receivedAt":"2011-03-02T16:24:13Z","isPatch":true,"sender":{"key":"piotr.krukowiecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3259959?v=4"},"body":"Hi,\n\nOn Wed, Mar 2, 2011 at 8:01 AM, Chris Packham wrote:\n> On 02/03/11 10:59, Jeff King wrote:\n>> On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:\n>>> Maybe we should use a slightly more verbose separator (i.e. more\n>>> unlikely to trigger false positives). As you say, we already have to\n>>> watch out for \"---\" because of \"am\", but that only applies to projects\n>>> that _use_ \"am\" (i.e. mailing-list-centric projects like git.git and\n>>> the Linux kernel). Other projects (e.g. github-centric projects or most\n>>> centralized \"$dayjob-style\" projects) seldom or never use \"am\" at all,\n>>> so I wouldn't expect those developers think of \"---\" as \"special\" in\n>>> any way.\n>>>\n>>> What about using something like \"--- Notes ---\" instead?\n>>\n>> Yeah, it is true that many git users will never care about the\n>> patch-through-mail workflow. And I think these days that is OK, because\n>> rebase will take care to keep their commit message intact even if it\n>> doesn't format well in a \"format-patch | am\" pipeline.\n>>\n>> I really wanted to keep it short and natural, though. Because eventually\n>> I'd like to have this on all the time via a config option, and I don't\n>> want to see \"--- Notes ---\" in every commit that doesn't have notes. But\n>> I _do_ want to be able to quickly say \"oh, let me make a note on this\"\n>> and just add a quick separator.\n\nIMO typing \"--- Notes ---\" is quite fast. I suspect that most commits won't\nhave any notes, so you'll have to type it rarely.\n\nAlso, what should be the template with notes enabled? Should there\nbe the separator by default or not? Assuming notes are entered rarely,\nI think template should not have the separator. But if you use \"--notes\"\ncommand line option (i.e. interactive use), it should be there.\n\n\n> \n> What about \"#---\"? Satisfies the quick to type and is a lot less likely\n> to appear in commit messages. Not sure about the implications of finding\n> that string before the commit message is stripped.\n> \n\nI think the separator should be:\n1. unique enough so people won't enter it by accident\n2. easy to remember, easy to type\n3. descriptive so you won't have to look into documentation to see what \"#---\"\n means.\n\nI think \"--- Notes ---\" fulfills all requirements.\n\nAlso, in case of separator with some text, like \"--- Notes ---\", we would\nlike to be able to translate it probably, so I will see \"--- Notatki\n---\" in Polish.\n\n\n-- \nPiotrek\n"},{"id":"162694","messageId":"AANLkTino7fGnLutJ3cAxcvx8O-JbcDPJDrYHznjoN-TC@mail.gmail.com","threadId":"26601","inReplyTo":"201103020121.54690.johan@herland.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-03-03T01:57:22Z","receivedAt":"2011-03-03T01:57:22Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Wed, Mar 2, 2011 at 01:21, Johan Herland wrote:\n>> I wonder if anyone actually uses\n>> \"-v\" these days. It seems like \"git add -p\" would have superseded it in\n>> most workflows.\n>\n> I find myself using -v every now and then, to just have the diff handy while\n> I construct the commit message. Makes it easier to refer to function names,\n> etc. in the commit message.\n\nCan someone explain why -v does not output it's data prefixed by a\n'#'? If someone really wanted to include it in their commit message\nthey can column-select-delete it, and if they don't, it just gets\ndeleted by the cleanup code?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"162699","messageId":"7v39n4ammh.fsf@alter.siamese.dyndns.org","threadId":"26601","inReplyTo":"AANLkTino7fGnLutJ3cAxcvx8O-JbcDPJDrYHznjoN-TC@mail.gmail.com","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-03T03:50:14Z","receivedAt":"2011-03-03T03:50:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Can someone explain why -v does not output it's data prefixed by a\n> '#'? If someone really wanted to include it in their commit message\n> they can column-select-delete it, and if they don't, it just gets\n> deleted by the cleanup code?\n\nGood question. There was no reason other than \"that is just a historical\naccident\".\n\nThe intention has always been \"allow people to review the change for the\nlast time while writing a log message\"; there was never a feature request\nto allow the diff to be included---it would always have been an unwelcome\naccident it that ever happened.\n"},{"id":"162713","messageId":"AANLkTikzgGY8Fryfc7n2MYiL8ZvY1Vr0cj4QStAypwBf@mail.gmail.com","threadId":"26601","inReplyTo":"7v39n4ammh.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-03-03T11:12:39Z","receivedAt":"2011-03-03T11:12:39Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, Mar 3, 2011 at 04:50, Junio C Hamano wrote:\n> Good question.  There was no reason other than \"that is just a historical\n> accident\".\n>\n> The intention has always been \"allow people to review the change for the\n> last time while writing a log message\"; there was never a feature request\n> to allow the diff to be included---it would always have been an unwelcome\n> accident it that ever happened.\n\nIn that case, can we change it to be that way now if it makes this case easier?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"162715","messageId":"1299151419-16027-1-git-send-email-icomfort@stanford.edu","threadId":"26601","inReplyTo":"AANLkTikzgGY8Fryfc7n2MYiL8ZvY1Vr0cj4QStAypwBf@mail.gmail.com","subject":"[PATCH] commit, status: #comment diff output in verbose mode","fromName":"Ian Ward Comfort","fromEmail":"icomfort@stanford.edu","sentAt":"2011-03-03T11:23:39Z","receivedAt":"2011-03-03T11:23:39Z","isPatch":true,"sender":{"key":"icomfort@stanford.edu","avatar":"https://avatars.githubusercontent.com/u/202841?v=4"},"body":"On 3 Mar 2011, at 3:12 AM, Sverre Rabbelier wrote:\n> On Thu, Mar 3, 2011 at 04:50, Junio C Hamano wrote:\n>> Good question. There was no reason other than \"that is just a historical\n>> accident\".\n>>\n>> The intention has always been \"allow people to review the change for the\n>> last time while writing a log message\"; there was never a feature request\n>> to allow the diff to be included---it would always have been an unwelcome\n>> accident it that ever happened.\n>\n> In that case, can we change it to be that way now if it makes this case\n> easier?\n\nHow about something like this?\n\n--8<--\n\nBy historical accident, diffs included in commit templates and status\noutput when the \"-v\" option is given are not prefixed with the # comment\ncharacter, as other advice and status information is. Stripping these\nlines is thus a best-effort operation, as it is not always possible to\ntell which lines were generated by \"-v\" and which were inserted by the\nuser.\n\nImprove this situation by adding the # prefix to diff output along with\nall other status output in these cases. The change is simply made thanks\nto a3c158d (Add a prefix output callback to diff output, 2010-05-26). The\nprefixed diff can be stripped (or not, as configured) by the standard\ncleanup code, so our special verbose-mode heuristic can be removed.\n\nDocumentation and a few tests which rely on the old \"-v\" format are\nupdated to match. One known breakage is fixed in t7507.\n\nSigned-off-by: Ian Ward Comfort \n---\n Documentation/git-commit.txt | 3 +--\n builtin/commit.c | 7 -------\n t/t4030-diff-textconv.sh | 2 +-\n t/t7502-commit.sh | 4 ++--\n t/t7507-commit-verbose.sh | 4 ++--\n wt-status.c | 12 ++++++++++++\n 6 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 8f89f6f..792f993 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -233,8 +233,7 @@ configuration variable documented in linkgit:git-config[1].\n --verbose::\n \tShow unified diff between the HEAD commit and what\n \twould be committed at the bottom of the commit message\n-\ttemplate. Note that this diff output doesn't have its\n-\tlines prefixed with '#'.\n+\ttemplate.\n \n -q::\n --quiet::\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 355b2cb..efecac3 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1381,13 +1381,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tdie(\"could not read commit message: %s\", strerror(saved_errno));\n \t}\n \n-\t/* Truncate the message just before the diff, if any. */\n-\tif (verbose) {\n-\t\tp = strstr(sb.buf, \"\\ndiff --git \");\n-\t\tif (p != NULL)\n-\t\t\tstrbuf_setlen(&sb, p - sb.buf + 1);\n-\t}\n-\n \tif (cleanup_mode != CLEANUP_NONE)\n \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (message_is_empty(&sb) && !allow_empty_message) {\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 88c5619..b00999e 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -79,7 +79,7 @@ test_expect_success 'format-patch produces binary' '\n test_expect_success 'status -v produces text' '\n \tgit reset --soft HEAD^ &&\n \tgit status -v >diff &&\n-\tfind_diff actual &&\n+\tsed -e \"s/^# //\" actual &&\n \ttest_cmp expect.text actual &&\n \tgit reset --soft HEAD@{1}\n '\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 50da034..a916001 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -151,8 +151,8 @@ test_expect_success 'verbose' '\n \n \techo minus >negative &&\n \tgit add negative &&\n-\tgit status -v | sed -ne \"/^diff --git /p\" >actual &&\n-\techo \"diff --git a/negative b/negative\" >expect &&\n+\tgit status -v | sed -ne \"/^# diff --git /p\" >actual &&\n+\techo \"# diff --git a/negative b/negative\" >expect &&\n \ttest_cmp expect actual\n \n '\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex da5bd3b..5b21bbb 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -5,7 +5,7 @@ test_description='verbose commit template'\n \n cat >check-for-diff <is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;\n \tsetup_revisions(0, NULL, &rev, &opt);\n \n+\tstrbuf_addstr(&diff_output_prefix, \"# \");\n+\n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n \trev.diffopt.detect_rename = 1;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n+\trev.diffopt.output_prefix = diff_output_prefix_callback;\n+\trev.diffopt.output_prefix_data = &diff_output_prefix;\n \t/*\n \t * If we're not going to stdout, then we definitely don't\n \t * want color, since we are going to the commit message\n@@ -609,6 +620,7 @@ static void wt_status_print_verbose(struct wt_status *s)\n \tif (s->fp != stdout)\n \t\tDIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);\n \trun_diff_index(&rev, 1);\n+\tstrbuf_release(&diff_output_prefix);\n }\n \n static void wt_status_print_tracking(struct wt_status *s)\n-- \n1.7.4.1.177.g1c06f\n"},{"id":"162714","messageId":"AANLkTinDWTEM4C2tkCyEa3zgrFNceUJW_qw7Bj94HDvL@mail.gmail.com","threadId":"26601","inReplyTo":"1299151419-16027-1-git-send-email-icomfort@stanford.edu","subject":"Re: [PATCH] commit, status: #comment diff output in verbose mode","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-03-03T11:25:59Z","receivedAt":"2011-03-03T11:25:59Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, Mar 3, 2011 at 12:23, Ian Ward Comfort wrote:\n> How about something like this?\n\nYup, that's what I meant :)\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"162947","messageId":"20110307233902.GA20447@sigill.intra.peff.net","threadId":"26601","inReplyTo":"201103020121.54690.johan@herland.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-07T23:39:02Z","receivedAt":"2011-03-07T23:39:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 02, 2011 at 01:21:54AM +0100, Johan Herland wrote:\n\n> Just grepping through a \"git log\" from git.git master, I can find one \n> almost-false-positive in b6b84d1 (\"---\" appears slightly indented), and \n> grepping through linux-2.6 master, I find plenty potential for false \n> positives:\n> \n> a2d49358ba9bc93204dc001d5568c5bdb299b77d (almost false positive)\n> 20cbd3e120a0c20bebe420e1fed0e816730bb988 (almost false positive)\n> 68845cb2c82275efd7390026bba70c320ca6ef86 (false positive)\n> 5e553110f27ff77591ec7305c6216ad6949f7a95 (false positive)\n> 9638d89a75776abc614c29cdeece0cc874ea2a4c (false positive)\n\nThere is actually one false positive in git.git (1dfcfbc), but it looks\nlike a broken commit message in the first place (IOW, \"---\" _was_\nspecial here, and it got broken during application). It appears many\ntimes in linux-2.6, but in most I examined it looks like a similar case:\nit _should_ have been removed during git-am or equivalent, but for some\nreason was not, and the result is \"---\" cruft at the bottom of the\nmessage, or sometimes a bunch of irrelevant patch text stuck in the\nmessage.\n\nThe ones you mentioned are indeed false positives. I wonder if linux-2.6\nis really a good repo to look at, though. Screwups aside, many patch\napplications are happening using \"git am\", so of course we wouldn't see\nthe true number of false positives, as they were already mangled before\nthey made it into the repo.\n\n> Remember that developers sometimes cut-n-paste output from other programs \n> (debug sessions, performance benchmarks, etc.) into their commit message, \n> and that makes a false positive a lot more likely to slip through.\n\nYeah, that's my biggest concern. I just really foresee myself getting\nannoyed by typing \"--- nOtes ---\", or \"-- Notes ---\". It's just a few\ncharacters shorter, but \"---\" is really less error prone.\n\n> > Or maybe the divider should be configurable and default to something\n> > long. But clueful people can set it to \"---\". That kind of seems like\n> > overkill, though.\n> \n> Not sure that would help. I consider myself \"clueful\" enough that I'd likely \n> set it to \"---\", but I also know myself well enough that if I pasted some \n> debug/performance output into a commit message, and that output happened to \n> contain a \"---\", it would likely slip through...\n\nI think you're arguing both sides here. Making it \"---\" is too\nerror-prone that we should make the decision on behalf of everyone to\nchoose something else. Yet if given the opportunity to make the\ndecision, you would choose \"---\"? :)\n\nI am really leaning towards configurability. Somebody else pointed out\nthat we would probably want it translatable anyway, so we will have to\ndeal with an arbitrary string anyway.\n\n> I find myself using -v every now and then, to just have the diff handy while \n> I construct the commit message. Makes it easier to refer to function names, \n> etc. in the commit message.\n\nMy new tests cover this (and --cleanup=verbatim leaving both intact).\n\n> Indeed, the notes rewrite does not depend on the post-rewrite hook at all.\n\nYeah, I was thinking of the config you have to setup, which I had not\ndone before. The original patch actually did OK with it, but we created\nuseless extra \"notes copy\" commits on the notes ref which got\nsuperseded. The new version just avoids the rewrite if we are doing an\nedit.\n\nSo here's my new version. Still some work to be done, as noted in the\ncover letter for 2/2.\n\n [1/2]: notes: make expand_notes_ref globally accessible\n [2/2]: commit: allow editing notes in commit message editor\n\n-Peff\n"},{"id":"162948","messageId":"20110307233956.GA20912@sigill.intra.peff.net","threadId":"26601","inReplyTo":"20110307233902.GA20447@sigill.intra.peff.net","subject":"[PATCH 1/2] notes: make expand_notes_ref globally accessible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-07T23:39:56Z","receivedAt":"2011-03-07T23:39:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function is useful for other commands besides \"git\nnotes\" which want to let users refer to notes by their\nshorthand name.\n\nSigned-off-by: Jeff King \n---\n builtin/notes.c | 10 ----------\n notes.c | 10 ++++++++++\n notes.h | 3 +++\n 3 files changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 0aab150..f2ccb75 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -100,16 +100,6 @@ struct msg_arg {\n \tstruct strbuf buf;\n };\n \n-static void expand_notes_ref(struct strbuf *sb)\n-{\n-\tif (!prefixcmp(sb->buf, \"refs/notes/\"))\n-\t\treturn; /* we're happy */\n-\telse if (!prefixcmp(sb->buf, \"notes/\"))\n-\t\tstrbuf_insert(sb, 0, \"refs/\", 5);\n-\telse\n-\t\tstrbuf_insert(sb, 0, \"refs/notes/\", 11);\n-}\n-\n static int list_each_note(const unsigned char *object_sha1,\n \t\tconst unsigned char *note_sha1, char *note_path,\n \t\tvoid *cb_data)\ndiff --git a/notes.c b/notes.c\nindex a013c1b..f6b9b6a 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1285,3 +1285,13 @@ int copy_note(struct notes_tree *t,\n \n \treturn 0;\n }\n+\n+void expand_notes_ref(struct strbuf *sb)\n+{\n+\tif (!prefixcmp(sb->buf, \"refs/notes/\"))\n+\t\treturn; /* we're happy */\n+\telse if (!prefixcmp(sb->buf, \"notes/\"))\n+\t\tstrbuf_insert(sb, 0, \"refs/\", 5);\n+\telse\n+\t\tstrbuf_insert(sb, 0, \"refs/notes/\", 11);\n+}\ndiff --git a/notes.h b/notes.h\nindex 83bd6e0..60bdf28 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -307,4 +307,7 @@ void string_list_add_refs_by_glob(struct string_list *list, const char *glob);\n 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+\n #endif\n-- \n1.7.4.31.g76e18\n"},{"id":"162949","messageId":"20110307234138.GB20912@sigill.intra.peff.net","threadId":"26601","inReplyTo":"20110307233902.GA20447@sigill.intra.peff.net","subject":"[PATCH 2/2] commit: allow editing notes in commit message editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-07T23:41:38Z","receivedAt":"2011-03-07T23:41:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One workflow for git-notes is to informally keep a list of\nchanges from one version of a patch to another as the patch\nis modified via \"commit --amend\" and \"git rebase\". Often the\nmost convenient time for this is while editing the commit\nmessage, since you see it during amend, during \"rebase -i\"\nedit stops, and when \"rebase\" finds a conflict.\n\nThis patch adds a \"--notes\" option which displays existing\nnotes in the commit editor (in the case of --amend), and\nextracts new notes from the editor message to add to the\nnewly created commit.\n\nThe feature is activated only for interactive edits, so \"-F\"\nand \"-m\" messages are safe from munging.\n\nSigned-off-by: Jeff King \n---\nChanges from v1:\n\n - fix bug with adding notes to new commit (we failed to\n initialize the notes tree properly in this case)\n\n - you can now do \"commit --notes=foo\" to view/edit\n refs/notes/foo\n\n - added tests for basic operations, plus interaction with\n --cleanup and -v\n\n - turn off commit rewriting when we edit\n\nTodo:\n\n - commit.notes config variable to have this on all the time\n\n - I punted on the separator decision here.\n\n - probably still some magic needed for rebase conflict\n case; we will be making a new commit, so we don't know\n to pull the notes in from the old commit as we do with\n --amend.\n\n - still needs the format-patch component to make the\n workflow complete :)\n\n builtin/commit.c | 87 ++++++++++++++++++++++-\n t/t7510-commit-notes.sh | 183 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 268 insertions(+), 2 deletions(-)\n create mode 100755 t/t7510-commit-notes.sh\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d71e1e0..f84ca23 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -26,6 +26,7 @@\n #include \"unpack-trees.h\"\n #include \"quote.h\"\n #include \"submodule.h\"\n+#include \"blob.h\"\n \n static const char * const builtin_commit_usage[] = {\n \t\"git commit [options] [--] ...\",\n@@ -90,6 +91,8 @@ static char *cleanup_arg;\n \n static int use_editor = 1, initial_commit, in_merge, include_status = 1;\n static int show_ignored_in_status;\n+static const char *edit_notes;\n+static struct notes_tree edit_notes_tree;\n static const char *only_include_assumed;\n static struct strbuf message;\n \n@@ -132,6 +135,8 @@ static struct option builtin_commit_options[] = {\n \tOPT_BOOLEAN('e', \"edit\", &edit_flag, \"force edit of commit\"),\n \tOPT_STRING(0, \"cleanup\", &cleanup_arg, \"default\", \"how to strip spaces and #comments from message\"),\n \tOPT_BOOLEAN(0, \"status\", &include_status, \"include status in commit message template\"),\n+\t{ OPTION_STRING, 0, \"notes\", &edit_notes, \"ref\", \"edit notes interactively\",\n+\t\tPARSE_OPT_OPTARG, NULL, 1 },\n \t/* end commit message options */\n \n \tOPT_GROUP(\"Commit contents options\"),\n@@ -559,6 +564,68 @@ static char *cut_ident_timestamp_part(char *string)\n \treturn ket;\n }\n \n+static void init_edit_notes() {\n+\tstruct strbuf ref = STRBUF_INIT;\n+\tif (edit_notes_tree.initialized)\n+\t\treturn;\n+\tstrbuf_addstr(&ref, edit_notes);\n+\texpand_notes_ref(&ref);\n+\tinit_notes(&edit_notes_tree, ref.buf,\n+\t\t combine_notes_overwrite, 0);\n+}\n+\n+static void add_notes_from_commit(struct strbuf *out, const char *name)\n+{\n+\tstruct commit *commit;\n+\tstruct strbuf note = STRBUF_INIT;\n+\n+\tinit_edit_notes();\n+\n+\tcommit = lookup_commit_reference_by_name(name);\n+\tif (!commit)\n+\t\tdie(\"could not lookup commit %s\", name);\n+\tformat_note(&edit_notes_tree, commit->object.sha1, ¬e,\n+\t\t get_commit_output_encoding(), 0);\n+\n+\tif (note.len) {\n+\t\tstrbuf_addstr(out, \"\\n---\\n\");\n+\t\tstrbuf_addbuf(out, ¬e);\n+\t}\n+\tstrbuf_release(¬e);\n+}\n+\n+static void extract_notes_from_message(struct strbuf *msg, struct strbuf *notes)\n+{\n+\tconst char *separator = strstr(msg->buf, \"\\n---\\n\");\n+\n+\tif (!separator)\n+\t\treturn;\n+\n+\tstrbuf_addstr(notes, separator + 5);\n+\tstrbuf_setlen(msg, separator - msg->buf + 1);\n+}\n+\n+static void update_notes_for_commit(struct strbuf *notes,\n+\t\t\t\t unsigned char *commit_sha1)\n+{\n+\tinit_edit_notes();\n+\n+\tif (cleanup_mode != CLEANUP_NONE)\n+\t\tstripspace(notes, cleanup_mode == CLEANUP_ALL);\n+\n+\tif (!notes->len)\n+\t\tremove_note(&edit_notes_tree, commit_sha1);\n+\telse {\n+\t\tunsigned char blob_sha1[20];\n+\t\tif (write_sha1_file(notes->buf, notes->len,\n+\t\t\t\t blob_type, blob_sha1) < 0)\n+\t\t\tdie(\"unable to write note blob\");\n+\t\tadd_note(&edit_notes_tree, commit_sha1, blob_sha1,\n+\t\t\t combine_notes_overwrite);\n+\t}\n+\tcommit_notes(&edit_notes_tree, \"updated by commit --notes\");\n+}\n+\n static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t struct wt_status *s,\n \t\t\t struct strbuf *author_ident)\n@@ -682,6 +749,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstrbuf_release(&sob);\n \t}\n \n+\tif (edit_notes && amend)\n+\t\tadd_notes_from_commit(&sb, \"HEAD\");\n+\n \tif (fwrite(sb.buf, 1, sb.len, fp) < sb.len)\n \t\tdie_errno(\"could not write commit template\");\n \n@@ -918,8 +988,13 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\tuse_editor = 0;\n \tif (edit_flag)\n \t\tuse_editor = 1;\n-\tif (!use_editor)\n+\tif (!use_editor) {\n \t\tsetenv(\"GIT_EDITOR\", \":\", 1);\n+\t\tedit_notes = NULL;\n+\t}\n+\t/* Magic value for \"no ref passed\" */\n+\tif (edit_notes == (void *)1)\n+\t\tedit_notes = default_notes_ref();\n \n \tif (get_sha1(\"HEAD\", head_sha1))\n \t\tinitial_commit = 1;\n@@ -1288,6 +1363,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf author_ident = STRBUF_INIT;\n+\tstruct strbuf notes = STRBUF_INIT;\n \tconst char *index_file, *reflog_msg;\n \tchar *nl, *p;\n \tunsigned char commit_sha1[20];\n@@ -1388,6 +1464,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tstrbuf_setlen(&sb, p - sb.buf + 1);\n \t}\n \n+\tif (edit_notes)\n+\t\textract_notes_from_message(&sb, ¬es);\n+\n \tif (cleanup_mode != CLEANUP_NONE)\n \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (message_is_empty(&sb) && !allow_empty_message) {\n@@ -1403,6 +1482,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t}\n \tstrbuf_release(&author_ident);\n \n+\tif (edit_notes)\n+\t\tupdate_notes_for_commit(¬es, commit_sha1);\n+\tstrbuf_release(¬es);\n+\n \tref_lock = lock_any_ref_for_update(\"HEAD\",\n \t\t\t\t\t initial_commit ? NULL : head_sha1,\n \t\t\t\t\t 0);\n@@ -1436,7 +1519,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \trerere(0);\n \trun_hook(get_index_file(), \"post-commit\", NULL);\n-\tif (amend && !no_post_rewrite) {\n+\tif (!edit_notes && amend && !no_post_rewrite) {\n \t\tstruct notes_rewrite_cfg *cfg;\n \t\tcfg = init_copy_notes_for_rewrite(\"amend\");\n \t\tif (cfg) {\ndiff --git a/t/t7510-commit-notes.sh b/t/t7510-commit-notes.sh\nnew file mode 100755\nindex 0000000..ffc0781\n--- /dev/null\n+++ b/t/t7510-commit-notes.sh\n@@ -0,0 +1,183 @@\n+#!/bin/sh\n+\n+test_description='commit w/ --notes'\n+. ./test-lib.sh\n+\n+# Fake editor to simulate user adding a note.\n+cat >add.sh <<'EOF'\n+perl -i -pe '\n+ BEGIN { $n = shift }\n+ # insert at $n-th blank line\n+ if (/^$/ && ++$count == $n) {\n+\t print \"---\\n\";\n+\t print \"added note\\n\";\n+\t print \"with multiple lines\\n\";\n+ }\n+' \"$@\"\n+EOF\n+cat >expect-add <<'EOF'\n+added note\n+with multiple lines\n+EOF\n+\n+# Fake editor to simulate user deleting a note.\n+cat >del.sh <<'EOF'\n+perl -i -ne '\n+ if (/^---$/) {\n+\t while (<>) {\n+\t\t last if /^$/;\n+\t }\n+\t next;\n+ }\n+ print;\n+' \"$1\"\n+EOF\n+\n+# Fake editor to simulate user modifying a note.\n+cat >mod.sh <<'EOF'\n+perl -i -pe '\n+ s/added note/modified note/\n+' \"$1\"\n+EOF\n+cat >expect-mod <<'EOF'\n+modified note\n+with multiple lines\n+EOF\n+\n+# Fake editor for leaving notes untouched.\n+cat >nil.sh <<'EOF'\n+EOF\n+\n+# Convenience function for setting up editor.\n+set_editor() {\n+\tgit config core.editor \"\\\"$SHELL_PATH\\\" $1.sh $2\"\n+}\n+\n+cat >expect-msg <<'EOF'\n+commit one\n+\n+this is the body of commit one\n+EOF\n+\n+test_expect_success 'setup' '\n+\ttest_commit one &&\n+\tgit commit --amend -F expect-msg\n+'\n+\n+test_expect_success 'add a note' '\n+\tset_editor add 2 &&\n+\tgit commit --notes --amend &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-add actual &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect-msg actual\n+'\n+\n+test_expect_success 'notes are preserved' '\n+\tset_editor nil &&\n+\tgit commit --notes --amend &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-add actual &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect-msg actual\n+'\n+\n+test_expect_success 'modify a note' '\n+\tset_editor mod &&\n+\tgit commit --notes --amend &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-mod actual &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect-msg actual\n+'\n+\n+test_expect_success 'delete a note' '\n+\tset_editor del &&\n+\tgit commit --notes --amend &&\n+\ttest_must_fail git notes show &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect-msg actual\n+'\n+\n+test_expect_success 'add to commit without body' '\n+\ttest_commit two &&\n+\tgit log -1 --pretty=format:%B >expect-msg &&\n+\tset_editor add 1 &&\n+\tgit commit --notes --amend &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-add actual &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect-msg actual\n+'\n+\n+cat >expect-verbatim-msg <<'EOF'\n+verbatim subject\n+\n+verbatim body\n+# embedded comment\n+\n+EOF\n+cat >expect-verbatim-note <<'EOF'\n+\n+verbatim note\n+with leading and trailing whitespace\n+# and embedded comments\n+\n+EOF\n+cat >verbatim.sh <<'EOF'\n+{\n+\tcat expect-verbatim-msg &&\n+\techo --- &&\n+\tcat expect-verbatim-note\n+} >\"$1\"\n+EOF\n+\n+test_expect_success 'commit --cleanup=verbatim preserves message and notes' '\n+\ttest_tick &&\n+\techo content >>file && git add file &&\n+\tset_editor verbatim &&\n+\tgit commit --notes --cleanup=verbatim &&\n+\tgit cat-file commit HEAD >msg.raw &&\n+\tsed \"1,/^\\$/d\" actual &&\n+\ttest_cmp expect-verbatim-msg actual &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-verbatim-note actual\n+'\n+\n+test_expect_success 'commit -v does not interfere with notes' '\n+\ttest_commit three &&\n+\tgit log -1 --pretty=format:%B >expect-msg\n+\tset_editor add 1 &&\n+\tgit commit -v --notes --amend &&\n+\tgit notes show >actual &&\n+\ttest_cmp actual expect-add &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect-msg actual\n+'\n+\n+test_expect_success 'edit notes on alternate ref' '\n+\ttest_commit four &&\n+\tset_editor add 1 &&\n+\tgit commit --notes=foo --amend &&\n+\ttest_must_fail git notes show &&\n+\tgit notes --ref refs/notes/foo show >actual &&\n+\ttest_cmp expect-add actual\n+'\n+\n+test_expect_success 'ref rewriting does not overwrite our edits' '\n+\tgit config notes.rewriteRef refs/notes/commits &&\n+\ttest_commit five &&\n+\tset_editor add 1 &&\n+\tgit commit --notes --amend &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-add actual &&\n+\tset_editor mod &&\n+\tgit commit --notes --amend &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-mod actual &&\n+\tset_editor del &&\n+\tgit commit --notes --amend &&\n+\ttest_must_fail git notes show\n+'\n+\n+test_done\n-- \n1.7.4.31.g76e18\n"},{"id":"162966","messageId":"201103080925.05761.johan@herland.net","threadId":"26601","inReplyTo":"20110307233956.GA20912@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] notes: make expand_notes_ref globally accessible","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-03-08T08:25:05Z","receivedAt":"2011-03-08T08:25:05Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 08 March 2011, Jeff King wrote:\n> This function is useful for other commands besides \"git\n> notes\" which want to let users refer to notes by their\n> shorthand name.\n> \n> Signed-off-by: Jeff King \n\nAcked-by: Johan Herland \n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"162975","messageId":"201103081015.24474.johan@herland.net","threadId":"26601","inReplyTo":"20110307234138.GB20912@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] commit: allow editing notes in commit message editor","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-03-08T09:15:24Z","receivedAt":"2011-03-08T09:15:24Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 08 March 2011, Jeff King wrote:\n> Changes from v1:\n> \n> - fix bug with adding notes to new commit (we failed to\n> initialize the notes tree properly in this case)\n> \n> - you can now do \"commit --notes=foo\" to view/edit\n> refs/notes/foo\n\nNice. :)\n\n> - added tests for basic operations, plus interaction with\n> --cleanup and -v\n> \n> - turn off commit rewriting when we edit\n> \n> Todo:\n> \n> - commit.notes config variable to have this on all the time\n> \n> - I punted on the separator decision here.\n\nWe probably want to make it configurable, as mentioned earlier in the \nthread. Still, making it configurable gives me the somewhat uneasy feeling \nthat we're \"blaming\" the user for any false positives (\"It's your fault for \nnot choosing a more unique separator...\")...\n\nWhat if we start the separator with a comment character (e.g. \"# ---\"). That \nway, the user could not expect a false positive to make it into the commit \nmessage in the first place (since it'd be stripped along with other \ncomments). Of course, we'd have to make sure that the notes separator was \nparsed before removing the comments, but I think that's already taken care \nof in the patch below.\n\n> - probably still some magic needed for rebase conflict\n> case; we will be making a new commit, so we don't know\n> to pull the notes in from the old commit as we do with\n> --amend.\n\nMaybe add a \"--notes-copy=\" argument to \"git commit\" that causes \n\"\" to be passed to add_notes_from_commit(). Of course, in the case \nof --amend, the default is \"--notes-copy=HEAD\".\n\n> - still needs the format-patch component to make the\n> workflow complete :)\n> \n> builtin/commit.c | 87 ++++++++++++++++++++++-\n> t/t7510-commit-notes.sh | 183 +++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 268 insertions(+), 2 deletions(-)\n> create mode 100755 t/t7510-commit-notes.sh\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index d71e1e0..f84ca23 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n\n[...]\n\n> @@ -559,6 +564,68 @@ static char *cut_ident_timestamp_part(char *string)\n> \treturn ket;\n> }\n> \n> +static void init_edit_notes() {\n\nstyle nit: move \"{\" to next line.\n\n[...]\n\n> +static void update_notes_for_commit(struct strbuf *notes,\n> +\t\t\t\t unsigned char *commit_sha1)\n> +{\n> +\tinit_edit_notes();\n> +\n> +\tif (cleanup_mode != CLEANUP_NONE)\n> +\t\tstripspace(notes, cleanup_mode == CLEANUP_ALL);\n> +\n> +\tif (!notes->len)\n> +\t\tremove_note(&edit_notes_tree, commit_sha1);\n> +\telse {\n> +\t\tunsigned char blob_sha1[20];\n> +\t\tif (write_sha1_file(notes->buf, notes->len,\n> +\t\t\t\t blob_type, blob_sha1) < 0)\n> +\t\t\tdie(\"unable to write note blob\");\n> +\t\tadd_note(&edit_notes_tree, commit_sha1, blob_sha1,\n> +\t\t\t combine_notes_overwrite);\n\nWe may want to consider adding a small convenience function to the notes API \nfor turning a strbuf into a notes blob. (Maybe s/strbuf/char* + len/ to \ncater for binary notes blobs as well.) This would move some low-level \ndetails (#include \"blob.h\", and write_sha1_file(...)) out of the notes API \nusers' code.\n\n\nOtherwise, this looks really good.\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"162985","messageId":"AANLkTi=ybDr61jH2J+sZq3r+zeyN2KxdguGpKH67wrAe@mail.gmail.com","threadId":"26601","inReplyTo":"20110307233902.GA20447@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Michel Lespinasse","fromEmail":"walken@google.com","sentAt":"2011-03-08T12:39:48Z","receivedAt":"2011-03-08T12:39:48Z","isPatch":true,"sender":{"key":"walken@google.com","avatar":null},"body":"On Mon, Mar 7, 2011 at 3:39 PM, Jeff King wrote:\n> Yeah, that's my biggest concern. I just really foresee myself getting\n> annoyed by typing \"--- nOtes ---\", or \"-- Notes ---\". It's just a few\n> characters shorter, but \"---\" is really less error prone.\n\ngit often puts comment lines lines (starting with '#') around the\ncommit message. How about just adding one more such comment:\n\n# Lines after this one will be added as git notes\n\nand make it so that any non-empty line entered afterwards does get\nadded as notes ?\n\n\nAlso, I would love to see this functionality in other places such as\nrebasing (if the original commit has notes attached, I would like\nthese to show up so I can attach these to the rebased commit as well).\nI realize not everybody would want that, but this could be easily\ncontrolled with message hooks...\n\n-- \nMichel \"Walken\" Lespinasse\nA program is never fully debugged until the last user dies.\n"},{"id":"163050","messageId":"20110309091307.4b759b7e@chalon.bertin.fr","threadId":"26601","inReplyTo":"20110225133056.GA1026@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] commit notes workflow","fromName":"Yann Dirson","fromEmail":"dirson@bertin.fr","sentAt":"2011-03-09T08:13:07Z","receivedAt":"2011-03-09T08:13:07Z","isPatch":true,"sender":{"key":"dirson@bertin.fr","avatar":null},"body":"That's a nice feature.\n\nIt may be good to extend the idea to support editing non-default notes\nrefs too. Maybe something like:\n\n\n--- Notes ---\n\n--- Notes ---\n\n\nIn this case, if we want to allow the user to customize the mark, we\nwill want to allow formatting like \"--- Notes %N ---\", but then the\ndefaulting for GIT_NOTES_REF would not fit - would we want to force the\nuse of \"--- Notes commits ---\" or similar ? Maybe this would warrant a\nseparate mark for this default case:\n\n\t--default-note-mark=\"---\"\n\t--note-mark=\"--- %N ---\"\n\nOTOH, using a single --note-mark and no special case for the default\nnotes ref seems more sane to me, since that shows the user when a\nnon-default GIT_NOTES_REF is in effect.\n\nWe may also want it to behave in a way similar to git-log, including\n--show-notes[=] support to override the list of notes ref\nto be considered.\n\n-- \nYann Dirson - Bertin Technologies\n"}]}