{"thread":{"id":"40158","subject":"[PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes","startedAt":"2015-08-22T15:14:39Z","lastAt":"2015-08-24T14:48:34Z","messageCount":7,"participants":["Johannes Schindelin","Johan Herland","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"268511","messageId":"0LZlZ2-1Z1Zyn1mzk-00lZ3Z@mail.gmx.com","threadId":"40158","inReplyTo":null,"subject":"[PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-22T15:14:39Z","receivedAt":"2015-08-22T15:14:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The `format_display_notes()` function clearly assumes that the data\nstructure holding the notes has been initialized already, i.e. that the\n`display_notes_trees` variable is no longer `NULL`.\n\nHowever, when there are no notes whatsoever, this variable is still\n`NULL`, even after initialization.\n\nSo let's be graceful and just return if that data structure is `NULL`.\n\nReported in https://github.com/msysgit/git/issues/363.\n\nSigned-off-by: Johannes Schindelin \n---\n notes.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/notes.c b/notes.c\nindex df08209..24a335a 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1266,7 +1266,10 @@ void format_display_notes(const unsigned char *object_sha1,\n \t\t\t struct strbuf *sb, const char *output_encoding, int raw)\n {\n \tint i;\n-\tassert(display_notes_trees);\n+\n+\tif (!display_notes_trees)\n+\t\treturn;\n+\n \tfor (i = 0; display_notes_trees[i]; i++)\n \t\tformat_note(display_notes_trees[i], object_sha1, sb,\n \t\t\t output_encoding, raw);\n-- \n2.3.1.windows.1.9.g8c01ab4\n"},{"id":"268513","messageId":"CALKQrgcvRvs8wLy-O2eZZg9iMG2T7coFMoFwmDu3nmAXHhdW1w@mail.gmail.com","threadId":"40158","inReplyTo":"0LZlZ2-1Z1Zyn1mzk-00lZ3Z@mail.gmx.com","subject":"Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-22T16:30:41Z","receivedAt":"2015-08-22T16:30:41Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sat, Aug 22, 2015 at 5:14 PM, Johannes Schindelin\n wrote:\n> The `format_display_notes()` function clearly assumes that the data\n> structure holding the notes has been initialized already, i.e. that the\n> `display_notes_trees` variable is no longer `NULL`.\n>\n> However, when there are no notes whatsoever, this variable is still\n> `NULL`, even after initialization.\n>\n> So let's be graceful and just return if that data structure is `NULL`.\n>\n> Reported in https://github.com/msysgit/git/issues/363.\n>\n> Signed-off-by: Johannes Schindelin \n\nAcked-by: Johan Herland \n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"268535","messageId":"20150823174309.GA24735@sigill.intra.peff.net","threadId":"40158","inReplyTo":"0LZlZ2-1Z1Zyn1mzk-00lZ3Z@mail.gmx.com","subject":"Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-23T17:43:09Z","receivedAt":"2015-08-23T17:43:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 22, 2015 at 05:14:39PM +0200, Johannes Schindelin wrote:\n\n> The `format_display_notes()` function clearly assumes that the data\n> structure holding the notes has been initialized already, i.e. that the\n> `display_notes_trees` variable is no longer `NULL`.\n> \n> However, when there are no notes whatsoever, this variable is still\n> `NULL`, even after initialization.\n> \n> So let's be graceful and just return if that data structure is `NULL`.\n\nHrm. This is supposed to be made non-NULL by calling init_display_notes.\nThe \"git-log\" code does this properly, and can show notes. The\n\"rev-list\" code does not, and hits this assert. But that also means that\nit cannot actually _show_ notes, and your patch is papering over the\nproblem.\n\nI would think we would need something more like this:\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff84a82..fc73a6f 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -279,6 +279,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tint bisect_show_vars = 0;\n \tint bisect_find_all = 0;\n \tint use_bitmap_index = 0;\n+\tstruct userformat_want w;\n \n \tgit_config(git_default_config, NULL);\n \tinit_revisions(&revs, prefix);\n@@ -349,6 +350,14 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t revs.diff)\n \t\tusage(rev_list_usage);\n \n+\tmemset(&w, 0, sizeof(w));\n+\tuserformat_find_requirements(NULL, &w);\n+\n+\tif (!revs.show_notes_given && w.notes)\n+\t\trevs.show_notes = 1;\n+\tif (revs.show_notes)\n+\t\tinit_display_notes(&revs.notes_opt);\n+\n \tsave_commit_buffer = (revs.verbose_header ||\n \t\t\t revs.grep_filter.pattern_list ||\n \t\t\t revs.grep_filter.header_list);\n\nbut it looks like that is not enough to convince the pretty-printer to\nactually show notes (I suspect we need something like the\npretty_ctx->notes_message setup that is in show_log()).\n\nI don't know how deeply anybody _cares_ about showing notes via\nrev-list. It has clearly never worked. But rather than silently\naccepting --show-notes (and not showing any notes!), perhaps we should\ntell the user that it does not work. Likewise, the \"%N\" --format\nspecifier never expands in rev-list, and should probably be removed from\nthe rev-list documentation.\n\nAlthough...\n\n> Reported in https://github.com/msysgit/git/issues/363.\n\nAfter reading your subject, I wondered why \"git rev-list --show-notes\nHEAD\" did not crash for me (whether or not I had notes). But the key\nelement from that issue is the addition of \"--grep\", which is what\ntriggers us to actually look at the notes (since rev-list otherwise does\nnot support displaying them). And that _does_ work with my patch above.\nSo perhaps that is useful, though again, it has never worked in the\npast (and with your patch, it would silently return no results, whether\nthe grep matched or not).\n\nOf course it's a terrible interface to make \"--show-notes --grep\" grep\nthe notes, but not actually _show_ them. :-/\n\n> diff --git a/notes.c b/notes.c\n> index df08209..24a335a 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -1266,7 +1266,10 @@ void format_display_notes(const unsigned char *object_sha1,\n> \t\t\t struct strbuf *sb, const char *output_encoding, int raw)\n> {\n> \tint i;\n> -\tassert(display_notes_trees);\n> +\n> +\tif (!display_notes_trees)\n> +\t\treturn;\n> +\n\nSo I'm not really in favor of this approach, but if we do go that route,\nthe comment above the declaration of format_display_notes needs to be\nupdated.\n\n-Peff\n"},{"id":"268537","messageId":"20150823175640.GB24735@sigill.intra.peff.net","threadId":"40158","inReplyTo":"20150823174309.GA24735@sigill.intra.peff.net","subject":"[PATCH] rev-list: make it obvious that we do not support notes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-23T17:56:40Z","receivedAt":"2015-08-23T17:56:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 23, 2015 at 01:43:09PM -0400, Jeff King wrote:\n\n> I don't know how deeply anybody _cares_ about showing notes via\n> rev-list. It has clearly never worked. But rather than silently\n> accepting --show-notes (and not showing any notes!), perhaps we should\n> tell the user that it does not work. Likewise, the \"%N\" --format\n> specifier never expands in rev-list, and should probably be removed from\n> the rev-list documentation.\n\nI think that would look like this. IMHO this is a strict improvement on\nthe current state. I would be happier still if somebody wanted to plumb\nthe notes options through for rev-list, but I am not personally\ninterested in spending the time on that.\n\n-- >8 --\nSubject: rev-list: make it obvious that we do not support notes\n\nThe rev-list command does not have the internal\ninfrastructure to display notes. Running:\n\n git rev-list --notes HEAD\n\nwill silently ignore the \"--notes\" option. Running:\n\n git rev-list --notes --grep=. HEAD\n\nwill crash on an assert. Running:\n\n git rev-list --format=%N HEAD\n\nwill place a literal \"%N\" in the output (it does not even\nexpand to an empty string).\n\nLet's have rev-list tell the user that it cannot fill the\nuser's request, rather than silently producing wrong data.\nLikewise, let's remove mention of the notes options from the\nrev-list documentation.\n\nSigned-off-by: Jeff King \n---\n Documentation/pretty-formats.txt | 2 ++\n Documentation/pretty-options.txt | 2 ++\n Documentation/rev-list-options.txt | 2 ++\n builtin/rev-list.c | 3 +++\n 4 files changed, 9 insertions(+)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex dc865cb..671cebd 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -139,7 +139,9 @@ The placeholders are:\n - '%f': sanitized subject line, suitable for a filename\n - '%b': body\n - '%B': raw body (unwrapped subject and body)\n+ifndef::git-rev-list[]\n - '%N': commit notes\n+endif::git-rev-list[]\n - '%GG': raw verification message from GPG for a signed commit\n - '%G?': show \"G\" for a Good signature, \"B\" for a Bad signature, \"U\" for a good,\n untrusted signature and \"N\" for no signature\ndiff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\nindex 642af6e..8d6c5ce 100644\n--- a/Documentation/pretty-options.txt\n+++ b/Documentation/pretty-options.txt\n@@ -42,6 +42,7 @@ people using 80-column terminals.\n \tverbatim; this means that invalid sequences in the original\n \tcommit may be copied to the output.\n \n+ifndef::git-rev-list[]\n --notes[=]::\n \tShow the notes (see linkgit:git-notes[1]) that annotate the\n \tcommit, when showing the commit log message. This is the default\n@@ -73,6 +74,7 @@ being displayed. Examples: \"--notes=foo\" will show only notes from\n --[no-]standard-notes::\n \tThese options are deprecated. Use the above --notes/--no-notes\n \toptions instead.\n+endif::git-rev-list[]\n \n --show-signature::\n \tCheck the validity of a signed commit object by passing the signature\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a9b808f..f1c5220 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -58,9 +58,11 @@ endif::git-rev-list[]\n \tmore than one `--grep=`, commits whose message\n \tmatches any of the given patterns are chosen (but see\n \t`--all-match`).\n+ifndef::git-rev-list[]\n +\n When `--show-notes` is in effect, the message from the notes is\n matched as if it were part of the log message.\n+endif::git-rev-list[]\n \n --all-match::\n \tLimit the commits output to ones that match all given `--grep`,\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex c0b4b53..d80d1ed 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -350,6 +350,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t revs.diff)\n \t\tusage(rev_list_usage);\n \n+\tif (revs.show_notes)\n+\t\tdie(_(\"rev-list does not support display of notes\"));\n+\n \tsave_commit_buffer = (revs.verbose_header ||\n \t\t\t revs.grep_filter.pattern_list ||\n \t\t\t revs.grep_filter.header_list);\n-- \n2.5.0.685.g0ca4974\n"},{"id":"268563","messageId":"98f2f674e384ac3af38e305165135a60@www.dscho.org","threadId":"40158","inReplyTo":"20150823174309.GA24735@sigill.intra.peff.net","subject":"Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-24T10:23:52Z","receivedAt":"2015-08-24T10:23:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-08-23 19:43, Jeff King wrote:\n> On Sat, Aug 22, 2015 at 05:14:39PM +0200, Johannes Schindelin wrote:\n> \n>> The `format_display_notes()` function clearly assumes that the data\n>> structure holding the notes has been initialized already, i.e. that the\n>> `display_notes_trees` variable is no longer `NULL`.\n>>\n>> However, when there are no notes whatsoever, this variable is still\n>> `NULL`, even after initialization.\n>>\n>> So let's be graceful and just return if that data structure is `NULL`.\n> \n> Hrm. This is supposed to be made non-NULL by calling init_display_notes.\n> The \"git-log\" code does this properly, and can show notes. The\n> \"rev-list\" code does not, and hits this assert. But that also means that\n> it cannot actually _show_ notes, and your patch is papering over the\n> problem.\n\nGood point...\n\n> it looks like [patch] is not enough to convince the pretty-printer to\n> actually show notes (I suspect we need something like the\n> pretty_ctx->notes_message setup that is in show_log()).\n> \n> I don't know how deeply anybody _cares_ about showing notes via\n> rev-list. It has clearly never worked. But rather than silently\n> accepting --show-notes (and not showing any notes!), perhaps we should\n> tell the user that it does not work. Likewise, the \"%N\" --format\n> specifier never expands in rev-list, and should probably be removed from\n> the rev-list documentation.\n\nHmpf. I really did not want to uncover yet another rabbit hole...\n\n>> Reported in https://github.com/msysgit/git/issues/363.\n> \n> After reading your subject, I wondered why \"git rev-list --show-notes\n> HEAD\" did not crash for me (whether or not I had notes). But the key\n> element from that issue is the addition of \"--grep\", which is what\n> triggers us to actually look at the notes (since rev-list otherwise does\n> not support displaying them). And that _does_ work with my patch above.\n> So perhaps that is useful, though again, it has never worked in the\n> past (and with your patch, it would silently return no results, whether\n> the grep matched or not).\n> \n> Of course it's a terrible interface to make \"--show-notes --grep\" grep\n> the notes, but not actually _show_ them. :-/\n\nIt is.\n\n> So I'm not really in favor of this approach, but if we do go that route,\n> the comment above the declaration of format_display_notes needs to be\n> updated.\n\nYou're right. I think the best approach for now is to error out when `--show-notes` is passed to rev-list. Do you agree?\n\nCiao,\nDscho\n"},{"id":"268568","messageId":"20150824144328.GA2151@sigill.intra.peff.net","threadId":"40158","inReplyTo":"98f2f674e384ac3af38e305165135a60@www.dscho.org","subject":"Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-24T14:43:29Z","receivedAt":"2015-08-24T14:43:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2015 at 12:23:52PM +0200, Johannes Schindelin wrote:\n\n> You're right. I think the best approach for now is to error out when\n> `--show-notes` is passed to rev-list. Do you agree?\n\nYes (I imagine you didn't yet read my follow-up patch when you wrote\nthis). :)\n\n-Peff\n"},{"id":"268569","messageId":"4855e31e35814e066cee2cd7fe7e82be@www.dscho.org","threadId":"40158","inReplyTo":"20150824144328.GA2151@sigill.intra.peff.net","subject":"Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-24T14:48:34Z","receivedAt":"2015-08-24T14:48:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-08-24 16:43, Jeff King wrote:\n> On Mon, Aug 24, 2015 at 12:23:52PM +0200, Johannes Schindelin wrote:\n> \n>> You're right. I think the best approach for now is to error out when\n>> `--show-notes` is passed to rev-list. Do you agree?\n> \n> Yes (I imagine you didn't yet read my follow-up patch when you wrote\n> this). :)\n\nYep, and I forgot to reply when I saw it. Thanks for continuing on that front!\n\nCiao,\nDscho\n"}]}