{"thread":{"id":"30052","subject":"[PATCH] rev-list: fix place holder %N (notes) in user format","startedAt":"2012-03-24T19:38:31Z","lastAt":"2012-07-19T17:25:50Z","messageCount":15,"participants":["Jukka Lehtniemi","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"187651","messageId":"1332617911-8281-1-git-send-email-jukka.lehtniemi@gmail.com","threadId":"30052","inReplyTo":null,"subject":"[PATCH] rev-list: fix place holder %N (notes) in user format","fromName":"Jukka Lehtniemi","fromEmail":"jukka.lehtniemi@gmail.com","sentAt":"2012-03-24T19:38:31Z","receivedAt":"2012-03-24T19:38:31Z","isPatch":true,"sender":{"key":"jukka.lehtniemi@gmail.com","avatar":null},"body":"\nSigned-off-by: Jukka Lehtniemi \n---\n\n Fixes a bug where the place holder for notes (%N) was not expanded \n in rev-list. To reproduce the bug:\n $ git notes add -m foo\n $ git rev-list --notes --format=format:%N HEAD ^HEAD^\n\n\n builtin/rev-list.c | 4 +++-\n t/t6006-rev-list-format.sh | 12 ++++++++++--\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 4c4d404..d6e7dfc 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -110,6 +110,7 @@ static void show_commit(struct commit *commit, void *data)\n \t\tctx.abbrev = revs->abbrev;\n \t\tctx.date_mode = revs->date_mode;\n \t\tctx.fmt = revs->commit_format;\n+\t\tctx.show_notes = revs->show_notes;\n \t\tpretty_print_commit(&ctx, commit, &buf);\n \t\tif (revs->graph) {\n \t\t\tif (buf.len) {\n@@ -323,7 +324,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tinfo.revs = &revs;\n \tif (revs.bisect)\n \t\tbisect_list = 1;\n-\n+\tif (revs.show_notes)\n+\t\tinit_display_notes(&revs.notes_opt);\n \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n \t\tinfo.flags |= REV_LIST_QUIET;\n \tfor (i = 1 ; i < argc; i++) {\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex 4442790..76af40a 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -7,18 +7,26 @@ test_description='git rev-list --pretty=format test'\n test_tick\n test_expect_success 'setup' '\n touch foo && git add foo && git commit -m \"added foo\" &&\n- echo changed >foo && git commit -a -m \"changed foo\"\n+ echo changed >foo && git commit -a -m \"changed foo\" &&\n+ git notes add -m \"note foo\"\n '\n \n # usage: test_format name format_string expect.$1\n \ttest_expect_success \"format $1\" \"\n-git rev-list --pretty=format:'$2' master >output.$1 &&\n+git rev-list --notes --pretty=format:'$2' master >output.$1 &&\n test_cmp expect.$1 output.$1\n \"\n }\n \n+test_format notes %N <<'EOF'\n+commit 131a310eb913d107dd3c09a65d1651175898735d\n+note foo\n+\n+commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873\n+EOF\n+\n test_format percent %%h <<'EOF'\n commit 131a310eb913d107dd3c09a65d1651175898735d\n %h\n-- \n1.7.4.1\n"},{"id":"187661","messageId":"20120325005504.GA27651@sigill.intra.peff.net","threadId":"30052","inReplyTo":"1332617911-8281-1-git-send-email-jukka.lehtniemi@gmail.com","subject":"Re: [PATCH] rev-list: fix place holder %N (notes) in user format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-25T00:55:04Z","receivedAt":"2012-03-25T00:55:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 24, 2012 at 09:38:31PM +0200, Jukka Lehtniemi wrote:\n\n> Signed-off-by: Jukka Lehtniemi \n> ---\n> \n> Fixes a bug where the place holder for notes (%N) was not expanded \n> in rev-list. To reproduce the bug:\n> $ git notes add -m foo\n> $ git rev-list --notes --format=format:%N HEAD ^HEAD^\n\nThis explanation should probably go in the commit message.\n\nI am not sure the behavior afterwards is right, though. With git-log, I\nsee the following behaviors:\n\n 1. With --notes, notes are displayed.\n\n 3. With --no-notes, notes are not displayed.\n\n 3. With no options, notes are displayed (i.e., we default to --notes).\n\n 4. With --format=%N, notes are displayed (automatically, without\n having to use \"--notes\").\n\nShouldn't rev-list behave exactly the same way, except for (3)? That is,\nit should respect the options in the same way, but default to not\nshowing notes?\n\nEven with your patch, we seem to be violating points (1) and (4).\n\"--notes\" does nothing by itself, and you need to use it to make \"%N\"\nwork.\n\nFor (4), you need to use userformat_find_requirement, as git-log does.\nI'm not sure without digging how to make (1) work.\n\n-Peff\n"},{"id":"195142","messageId":"1342463409-6919-1-git-send-email-jukka.lehtniemi@gmail.com","threadId":"30052","inReplyTo":"20120325005504.GA27651@sigill.intra.peff.net","subject":"[PATCH v2] Fix notes handling in rev-list","fromName":"Jukka Lehtniemi","fromEmail":"jukka.lehtniemi@gmail.com","sentAt":"2012-07-16T18:30:09Z","receivedAt":"2012-07-16T18:30:09Z","isPatch":true,"sender":{"key":"jukka.lehtniemi@gmail.com","avatar":null},"body":"Display notes in the rev-list when switch '--notes' is used.\nAlso expand notes place holder (%N) in user format.\nPreviously rev-list ignored both of these.\n\nSigned-off-by: Jukka Lehtniemi \n---\n\nThanks for your feedback Peff!\n\n builtin/rev-list.c | 16 +++++++++++++++-\n t/t6006-rev-list-format.sh | 22 +++++++++++++++++++++-\n 2 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff5a383..ad8abcb 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -7,6 +7,7 @@\n #include \"log-tree.h\"\n #include \"graph.h\"\n #include \"bisect.h\"\n+#include \"notes.h\"\n \n static const char rev_list_usage[] =\n \"git rev-list [OPTION] ... [ -- paths... ]\\n\"\n@@ -111,6 +112,7 @@ static void show_commit(struct commit *commit, void *data)\n \t\tctx.date_mode = revs->date_mode;\n \t\tctx.date_mode_explicit = revs->date_mode_explicit;\n \t\tctx.fmt = revs->commit_format;\n+\t\tctx.show_notes = revs->show_notes;\n \t\tpretty_print_commit(&ctx, commit, &buf);\n \t\tif (revs->graph) {\n \t\t\tif (buf.len) {\n@@ -159,6 +161,12 @@ static void show_commit(struct commit *commit, void *data)\n \t} else {\n \t\tif (graph_show_remainder(revs->graph))\n \t\t\tputchar('\\n');\n+\t\tif (revs->show_notes_given) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tformat_display_notes(commit->object.sha1, &buf, 0, NOTES_SHOW_HEADER|NOTES_INDENT); \n+\t\t\tfwrite(buf.buf, 1, buf.len, stdout);\n+\t\t\tstrbuf_release(&buf);\n+\t\t}\n \t}\n \tmaybe_flush_or_die(stdout, \"stdout\");\n \tfinish_commit(commit, data);\n@@ -309,6 +317,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n \tstruct rev_list_info info;\n+\tstruct userformat_want userformat_want = {0};\n \tint i;\n \tint bisect_list = 0;\n \tint bisect_show_vars = 0;\n@@ -322,9 +331,14 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n \tmemset(&info, 0, sizeof(info));\n \tinfo.revs = &revs;\n+\n+\tuserformat_find_requirements(NULL, &userformat_want);\n+\tif (userformat_want.notes)\n+\t\trevs.show_notes = 1;\n \tif (revs.bisect)\n \t\tbisect_list = 1;\n-\n+\tif (revs.show_notes)\n+\t\tinit_display_notes(&revs.notes_opt);\n \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n \t\tinfo.flags |= REV_LIST_QUIET;\n \tfor (i = 1 ; i < argc; i++) {\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex f94f0c4..ab616a0 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -7,7 +7,8 @@ test_description='git rev-list --pretty=format test'\n test_tick\n test_expect_success 'setup' '\n touch foo && git add foo && git commit -m \"added foo\" &&\n- echo changed >foo && git commit -a -m \"changed foo\"\n+ echo changed >foo && git commit -a -m \"changed foo\" &&\n+ git notes add -m \"note foo\"\n '\n \n # usage: test_format name format_string expect.notes_switch <<'EOF'\n+131a310eb913d107dd3c09a65d1651175898735d\n+\n+Notes:\n+ note foo\n+86c75cfd708a0e5868dc876ed5b8bb66c80b4873\n+EOF\n+git rev-list --notes master >output.notes_switch &&\n+test_cmp expect.notes_switch output.notes_switch\n+\"\n+\n+test_format notes %N <<'EOF'\n+commit 131a310eb913d107dd3c09a65d1651175898735d\n+note foo\n+\n+commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873\n+EOF\n+\n test_format percent %%h <<'EOF'\n commit 131a310eb913d107dd3c09a65d1651175898735d\n %h\n-- \n1.7.4.1\n"},{"id":"195145","messageId":"7vipdn1qb7.fsf@alter.siamese.dyndns.org","threadId":"30052","inReplyTo":"1342463409-6919-1-git-send-email-jukka.lehtniemi@gmail.com","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-16T19:03:40Z","receivedAt":"2012-07-16T19:03:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jukka Lehtniemi writes:\n\n> Display notes in the rev-list when switch '--notes' is used.\n> Also expand notes place holder (%N) in user format.\n> Previously rev-list ignored both of these.\n>\n> Signed-off-by: Jukka Lehtniemi \n> ---\n>\n> Thanks for your feedback Peff!\n\nIf it is an update for some old patch (I am guessing that is the\ncase from \"v2\" and \"feedback\" above), please hint where the\noriginal can be found not to waste reviewers' time.\n\nAs \"git rev-list -h\" does not say anything about \"notes\", I do not\nthink this should be even called \"Fix\"; rather it is \"teach rev-list\nto show notes with --notes\", a new feature.\n\nAnd as a new feature, \"git rev-list -h\" should be taught to include\nthis new option in its output. I didn't check the documentation but\nyou may also want to add --notes there, too (hint: grep for \"--pretty\"\nto find where you may need to add the new option).\n"},{"id":"195182","messageId":"20120717031727.GA20945@sigill.intra.peff.net","threadId":"30052","inReplyTo":"7vipdn1qb7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-17T03:17:28Z","receivedAt":"2012-07-17T03:17:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2012 at 12:03:40PM -0700, Junio C Hamano wrote:\n\n> Jukka Lehtniemi writes:\n> \n> > Display notes in the rev-list when switch '--notes' is used.\n> > Also expand notes place holder (%N) in user format.\n> > Previously rev-list ignored both of these.\n> >\n> > Signed-off-by: Jukka Lehtniemi \n> > ---\n> >\n> > Thanks for your feedback Peff!\n> \n> If it is an update for some old patch (I am guessing that is the\n> case from \"v2\" and \"feedback\" above), please hint where the\n> original can be found not to waste reviewers' time.\n\nAgreed. For reference, v1 is here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/193842\n\n> As \"git rev-list -h\" does not say anything about \"notes\", I do not\n> think this should be even called \"Fix\"; rather it is \"teach rev-list\n> to show notes with --notes\", a new feature.\n\nIt does not, but \"git rev-list --help\" does (and it mentions \"%N\" for\nthe pretty userformat). So it's debatable whether it is a code bug or a\ndocumentation bug. But whatever we call it, I think it is an\nimprovement.\n\n> And as a new feature, \"git rev-list -h\" should be taught to include\n> this new option in its output. I didn't check the documentation but\n> you may also want to add --notes there, too (hint: grep for \"--pretty\"\n> to find where you may need to add the new option).\n\n\"rev-list -h\" is already an unwieldy 35 lines, yet still manages to miss\nmany options (e.g., --grep, --author, --cherry-*, variations of\n--left-right, --boundary, history simplification options like\n--full-history, and so on). I don't think one more option is going to\nbreak the camel's back, but I wonder if \"rev-list -h\" could use some\ncleanup. E.g., maybe drop seldom used stuff like --bisect-vars, format\nsimilar options on a single line to save space, and add in some missing\noptions.\n\nMy preference would actually be to just give up and refer people to the\nmanpage after a one or two line usage. But I think we have had that\ndiscussion before and you did not agree.\n\n-Peff\n"},{"id":"195184","messageId":"7v1ukbys0n.fsf@alter.siamese.dyndns.org","threadId":"30052","inReplyTo":"20120717031727.GA20945@sigill.intra.peff.net","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-17T03:40:24Z","receivedAt":"2012-07-17T03:40:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... But whatever we call it, I think it is an\n> improvement.\n\nI didn't say it makes things worse in any way, did I?\n\nI was reacting on the Subject: line because that will what I later\nhave to work from when reading shortlog, summarizing changes, etc.\n\n> ... I don't think one more option is going to\n> break the camel's back, but I wonder if \"rev-list -h\" could use some\n> cleanup. E.g., maybe drop seldom used stuff like --bisect-vars, format\n> similar options on a single line to save space, and add in some missing\n> options.\n\nSounds good. The manpage also needs some cleaning up, I would\nthink, to omit things that may happen to work (primarily because a\nlot of code is shared with \"log\") but do not have to, for example.\n"},{"id":"195185","messageId":"20120717034640.GB20945@sigill.intra.peff.net","threadId":"30052","inReplyTo":"1342463409-6919-1-git-send-email-jukka.lehtniemi@gmail.com","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-17T03:46:40Z","receivedAt":"2012-07-17T03:46:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2012 at 09:30:09PM +0300, Jukka Lehtniemi wrote:\n\n> @@ -111,6 +112,7 @@ static void show_commit(struct commit *commit, void *data)\n> \t\tctx.date_mode = revs->date_mode;\n> \t\tctx.date_mode_explicit = revs->date_mode_explicit;\n> \t\tctx.fmt = revs->commit_format;\n> +\t\tctx.show_notes = revs->show_notes;\n> \t\tpretty_print_commit(&ctx, commit, &buf);\n> \t\tif (revs->graph) {\n> \t\t\tif (buf.len) {\n\nMakes sense. We were just failing to propagate the show_notes flag to\nthe pretty-print code, as log-tree does.\n\n> @@ -159,6 +161,12 @@ static void show_commit(struct commit *commit, void *data)\n> \t} else {\n> \t\tif (graph_show_remainder(revs->graph))\n> \t\t\tputchar('\\n');\n> +\t\tif (revs->show_notes_given) {\n> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\t\tformat_display_notes(commit->object.sha1, &buf, 0, NOTES_SHOW_HEADER|NOTES_INDENT); \n> +\t\t\tfwrite(buf.buf, 1, buf.len, stdout);\n> +\t\t\tstrbuf_release(&buf);\n> +\t\t}\n\nBut why are we using show_notes_given here instead of show_notes? The\nformer is about \"did we get any kind of --notes option on the\ncommand-line\". So doing \"git rev-list --no-notes\" would trigger it,\nwhich seems wrong. We should simply be checking show_notes again, no?\n\nAlso, it seems odd to me to show the notes after graph_show_remainder.\nYour first hunk is about passing the notes option to the pretty-printer,\nwhich handles graph output already, and looks like this:\n\n $ git rev-list --oneline --graph --notes -2 HEAD\n * f6bbb09 Fix notes handling in rev-list\n | Notes:\n | foobar\n | \n * 31c7954 Update draft release notes for 7th batch\n\nJust like log, the notes are part of the commit information to the right\nof the graph. But this second hunk is for when we are not using the\npretty-printer at all, and the output looks like this:\n\n $ git rev-list --graph --notes -2 HEAD\n * f6bbb09529a4cc73446c7c115ac1468477bd0cc6\n\n Notes:\n foobar\n * 31c79549b85c6393be4f40432f5b86ebc097fc7e\n\nwhich doesn't make sense; we've broken the graph with our notes output.\nI think you would need to feed the output to the graph code. Something\nlike:\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff5a383..dbe7349 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -157,6 +159,13 @@ static void show_commit(struct commit *commit, void *data)\n \t\t}\n \t\tstrbuf_release(&buf);\n \t} else {\n+\t\tif (revs->show_notes) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tformat_display_notes(commit->object.sha1, &buf, 0,\n+\t\t\t\t\t NOTES_SHOW_HEADER|NOTES_INDENT);\n+\t\t\tgraph_show_commit_msg(revs->graph, &buf);\n+\t\t\tstrbuf_release(&buf);\n+\t\t}\n \t\tif (graph_show_remainder(revs->graph))\n \t\t\tputchar('\\n');\n \t}\n\nExcept that it seems to introduce an extra newline before the notes, and\nit is probably an abuse of graph_show_commit_msg (there is a\ngraph_show_strbuf which graph_show_commit_msg is built around, and it\ncould perhaps be made public). I'd look at how log-tree handles it for\ninspiration.\n\n> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\n> index f94f0c4..ab616a0 100755\n> --- a/t/t6006-rev-list-format.sh\n> +++ b/t/t6006-rev-list-format.sh\n> [...]\n> @@ -19,6 +20,25 @@ test_cmp expect.$1 output.$1\n> \"\n> }\n> \n> +test_expect_success 'notes switch' \"\n> +cat >expect.notes_switch <<'EOF'\n> +131a310eb913d107dd3c09a65d1651175898735d\n> +\n> +Notes:\n> + note foo\n> +86c75cfd708a0e5868dc876ed5b8bb66c80b4873\n> +EOF\n> +git rev-list --notes master >output.notes_switch &&\n> +test_cmp expect.notes_switch output.notes_switch\n> +\"\n> +\n> +test_format notes %N <<'EOF'\n> +commit 131a310eb913d107dd3c09a65d1651175898735d\n> +note foo\n> +\n> +commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873\n> +EOF\n> +\n\nNice. Of the four behaviors I mentioned in my previous review:\n\n 1. --notes shows notes\n\n 2. --no-notes does not show notes\n\n 3. when no options are given, notes are not displayed\n\n 4. --format=%N shows notes\n\nthis tests 2 of them (1 and 4). I don't know if it is worth testing (2),\nas it should not do anything (though it actually is broken with your\npatch). It is definitely worth making sure (3) works, but it is\nimplicitly checked by the other tests in the script (which would break\nif we started showing notes). Still, it might be worth adding an\nexplicit \"rev-list does not show notes by default\" to make sure it is\nclearly documented with a test.\n\n-Peff\n"},{"id":"195186","messageId":"20120717035138.GC20945@sigill.intra.peff.net","threadId":"30052","inReplyTo":"7v1ukbys0n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-17T03:51:38Z","receivedAt":"2012-07-17T03:51:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2012 at 08:40:24PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... But whatever we call it, I think it is an\n> > improvement.\n> \n> I didn't say it makes things worse in any way, did I?\n\nNo, you did not. That was my attempt to end the paragraph on a positive\nand constructive note. What I meant was \"I think you are wrong and it is\na bugfix, but I do not care overly what we call it as long as we do it.\"\n:)\n\nDo note that my \"as long as we do it\" is about the direction, not the\npatch. There are a few issues with the patch itself, and I just posted a\nreview.\n\n> I was reacting on the Subject: line because that will what I later\n> have to work from when reading shortlog, summarizing changes, etc.\n\nThat is a fair point. From the release notes perspective, it probably is\na feature (there is a documentation bug, but it happens to be fixed at\nthe same time).\n\n-Peff\n"},{"id":"195189","messageId":"7vvchnx7tc.fsf@alter.siamese.dyndns.org","threadId":"30052","inReplyTo":"20120717034640.GB20945@sigill.intra.peff.net","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-17T05:42:07Z","receivedAt":"2012-07-17T05:42:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 16, 2012 at 09:30:09PM +0300, Jukka Lehtniemi wrote:\n>\n>> @@ -111,6 +112,7 @@ static void show_commit(struct commit *commit, void *data)\n>> \t\tctx.date_mode = revs->date_mode;\n>> \t\tctx.date_mode_explicit = revs->date_mode_explicit;\n>> \t\tctx.fmt = revs->commit_format;\n>> +\t\tctx.show_notes = revs->show_notes;\n>> \t\tpretty_print_commit(&ctx, commit, &buf);\n>> \t\tif (revs->graph) {\n>> \t\t\tif (buf.len) {\n>\n> Makes sense. We were just failing to propagate the show_notes flag to\n> the pretty-print code, as log-tree does.\n>\n>> @@ -159,6 +161,12 @@ static void show_commit(struct commit *commit, void *data)\n>> \t} else {\n>> \t\tif (graph_show_remainder(revs->graph))\n>> \t\t\tputchar('\\n');\n>> +\t\tif (revs->show_notes_given) {\n>> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n>> +\t\t\tformat_display_notes(commit->object.sha1, &buf, 0, NOTES_SHOW_HEADER|NOTES_INDENT); \n>> +\t\t\tfwrite(buf.buf, 1, buf.len, stdout);\n>> +\t\t\tstrbuf_release(&buf);\n>> +\t\t}\n>\n> But why are we using show_notes_given here instead of show_notes? The\n> former is about \"did we get any kind of --notes option on the\n> command-line\". So doing \"git rev-list --no-notes\" would trigger it,\n> which seems wrong. We should simply be checking show_notes again, no?\n>\n> Also, it seems odd to me to show the notes after graph_show_remainder.\n> Your first hunk is about passing the notes option to the pretty-printer,\n> which handles graph output already, and looks like this:\n>\n> $ git rev-list --oneline --graph --notes -2 HEAD\n> * f6bbb09 Fix notes handling in rev-list\n> | Notes:\n> | foobar\n> | \n> * 31c7954 Update draft release notes for 7th batch\n>\n> Just like log, the notes are part of the commit information to the right\n> of the graph. But this second hunk is for when we are not using the\n> pretty-printer at all, and the output looks like this:\n>\n> $ git rev-list --graph --notes -2 HEAD\n> * f6bbb09529a4cc73446c7c115ac1468477bd0cc6\n>\n> Notes:\n> foobar\n> * 31c79549b85c6393be4f40432f5b86ebc097fc7e\n>\n> which doesn't make sense\n\nI actually have quite a different feeling about this. As I said in\nthe separate message, I think --graph, or anything that makes the\noutput unparsable or harder to parse for machines for that matter,\nin rev-list are not something we have because we wanted to support\nthem, but that which just happen to work because the large part of\nrev-list and log can share building blocks to do similar things.\nThe key phrase is \"can share\" here; it does not necessarily mean\nthey \"should\" [*1*].\n\nFirst and foremost, rev-list is a tool for people who hate what our\nvanilla \"git log\" Porcelain does enough that they want to write\ntheir own Porcelain scripts using it.\n\nI do not mind having an option to show the notes text, but I doubt\nit is a sane thing to do to make \"rev-list --notes\" unconditionally\nshow the payload of the notes blob. \"rev-list --objects\" only shows\nthe object names of trees and blobs, not the payload in these\nobjects, and this is very much on purpuse. It allows the downstream\nprocess that reads its output from the pipe to easily parse the\noutput and choose to do whatever it wants to do using them.\n\nI wonder if we should show the blob object names that store the\nnotes payload if we were given --notes option in a format that is\neasy for readers to mechanically parse its output.\n\nIn any case, the use of format_display_notes() that is meant for\nhuman consumption feels very wrong to me, especially it seems to be\nplaced outside the \"if (revs->verbose_header && commit->buffer)\"\nblock in this patch. I do not have any problem if the patch makes\nthe notes text shown in the other side of the if block that uses\npretty_print_commit(), though.\n\n\n[Footnote]\n\n*1* A simple litmus test is to ask this question: if somebody comes\n up with a \"better\" way to show the same output for the option,\n would we accept that update without worrying about breaking\n existing scripts? If the answer is yes, that is a secondary\n feature in the context of \"rev-list\" plumbing like --graph is.\n"},{"id":"195221","messageId":"CAOQ7pMO8w79B1kpztv4a+x8oCSzL4b8t7STyQu2i68ZT6qk_wQ@mail.gmail.com","threadId":"30052","inReplyTo":"7vvchnx7tc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Jukka Lehtniemi","fromEmail":"jukka.lehtniemi@gmail.com","sentAt":"2012-07-17T21:22:32Z","receivedAt":"2012-07-17T21:22:32Z","isPatch":true,"sender":{"key":"jukka.lehtniemi@gmail.com","avatar":null},"body":"First of all, thanks for you feedback, both of you. And sorry for\nwasting your time . I thought that the \"In-Reply-To\"-header would\nserve as a reference to the original patch but obviously it wasn't\nenough.\n\nOn Tue, Jul 17, 2012 at 8:42 AM, Junio C Hamano wrote:\n>\n> I wonder if we should show the blob object names that store the\n> notes payload if we were given --notes option in a format that is\n> easy for readers to mechanically parse its output.\n>\n\nVery good point indeed. I think this is how it should be. How would\nyou prefer the output format to be? Would e.g.\n\n 189899d229ec Notes: 888ecad77e88\n\nbe ok?\n\n> In any case, the use of format_display_notes() that is meant for\n> human consumption feels very wrong to me, especially it seems to be\n> placed outside the \"if (revs->verbose_header && commit->buffer)\"\n> block in this patch. I do not have any problem if the patch makes\n> the notes text shown in the other side of the if block that uses\n> pretty_print_commit(), though.\n>\n\nI think that another place where human readable notes should be shown\nis inside the graph.\n\n-- \nJukka\n"},{"id":"195253","messageId":"20120718072104.GB12942@sigill.intra.peff.net","threadId":"30052","inReplyTo":"7vvchnx7tc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-18T07:21:04Z","receivedAt":"2012-07-18T07:21:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2012 at 10:42:07PM -0700, Junio C Hamano wrote:\n\n> > Just like log, the notes are part of the commit information to the right\n> > of the graph. But this second hunk is for when we are not using the\n> > pretty-printer at all, and the output looks like this:\n> >\n> > $ git rev-list --graph --notes -2 HEAD\n> > * f6bbb09529a4cc73446c7c115ac1468477bd0cc6\n> >\n> > Notes:\n> > foobar\n> > * 31c79549b85c6393be4f40432f5b86ebc097fc7e\n> >\n> > which doesn't make sense\n> \n> I actually have quite a different feeling about this. As I said in\n> the separate message, I think --graph, or anything that makes the\n> output unparsable or harder to parse for machines for that matter,\n> in rev-list are not something we have because we wanted to support\n> them, but that which just happen to work because the large part of\n> rev-list and log can share building blocks to do similar things.\n> The key phrase is \"can share\" here; it does not necessarily mean\n> they \"should\" [*1*].\n\nSomebody went to the trouble to make \"rev-list --graph\" work[1] (that is\nwhat the call to graph_show_remainder in the else clause of the\nconditional is about). I agree it seems kind of useless, but it does\nwork now, and we should at least not make it worse (and I think we both\nagree that the output above is just wrong).\n\nSo whatever we show for a note, it should look like:\n\n * f6bbb095...\n | the notes thing to show\n * 31c79549...\n\nBecause that is how graph output is formatted. Either that, or we should\ndisallow --graph entirely with rev-list (which I'd also be OK with).\n\n> I do not mind having an option to show the notes text, but I doubt\n> it is a sane thing to do to make \"rev-list --notes\" unconditionally\n> show the payload of the notes blob. \"rev-list --objects\" only shows\n> the object names of trees and blobs, not the payload in these\n> objects, and this is very much on purpuse. It allows the downstream\n> process that reads its output from the pipe to easily parse the\n> output and choose to do whatever it wants to do using them.\n> \n> I wonder if we should show the blob object names that store the\n> notes payload if we were given --notes option in a format that is\n> easy for readers to mechanically parse its output.\n\nSo leaving aside the --graph issues, we would need to decide what to\nshow in the non-graph case. And I think your suggestion is good; there\nis no real need to dereference the blob (if you want that, you can turn\non the pretty-printer).\n\nI'm just not sure what the output should be. I guess:\n\n \n\nis probably the most sensible (it's sort of like --parents). And that\nsolves the --graph issue, too, since it continues to take only a single\nline.\n\n-Peff\n\n[1] Looking at the code, I do think somebody wanted \"rev-list --graph\"\nto work, and it is not an accident. But I think they did not do a very\nthorough job, as things like \"git rev-list --objects --graph\" produce\nnonsensical output.\n"},{"id":"195301","messageId":"7vhat4wv6h.fsf@alter.siamese.dyndns.org","threadId":"30052","inReplyTo":"20120718072104.GB12942@sigill.intra.peff.net","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-18T22:39:34Z","receivedAt":"2012-07-18T22:39:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So leaving aside the --graph issues, we would need to decide what to\n> show in the non-graph case. And I think your suggestion is good; there\n> is no real need to dereference the blob (if you want that, you can turn\n> on the pretty-printer).\n>\n> I'm just not sure what the output should be. I guess:\n>\n> \n>\n> is probably the most sensible (it's sort of like --parents). And that\n> solves the --graph issue, too, since it continues to take only a single\n> line.\n\nSurely. \"rev-list --parents --notes\" would still be usable that\nway, as a reader that requests such a combination is prepared to\ntell commits (i.e. parents) and blobs (i.e. notes) apart.\n"},{"id":"195326","messageId":"20120719113535.GA29774@sigill.intra.peff.net","threadId":"30052","inReplyTo":"7vhat4wv6h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-19T11:35:35Z","receivedAt":"2012-07-19T11:35:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2012 at 03:39:34PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So leaving aside the --graph issues, we would need to decide what to\n> > show in the non-graph case. And I think your suggestion is good; there\n> > is no real need to dereference the blob (if you want that, you can turn\n> > on the pretty-printer).\n> >\n> > I'm just not sure what the output should be. I guess:\n> >\n> > \n> >\n> > is probably the most sensible (it's sort of like --parents). And that\n> > solves the --graph issue, too, since it continues to take only a single\n> > line.\n> \n> Surely. \"rev-list --parents --notes\" would still be usable that\n> way, as a reader that requests such a combination is prepared to\n> tell commits (i.e. parents) and blobs (i.e. notes) apart.\n\nI don't think we forbid non-blob values in notes trees, so technically\nthere could be some ambiguity. I doubt it is a big problem in practice\n(especially since I haven't even heard of a good use case yet for \"git\nrev-list --notes\", let alone \"git rev-list --notes --parents\"). But now\nwould be the time to avoid a crappy format that we will be stuck with\nlater.\n\nUnlike elements of the commit object itself, like --parents or\n--timestamp, notes do not really gain any efficiency by being printed as\npart of the traversal. So modulo the cost of piping the list of commits,\nit would not really be any more efficient than \"git rev-list | git notes\nlist --stdin\" (except that the latter does not take a --stdin argument,\nbut could easily do so). And the latter is way more flexible.\n\nSo for plumbing, I think this is the wrong direction, anyway. The real\nvalue of this patch is that the pretty-printed code path would work more\nlike git-log (especially the \"%N\" format, which lets callers make their\nown micro-format for specifying all the bits they are interested in).\n\nMaybe the best thing is to simply disallow --notes when not using a\npretty-printed format.\n\n-Peff\n"},{"id":"195336","messageId":"7vr4s7vfal.fsf@alter.siamese.dyndns.org","threadId":"30052","inReplyTo":"20120719113535.GA29774@sigill.intra.peff.net","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-19T17:20:18Z","receivedAt":"2012-07-19T17:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Unlike elements of the commit object itself, like --parents or\n> --timestamp, notes do not really gain any efficiency by being printed as\n> part of the traversal. So modulo the cost of piping the list of commits,\n> it would not really be any more efficient than \"git rev-list | git notes\n> list --stdin\" (except that the latter does not take a --stdin argument,\n> but could easily do so). And the latter is way more flexible.\n\nYeah, I prefer that (not that I think we need either badly).\n\n> So for plumbing, I think this is the wrong direction, anyway. The real\n> value of this patch is that the pretty-printed code path would work more\n> like git-log (especially the \"%N\" format, which lets callers make their\n> own micro-format for specifying all the bits they are interested in).\n\nYeah, but at that point the obvious question becomes \"why you aren't\nusing 'git log' in the first place\".\n\n> Maybe the best thing is to simply disallow --notes when not using a\n> pretty-printed format.\n\nYeah, or simply ignore it.\n"},{"id":"195340","messageId":"20120719172550.GA12883@sigill.intra.peff.net","threadId":"30052","inReplyTo":"7vr4s7vfal.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix notes handling in rev-list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-19T17:25:50Z","receivedAt":"2012-07-19T17:25:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2012 at 10:20:18AM -0700, Junio C Hamano wrote:\n\n> > So for plumbing, I think this is the wrong direction, anyway. The real\n> > value of this patch is that the pretty-printed code path would work more\n> > like git-log (especially the \"%N\" format, which lets callers make their\n> > own micro-format for specifying all the bits they are interested in).\n> \n> Yeah, but at that point the obvious question becomes \"why you aren't\n> using 'git log' in the first place\".\n\nI dunno. I guess there are other plumbing-like behaviors of rev-list\nthat you would want, but the only ones I can think of are diff options,\nwhich rev-list does not handle at all.\n\n> > Maybe the best thing is to simply disallow --notes when not using a\n> > pretty-printed format.\n> \n> Yeah, or simply ignore it.\n\nI'd rather generate an error to make it more obvious what is happening\n(and it is not that we are somehow failing to find any notes). And it\nmight help prevent the later question of: why does \"git rev-list\n--oneline --notes\" show notes, but \"git rev-list --notes\" silently\nignores it? \n\n-Peff\n"}]}