{"thread":{"id":"53239","subject":"[PATCH 0/2] range-diff: fix a crash in parsing git-log output","startedAt":"2020-04-15T14:28:57Z","lastAt":"2020-04-16T01:07:44Z","messageCount":15,"participants":["Vasil Dimov via GitGitGadget","Junio C Hamano","Taylor Blau","Vasil Dimov","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"395494","messageId":"pull.760.git.git.1586960921.gitgitgadget@gmail.com","threadId":"53239","inReplyTo":null,"subject":"[PATCH 0/2] range-diff: fix a crash in parsing git-log output","fromName":"Vasil Dimov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-15T14:28:39Z","receivedAt":"2020-04-15T14:28:57Z","isPatch":true,"sender":{"key":"vd@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/266751?v=4"},"body":" * override a possibly user-customized format.pretty that would render git\n log output unparsable by git range-diff\n \n \n * don't use negative string precision, e.g. \"%.*s\", -5, \"foo\"\n\nVasil Dimov (2):\n range-diff: fix a crash in parsing git-log output\n range-diff: avoid negative string precision\n\n range-diff.c | 18 +++++++++++++++++-\n t/t3206-range-diff.sh | 10 ++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\n\nbase-commit: de49261b050d9cd8ec73842356077bc5b606640f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-760%2Fvasild%2Frange-diff-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-760/vasild/range-diff-v1\nPull-Request: https://github.com/git/git/pull/760\n-- \ngitgitgadget\n"},{"id":"395495","messageId":"b3384880c7201d65adb7341ce23386d578e69193.1586960921.git.gitgitgadget@gmail.com","threadId":"53239","inReplyTo":"pull.760.git.git.1586960921.gitgitgadget@gmail.com","subject":"[PATCH 2/2] range-diff: avoid negative string precision","fromName":"Vasil Dimov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-15T14:28:41Z","receivedAt":"2020-04-15T14:28:59Z","isPatch":true,"sender":{"key":"vd@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/266751?v=4"},"body":"From: Vasil Dimov \n\nIf the supplied integer for \"precisoin\" is negative in\n`\"%.*s\", len, line` then it is ignored. So the current code is\nequivalent to just `\"%s\", line` because it is executed only if\n`len` is negative.\n\nFix this by saving the value of `len` before overwriting it with the\nreturn value of `parse_git_diff_header()`.\n\nSigned-off-by: Vasil Dimov \n---\n range-diff.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 5cc920be391..40af0862818 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -123,16 +123,19 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\tstruct patch patch = { 0 };\n \t\t\tstruct strbuf root = STRBUF_INIT;\n \t\t\tint linenr = 0;\n+\t\t\tint orig_len;\n \n \t\t\tin_header = 0;\n \t\t\tstrbuf_addch(&buf, '\\n');\n \t\t\tif (!util->diff_offset)\n \t\t\t\tutil->diff_offset = buf.len;\n \t\t\tline[len - 1] = '\\n';\n+\t\t\torig_len = len;\n \t\t\tlen = parse_git_diff_header(&root, &linenr, 0, line,\n \t\t\t\t\t\t len, size, &patch);\n \t\t\tif (len < 0)\n-\t\t\t\tdie(_(\"could not parse git header '%.*s'\"), (int)len, line);\n+\t\t\t\tdie(_(\"could not parse git header '%.*s'\"),\n+\t\t\t\t orig_len, line);\n \t\t\tstrbuf_addstr(&buf, \" ## \");\n \t\t\tif (patch.is_new > 0)\n \t\t\t\tstrbuf_addf(&buf, \"%s (new)\", patch.new_name);\n-- \ngitgitgadget\n"},{"id":"395496","messageId":"2375e34100e571f9c3ce658d28aba6648fba18a6.1586960921.git.gitgitgadget@gmail.com","threadId":"53239","inReplyTo":"pull.760.git.git.1586960921.gitgitgadget@gmail.com","subject":"[PATCH 1/2] range-diff: fix a crash in parsing git-log output","fromName":"Vasil Dimov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-15T14:28:40Z","receivedAt":"2020-04-15T14:29:06Z","isPatch":true,"sender":{"key":"vd@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/266751?v=4"},"body":"From: Vasil Dimov \n\n`git range-diff` calls `git log` internally and tries to parse its\noutput. But `git log` output can be customized by the user in their\ngit config and for certain configurations either an error will be\nreturned by `git range-diff` or it will crash.\n\nTo fix this explicitly set the output format of the internally\nexecuted `git log` with `--pretty=medium`. Because that cancels\n`--notes`, add explicitly `--notes` at the end.\n\nAlso, make sure we never crash in the same way - trying to dereference\n`util` which was never created and has remained NULL. It would happen\nif the first line of `git log` output does not begin with 'commit '.\n\nAlternative considered but discarded - somehow disable all git configs\nand behave as if no config is present in the internally executed\n`git log`, but that does not seem to be possible. GIT_CONFIG_NOSYSTEM\nis the closest to it, but even with that we would still read\n`.git/config`.\n\nSigned-off-by: Vasil Dimov \n---\n range-diff.c | 13 +++++++++++++\n t/t3206-range-diff.sh | 10 ++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex f745567cf67..5cc920be391 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -63,6 +63,8 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\t\"--output-indicator-old=<\",\n \t\t\t\"--output-indicator-context=#\",\n \t\t\t\"--no-abbrev-commit\",\n+\t\t\t\"--pretty=medium\",\n+\t\t\t\"--notes\",\n \t\t\tNULL);\n \tif (other_arg)\n \t\targv_array_pushv(&cp.args, other_arg->argv);\n@@ -106,6 +108,17 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (!util) {\n+\t\t\terror(_(\"could not parse first line of `log` output: \"\n+\t\t\t\t\"did not start with 'commit ': '%s'\"),\n+\t\t\t line);\n+\t\t\tstring_list_clear(list, 1);\n+\t\t\tstrbuf_release(&buf);\n+\t\t\tstrbuf_release(&contents);\n+\t\t\tfinish_command(&cp);\n+\t\t\treturn -1;\n+\t\t}\n+\n \t\tif (starts_with(line, \"diff --git\")) {\n \t\t\tstruct patch patch = { 0 };\n \t\t\tstruct strbuf root = STRBUF_INIT;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex bd808f87ed5..e024cff65cb 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -513,6 +513,16 @@ test_expect_success 'range-diff overrides diff.noprefix internally' '\n \tgit -c diff.noprefix=true range-diff HEAD^...\n '\n \n+test_expect_success 'basic with modified format.pretty with suffix' '\n+\tgit -c format.pretty=\"format:commit %H%d%n\" range-diff \\\n+\t\tmaster..topic master..unmodified\n+'\n+\n+test_expect_success 'basic with modified format.pretty without \"commit \"' '\n+\tgit -c format.pretty=\"format:%H%n\" range-diff \\\n+\t\tmaster..topic master..unmodified\n+'\n+\n test_expect_success 'range-diff compares notes by default' '\n \tgit notes add -m \"topic note\" topic &&\n \tgit notes add -m \"unmodified note\" unmodified &&\n-- \ngitgitgadget\n\n"},{"id":"395503","messageId":"xmqqsgh47okk.fsf@gitster.c.googlers.com","threadId":"53239","inReplyTo":"2375e34100e571f9c3ce658d28aba6648fba18a6.1586960921.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] range-diff: fix a crash in parsing git-log output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-15T15:31:39Z","receivedAt":"2020-04-15T15:31:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Vasil Dimov via GitGitGadget\" writes:\n\n> From: Vasil Dimov \n>\n> `git range-diff` calls `git log` internally and tries to parse its\n> output. But `git log` output can be customized by the user in their\n> git config and for certain configurations either an error will be\n> returned by `git range-diff` or it will crash.\n>\n> To fix this explicitly set the output format of the internally\n> executed `git log` with `--pretty=medium`. Because that cancels\n> `--notes`, add explicitly `--notes` at the end.\n\nGood finding. \n\nShouldn't we also disable customizations that come from the\nconfiguration variables like diff.external, diff..command?\n\nThanks.\n\n"},{"id":"395514","messageId":"20200415161324.GC22823@syl.local","threadId":"53239","inReplyTo":"2375e34100e571f9c3ce658d28aba6648fba18a6.1586960921.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] range-diff: fix a crash in parsing git-log output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-15T16:13:24Z","receivedAt":"2020-04-15T16:13:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Vasil,\n\nNice find. I have a question below:\n\nOn Wed, Apr 15, 2020 at 02:28:40PM +0000, Vasil Dimov via GitGitGadget wrote:\n> From: Vasil Dimov \n>\n> `git range-diff` calls `git log` internally and tries to parse its\n> output. But `git log` output can be customized by the user in their\n> git config and for certain configurations either an error will be\n> returned by `git range-diff` or it will crash.\n>\n> To fix this explicitly set the output format of the internally\n> executed `git log` with `--pretty=medium`. Because that cancels\n> `--notes`, add explicitly `--notes` at the end.\n>\n> Also, make sure we never crash in the same way - trying to dereference\n> `util` which was never created and has remained NULL. It would happen\n> if the first line of `git log` output does not begin with 'commit '.\n>\n> Alternative considered but discarded - somehow disable all git configs\n> and behave as if no config is present in the internally executed\n> `git log`, but that does not seem to be possible. GIT_CONFIG_NOSYSTEM\n> is the closest to it, but even with that we would still read\n> `.git/config`.\n\nI don't know of a great way to do this off-hand. Perhaps an internal\n`--for-range-diff` option that ignores options that are incompatible\nwith range-diff's own parsing seems like an OK path forward to me.\n\nHere, internal means that it's not part of the manual page. We can pass\n'PARSE_OPT_NONEG' to make sure that a caller can't later overwrite it by\npassing '--no-for-range-diff'.\n\n> Signed-off-by: Vasil Dimov \n> ---\n> range-diff.c | 13 +++++++++++++\n> t/t3206-range-diff.sh | 10 ++++++++++\n> 2 files changed, 23 insertions(+)\n>\n> diff --git a/range-diff.c b/range-diff.c\n> index f745567cf67..5cc920be391 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -63,6 +63,8 @@ static int read_patches(const char *range, struct string_list *list,\n> \t\t\t\"--output-indicator-old=<\",\n> \t\t\t\"--output-indicator-context=#\",\n> \t\t\t\"--no-abbrev-commit\",\n> +\t\t\t\"--pretty=medium\",\n> +\t\t\t\"--notes\",\n> \t\t\tNULL);\n> \tif (other_arg)\n> \t\targv_array_pushv(&cp.args, other_arg->argv);\n> @@ -106,6 +108,17 @@ static int read_patches(const char *range, struct string_list *list,\n> \t\t\tcontinue;\n> \t\t}\n>\n> +\t\tif (!util) {\n> +\t\t\terror(_(\"could not parse first line of `log` output: \"\n> +\t\t\t\t\"did not start with 'commit ': '%s'\"),\n> +\t\t\t line);\n> +\t\t\tstring_list_clear(list, 1);\n> +\t\t\tstrbuf_release(&buf);\n> +\t\t\tstrbuf_release(&contents);\n> +\t\t\tfinish_command(&cp);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> \t\tif (starts_with(line, \"diff --git\")) {\n> \t\t\tstruct patch patch = { 0 };\n> \t\t\tstruct strbuf root = STRBUF_INIT;\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index bd808f87ed5..e024cff65cb 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -513,6 +513,16 @@ test_expect_success 'range-diff overrides diff.noprefix internally' '\n> \tgit -c diff.noprefix=true range-diff HEAD^...\n> '\n>\n> +test_expect_success 'basic with modified format.pretty with suffix' '\n> +\tgit -c format.pretty=\"format:commit %H%d%n\" range-diff \\\n> +\t\tmaster..topic master..unmodified\n> +'\n> +\n> +test_expect_success 'basic with modified format.pretty without \"commit \"' '\n> +\tgit -c format.pretty=\"format:%H%n\" range-diff \\\n> +\t\tmaster..topic master..unmodified\n> +'\n> +\n> test_expect_success 'range-diff compares notes by default' '\n> \tgit notes add -m \"topic note\" topic &&\n> \tgit notes add -m \"unmodified note\" unmodified &&\n> --\n> gitgitgadget\n\nOtherwise what you have here does look good to me, too. I'm just not\nsure that there aren't other ways that a caller could circumvent placing\n'--notes --pretty=medium' at the end, anyway.\n\nThanks,\nTaylor\n"},{"id":"395515","messageId":"20200415161647.GA39892@smle","threadId":"53239","inReplyTo":"xmqqsgh47okk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] range-diff: fix a crash in parsing git-log output","fromName":"Vasil Dimov","fromEmail":"vd@freebsd.org","sentAt":"2020-04-15T16:16:47Z","receivedAt":"2020-04-15T16:17:21Z","isPatch":true,"sender":{"key":"vd@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/266751?v=4"},"body":"On Wed, Apr 15, 2020 at 08:31:39 -0700, Junio C Hamano wrote:\n> \"Vasil Dimov via GitGitGadget\" writes:\n>\n> > From: Vasil Dimov \n> >\n> > `git range-diff` calls `git log` internally and tries to parse its\n> > output. But `git log` output can be customized by the user in their\n> > git config and for certain configurations either an error will be\n> > returned by `git range-diff` or it will crash.\n> >\n> > To fix this explicitly set the output format of the internally\n> > executed `git log` with `--pretty=medium`. Because that cancels\n> > `--notes`, add explicitly `--notes` at the end.\n>\n> Good finding.\n>\n> Shouldn't we also disable customizations that come from the\n> configuration variables like diff.external, diff..command?\n\nHmm, I am not sure. I just read the doc about diff..command. Is\nit possible that the user has set up some custom diff tools to compare\ne.g. .jpg files by only comparing their EXIF data instead of the entire\n(binary) files?\n\nSurely if the customizations wrt diff.external and\ndiff..command produce result that is unparsable by\ngit range-diff, then they are not good. But maybe the opposite is the\ncase?\n\nI don't feel confident enough to judge.\n\n-- \nVasil Dimov\ngro.DSBeerF@dv\n%\nSometimes I really think people ought to have to pass a proper exam\nbefore they're allowed to be parents. Not just the practical, I mean.\n -- (Terry Pratchett, Thief of Time)\n"},{"id":"395516","messageId":"20200415162035.GD22823@syl.local","threadId":"53239","inReplyTo":"b3384880c7201d65adb7341ce23386d578e69193.1586960921.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] range-diff: avoid negative string precision","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-15T16:20:35Z","receivedAt":"2020-04-15T16:21:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 15, 2020 at 02:28:41PM +0000, Vasil Dimov via GitGitGadget wrote:\n> From: Vasil Dimov \n>\n> If the supplied integer for \"precisoin\" is negative in\n\ns/precisoin/precision\n\n> `\"%.*s\", len, line` then it is ignored. So the current code is\n> equivalent to just `\"%s\", line` because it is executed only if\n> `len` is negative.\n>\n> Fix this by saving the value of `len` before overwriting it with the\n> return value of `parse_git_diff_header()`.\n>\n> Signed-off-by: Vasil Dimov \n> ---\n> range-diff.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/range-diff.c b/range-diff.c\n> index 5cc920be391..40af0862818 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -123,16 +123,19 @@ static int read_patches(const char *range, struct string_list *list,\n> \t\t\tstruct patch patch = { 0 };\n> \t\t\tstruct strbuf root = STRBUF_INIT;\n> \t\t\tint linenr = 0;\n> +\t\t\tint orig_len;\n\nAny reason to not assign this to 'len' up here?\n>\n> \t\t\tin_header = 0;\n> \t\t\tstrbuf_addch(&buf, '\\n');\n> \t\t\tif (!util->diff_offset)\n> \t\t\t\tutil->diff_offset = buf.len;\n> \t\t\tline[len - 1] = '\\n';\n> +\t\t\torig_len = len;\n> \t\t\tlen = parse_git_diff_header(&root, &linenr, 0, line,\n> \t\t\t\t\t\t len, size, &patch);\n\nOK, so we cut up the line by placing a NL at len, and then feed it to\n'parse_git_diff_header' which will tell us the length of the thing that\nit parsed, or give a negative value if it couldn't parse...\n\n> \t\t\tif (len < 0)\n> -\t\t\t\tdie(_(\"could not parse git header '%.*s'\"), (int)len, line);\n> +\t\t\t\tdie(_(\"could not parse git header '%.*s'\"),\n> +\t\t\t\t orig_len, line);\n\n...and then you restore the original length and print it out here. It\nseems like this error is now misleading though, because the line is\nalready modified at the point that the newline was inserted.\n\n> \t\t\tstrbuf_addstr(&buf, \" ## \");\n> \t\t\tif (patch.is_new > 0)\n> \t\t\t\tstrbuf_addf(&buf, \"%s (new)\", patch.new_name);\n> --\n> gitgitgadget\n\nThanks,\nTaylor\n"},{"id":"395517","messageId":"20200415162326.GG2464307@coredump.intra.peff.net","threadId":"53239","inReplyTo":"xmqqsgh47okk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] range-diff: fix a crash in parsing git-log output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-15T16:23:26Z","receivedAt":"2020-04-15T16:23:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 15, 2020 at 08:31:39AM -0700, Junio C Hamano wrote:\n\n> \"Vasil Dimov via GitGitGadget\" writes:\n> \n> > From: Vasil Dimov \n> >\n> > `git range-diff` calls `git log` internally and tries to parse its\n> > output. But `git log` output can be customized by the user in their\n> > git config and for certain configurations either an error will be\n> > returned by `git range-diff` or it will crash.\n> >\n> > To fix this explicitly set the output format of the internally\n> > executed `git log` with `--pretty=medium`. Because that cancels\n> > `--notes`, add explicitly `--notes` at the end.\n> \n> Good finding. \n> \n> Shouldn't we also disable customizations that come from the\n> configuration variables like diff.external, diff..command?\n\nIf range-diff were a script, I would say it should be using the\n\"rev-list | diff-tree --stdin\" plumbing under the hood, rather than\n\"log\".\n\nThe read_patches() function does let callers pass options to git-log,\nbut I don't _think_ this is exposed to the user. We only allow a few\n--notes options to be passed, and we should be able to apply those to\ndiff-tree. So converting it to use plumbing might be an option.\n\nThough I think there is another bug:\n\n $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes\n commit 8f3d9f354286745c751374f5f1fcafee6b3f3136\n git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.\n Aborted\n\nAnother option would be for range-diff to directly call the\nrevision-traversal plumbing itself. There may be complications there,\nthough (or else it would have been done from the outset).\n\nWe should fix that assertion regardless, though.\n\n-Peff\n"},{"id":"395529","messageId":"20200415201947.GA47498@smle","threadId":"53239","inReplyTo":"20200415162035.GD22823@syl.local","subject":"Re: [PATCH 2/2] range-diff: avoid negative string precision","fromName":"Vasil Dimov","fromEmail":"vd@freebsd.org","sentAt":"2020-04-15T20:19:47Z","receivedAt":"2020-04-15T20:19:57Z","isPatch":true,"sender":{"key":"vd@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/266751?v=4"},"body":"On Wed, Apr 15, 2020 at 10:20:35 -0600, Taylor Blau wrote:\n> On Wed, Apr 15, 2020 at 02:28:41PM +0000, Vasil Dimov via GitGitGadget wrote:\n> > From: Vasil Dimov \n> >\n> > If the supplied integer for \"precisoin\" is negative in\n> \n> s/precisoin/precision\n\nFixed in v2.\n\n> > `\"%.*s\", len, line` then it is ignored. So the current code is\n> > equivalent to just `\"%s\", line` because it is executed only if\n> > `len` is negative.\n> >\n> > Fix this by saving the value of `len` before overwriting it with the\n> > return value of `parse_git_diff_header()`.\n> >\n> > Signed-off-by: Vasil Dimov \n> > ---\n> > range-diff.c | 5 ++++-\n> > 1 file changed, 4 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/range-diff.c b/range-diff.c\n> > index 5cc920be391..40af0862818 100644\n> > --- a/range-diff.c\n> > +++ b/range-diff.c\n> > @@ -123,16 +123,19 @@ static int read_patches(const char *range, struct string_list *list,\n> > \t\t\tstruct patch patch = { 0 };\n> > \t\t\tstruct strbuf root = STRBUF_INIT;\n> > \t\t\tint linenr = 0;\n> > +\t\t\tint orig_len;\n> \n> Any reason to not assign this to 'len' up here?\n\nI believe that assigning it just before len is changed and that grouping\nall usage of the new variable close to each other makes the code more\nreadable. Ideally it would also be defined below.\n\n> > \t\t\tin_header = 0;\n> > \t\t\tstrbuf_addch(&buf, '\\n');\n> > \t\t\tif (!util->diff_offset)\n> > \t\t\t\tutil->diff_offset = buf.len;\n> > \t\t\tline[len - 1] = '\\n';\n> > +\t\t\torig_len = len;\n> > \t\t\tlen = parse_git_diff_header(&root, &linenr, 0, line,\n> > \t\t\t\t\t\t len, size, &patch);\n> \n> OK, so we cut up the line by placing a NL at len, and then feed it to\n> 'parse_git_diff_header' which will tell us the length of the thing that\n> it parsed, or give a negative value if it couldn't parse...\n> \n> > \t\t\tif (len < 0)\n> > -\t\t\t\tdie(_(\"could not parse git header '%.*s'\"), (int)len, line);\n> > +\t\t\t\tdie(_(\"could not parse git header '%.*s'\"),\n> > +\t\t\t\t orig_len, line);\n> \n> ...and then you restore the original length and print it out here. It\n> seems like this error is now misleading though, because the line is\n> already modified at the point that the newline was inserted.\n[...]\n\nIt was '\\0' before we overwrote it with '\\n':\n\n 89 len = find_end_of_line(line, size);\n 90 line[len - 1] = '\\0';\n\n`line` points to a buffer of the entire output of `git log`, with many\nnewlines in it. In the beginning of the loop we overwrite the first new\nline char in the buffer with '\\0', then we restore it to '\\n' and\neventually advance the pointer to the start of the next line.\n\nI think that the intention of this code is to print only one line (the\ncurrent one).\n\n-- \nVasil Dimov\ngro.DSBeerF@dv\n%\nSuccess consists of going from failure to failure without loss of\nenthusiasm.\n -- Winston Churchill\n"},{"id":"395531","messageId":"2375e34100e571f9c3ce658d28aba6648fba18a6.1586982745.git.gitgitgadget@gmail.com","threadId":"53239","inReplyTo":"pull.760.v2.git.git.1586982745.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] range-diff: fix a crash in parsing git-log output","fromName":"Vasil Dimov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-15T20:32:24Z","receivedAt":"2020-04-15T20:32:46Z","isPatch":true,"sender":{"key":"vd@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/266751?v=4"},"body":"From: Vasil Dimov \n\n`git range-diff` calls `git log` internally and tries to parse its\noutput. But `git log` output can be customized by the user in their\ngit config and for certain configurations either an error will be\nreturned by `git range-diff` or it will crash.\n\nTo fix this explicitly set the output format of the internally\nexecuted `git log` with `--pretty=medium`. Because that cancels\n`--notes`, add explicitly `--notes` at the end.\n\nAlso, make sure we never crash in the same way - trying to dereference\n`util` which was never created and has remained NULL. It would happen\nif the first line of `git log` output does not begin with 'commit '.\n\nAlternative considered but discarded - somehow disable all git configs\nand behave as if no config is present in the internally executed\n`git log`, but that does not seem to be possible. GIT_CONFIG_NOSYSTEM\nis the closest to it, but even with that we would still read\n`.git/config`.\n\nSigned-off-by: Vasil Dimov \n---\n range-diff.c | 13 +++++++++++++\n t/t3206-range-diff.sh | 10 ++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex f745567cf67..5cc920be391 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -63,6 +63,8 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\t\"--output-indicator-old=<\",\n \t\t\t\"--output-indicator-context=#\",\n \t\t\t\"--no-abbrev-commit\",\n+\t\t\t\"--pretty=medium\",\n+\t\t\t\"--notes\",\n \t\t\tNULL);\n \tif (other_arg)\n \t\targv_array_pushv(&cp.args, other_arg->argv);\n@@ -106,6 +108,17 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (!util) {\n+\t\t\terror(_(\"could not parse first line of `log` output: \"\n+\t\t\t\t\"did not start with 'commit ': '%s'\"),\n+\t\t\t line);\n+\t\t\tstring_list_clear(list, 1);\n+\t\t\tstrbuf_release(&buf);\n+\t\t\tstrbuf_release(&contents);\n+\t\t\tfinish_command(&cp);\n+\t\t\treturn -1;\n+\t\t}\n+\n \t\tif (starts_with(line, \"diff --git\")) {\n \t\t\tstruct patch patch = { 0 };\n \t\t\tstruct strbuf root = STRBUF_INIT;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex bd808f87ed5..e024cff65cb 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -513,6 +513,16 @@ test_expect_success 'range-diff overrides diff.noprefix internally' '\n \tgit -c diff.noprefix=true range-diff HEAD^...\n '\n \n+test_expect_success 'basic with modified format.pretty with suffix' '\n+\tgit -c format.pretty=\"format:commit %H%d%n\" range-diff \\\n+\t\tmaster..topic master..unmodified\n+'\n+\n+test_expect_success 'basic with modified format.pretty without \"commit \"' '\n+\tgit -c format.pretty=\"format:%H%n\" range-diff \\\n+\t\tmaster..topic master..unmodified\n+'\n+\n test_expect_success 'range-diff compares notes by default' '\n \tgit notes add -m \"topic note\" topic &&\n \tgit notes add -m \"unmodified note\" unmodified &&\n-- \ngitgitgadget\n\n"},{"id":"395532","messageId":"pull.760.v2.git.git.1586982745.gitgitgadget@gmail.com","threadId":"53239","inReplyTo":"pull.760.git.git.1586960921.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] range-diff: fix a crash in parsing git-log output","fromName":"Vasil Dimov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-15T20:32:23Z","receivedAt":"2020-04-15T20:32:55Z","isPatch":true,"sender":{"key":"vd@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/266751?v=4"},"body":" * override a possibly user-customized format.pretty that would render git\n log output unparsable by git range-diff\n \n \n * don't use negative string precision, e.g. \"%.*s\", -5, \"foo\"\n \n \n\nChanges since v1:\n\n * Fixed a typo in the commit message (found by Taylor Blau)\n\nVasil Dimov (2):\n range-diff: fix a crash in parsing git-log output\n range-diff: avoid negative string precision\n\n range-diff.c | 18 +++++++++++++++++-\n t/t3206-range-diff.sh | 10 ++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\n\nbase-commit: de49261b050d9cd8ec73842356077bc5b606640f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-760%2Fvasild%2Frange-diff-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-760/vasild/range-diff-v2\nPull-Request: https://github.com/git/git/pull/760\n\nRange-diff vs v1:\n\n 1: 2375e34100e = 1: 2375e34100e range-diff: fix a crash in parsing git-log output\n 2: b3384880c72 ! 2: 72fddcff554 range-diff: avoid negative string precision\n @@ Metadata\n ## Commit message ##\n range-diff: avoid negative string precision\n \n - If the supplied integer for \"precisoin\" is negative in\n + If the supplied integer for \"precision\" is negative in\n `\"%.*s\", len, line` then it is ignored. So the current code is\n equivalent to just `\"%s\", line` because it is executed only if\n `len` is negative.\n\n-- \ngitgitgadget\n"},{"id":"395533","messageId":"72fddcff55425cffb598790fce3d5e7c47325234.1586982745.git.gitgitgadget@gmail.com","threadId":"53239","inReplyTo":"pull.760.v2.git.git.1586982745.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] range-diff: avoid negative string precision","fromName":"Vasil Dimov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-15T20:32:25Z","receivedAt":"2020-04-15T20:32:59Z","isPatch":true,"sender":{"key":"vd@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/266751?v=4"},"body":"From: Vasil Dimov \n\nIf the supplied integer for \"precision\" is negative in\n`\"%.*s\", len, line` then it is ignored. So the current code is\nequivalent to just `\"%s\", line` because it is executed only if\n`len` is negative.\n\nFix this by saving the value of `len` before overwriting it with the\nreturn value of `parse_git_diff_header()`.\n\nSigned-off-by: Vasil Dimov \n---\n range-diff.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 5cc920be391..40af0862818 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -123,16 +123,19 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\tstruct patch patch = { 0 };\n \t\t\tstruct strbuf root = STRBUF_INIT;\n \t\t\tint linenr = 0;\n+\t\t\tint orig_len;\n \n \t\t\tin_header = 0;\n \t\t\tstrbuf_addch(&buf, '\\n');\n \t\t\tif (!util->diff_offset)\n \t\t\t\tutil->diff_offset = buf.len;\n \t\t\tline[len - 1] = '\\n';\n+\t\t\torig_len = len;\n \t\t\tlen = parse_git_diff_header(&root, &linenr, 0, line,\n \t\t\t\t\t\t len, size, &patch);\n \t\t\tif (len < 0)\n-\t\t\t\tdie(_(\"could not parse git header '%.*s'\"), (int)len, line);\n+\t\t\t\tdie(_(\"could not parse git header '%.*s'\"),\n+\t\t\t\t orig_len, line);\n \t\t\tstrbuf_addstr(&buf, \" ## \");\n \t\t\tif (patch.is_new > 0)\n \t\t\t\tstrbuf_addf(&buf, \"%s (new)\", patch.new_name);\n-- \ngitgitgadget\n"},{"id":"395542","messageId":"20200415220242.GA45241@syl.local","threadId":"53239","inReplyTo":"20200415162326.GG2464307@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] range-diff: fix a crash in parsing git-log output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-15T22:02:42Z","receivedAt":"2020-04-15T22:02:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Wed, Apr 15, 2020 at 12:23:26PM -0400, Jeff King wrote:\n> On Wed, Apr 15, 2020 at 08:31:39AM -0700, Junio C Hamano wrote:\n>\n> > \"Vasil Dimov via GitGitGadget\" writes:\n> >\n> > > From: Vasil Dimov \n> > >\n> > > `git range-diff` calls `git log` internally and tries to parse its\n> > > output. But `git log` output can be customized by the user in their\n> > > git config and for certain configurations either an error will be\n> > > returned by `git range-diff` or it will crash.\n> > >\n> > > To fix this explicitly set the output format of the internally\n> > > executed `git log` with `--pretty=medium`. Because that cancels\n> > > `--notes`, add explicitly `--notes` at the end.\n> >\n> > Good finding.\n> >\n> > Shouldn't we also disable customizations that come from the\n> > configuration variables like diff.external, diff..command?\n>\n> If range-diff were a script, I would say it should be using the\n> \"rev-list | diff-tree --stdin\" plumbing under the hood, rather than\n> \"log\".\n>\n> The read_patches() function does let callers pass options to git-log,\n> but I don't _think_ this is exposed to the user. We only allow a few\n> --notes options to be passed, and we should be able to apply those to\n> diff-tree. So converting it to use plumbing might be an option.\n>\n> Though I think there is another bug:\n>\n> $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes\n> commit 8f3d9f354286745c751374f5f1fcafee6b3f3136\n> git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.\n> Aborted\n\nNice find. I think that I have a patch addressing this below. Please let\nme know what you think:\n\n--- >8 ---\n\nSubject: [PATCH] diff-tree.c: load notes machinery with '--notes'\n\nSince its introduction in 7249e91 (revision.c: support --notes\ncommand-line option, 2011-03-29), combining '--notes' with '--pretty'\ncauses 'git diff-tree' to fail a runtime assertion:\n\n $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes\n commit 8f3d9f354286745c751374f5f1fcafee6b3f3136\n git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.\n Aborted\n\nThis failure is due to diff-tree not calling 'load_display_notes' to\ninitialize the notes machinery.\n\nOrdinarily, this failure isn't triggered, because it requires passing\nboth '--notes' and '--pretty'. Specifically, passing '--pretty' sets\n'opt->verbose_header', causing 'show_log()' to eventually call\n'format_display_notes()', which expects a non-NULL 'display_note_trees'.\nWithout initializing the notes machinery, 'display_note_trees' remains\nNULL, and thus triggers an assertion failure. This doesn't occur without\n'--pretty' since we never call 'format_display_notes()' without it.\n\nFix this by initializing the notes machinery after parsing our options,\nand harden this behavior against regression with a test in t4013.\n\nReported-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n builtin/diff-tree.c | 2 ++\n t/t4013-diff-various.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex cb9ea79367..17c1cc8c3c 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -126,6 +126,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n\n \tprecompose_argv(argc, argv);\n \targc = setup_revisions(argc, argv, opt, &s_r_opt);\n+\tif (opt->show_notes)\n+\t\tload_display_notes(&opt->notes_opt);\n\n \twhile (--argc > 0) {\n \t\tconst char *arg = *++argv;\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex dde3f11fec..6ae8cfb271 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -398,6 +398,7 @@ diff --no-index --raw --no-abbrev dir2 dir\n\n diff-tree --pretty --root --stat --compact-summary initial\n diff-tree --pretty -R --root --stat --compact-summary initial\n+diff-tree --pretty --notes initial\n diff-tree --stat --compact-summary initial mode\n diff-tree -R --stat --compact-summary initial mode\n EOF\n--\n2.26.0.106.g9fadedd637\n"},{"id":"395545","messageId":"20200415222905.GA3595509@coredump.intra.peff.net","threadId":"53239","inReplyTo":"20200415220242.GA45241@syl.local","subject":"Re: [PATCH 1/2] range-diff: fix a crash in parsing git-log output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-15T22:29:05Z","receivedAt":"2020-04-15T22:29:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 15, 2020 at 04:02:42PM -0600, Taylor Blau wrote:\n\n> Subject: [PATCH] diff-tree.c: load notes machinery with '--notes'\n> \n> Since its introduction in 7249e91 (revision.c: support --notes\n> command-line option, 2011-03-29), combining '--notes' with '--pretty'\n> causes 'git diff-tree' to fail a runtime assertion:\n> \n> $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes\n> commit 8f3d9f354286745c751374f5f1fcafee6b3f3136\n> git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.\n> Aborted\n> \n> This failure is due to diff-tree not calling 'load_display_notes' to\n> initialize the notes machinery.\n\nYes, I think that's the problem that I saw. And this definitely fixes\nthat case, but I think there's another related one.\n\n $ git notes add -m foo\n $ git log -1 --format='%h %N'\n 94316974f7 foo\n $ git rev-list -1 HEAD | git diff-tree --stdin --format='%h %N' -s\n 94316974f7e27dccc6b87f3946bce5d2fc252dc2 %N\n\nThis is true even with your patch. With your patch I can add --notes to\nget the right output:\n\n $ git rev-list -1 HEAD | git diff-tree --stdin --format='%h %N' -s --notes\n 94316974f7e27dccc6b87f3946bce5d2fc252dc2 foo\n\n(It's also slightly curious that %h doesn't abbreviate in diff-tree; I\nguess this is a side effect of the plumbing having no default abbrev\nsetting; it may be simplest to just live with it).\n\n> @@ -126,6 +126,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n> \n> \tprecompose_argv(argc, argv);\n> \targc = setup_revisions(argc, argv, opt, &s_r_opt);\n> +\tif (opt->show_notes)\n> +\t\tload_display_notes(&opt->notes_opt);\n\nIn git-log we have the equivalent of these new lines, but just before it\nwe check the userformat, too:\n\n memset(&w, 0, sizeof(w));\n userformat_find_requirements(NULL, &w);\n \n if (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n rev->show_notes = 1;\n if (rev->show_notes)\n load_display_notes(&rev->notes_opt);\n\nI think we'd want to do the same here. Even though it's plumbing, I\ncan't think of any reason why somebody would _not_ want notes to be\nauto-enabled when they say \"%N\".\n\n> Ordinarily, this failure isn't triggered, because it requires passing\n> both '--notes' and '--pretty'. Specifically, passing '--pretty' sets\n> 'opt->verbose_header', causing 'show_log()' to eventually call\n> 'format_display_notes()', which expects a non-NULL 'display_note_trees'.\n> Without initializing the notes machinery, 'display_note_trees' remains\n> NULL, and thus triggers an assertion failure. This doesn't occur without\n> '--pretty' since we never call 'format_display_notes()' without it.\n\nIt's not just --pretty, of course, but any option that causes us to\nactually try to format notes (--format, --oneline, etc).\n\n-Peff\n"},{"id":"395560","messageId":"20200416010734.GE36156@syl.local","threadId":"53239","inReplyTo":"pull.760.v2.git.git.1586982745.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] range-diff: fix a crash in parsing git-log output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-16T01:07:34Z","receivedAt":"2020-04-16T01:07:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Vasil,\n\nThanks for sending a v2. This all looks good to me.\n\nOn Wed, Apr 15, 2020 at 08:32:23PM +0000, Vasil Dimov via GitGitGadget wrote:\n> * override a possibly user-customized format.pretty that would render git\n> log output unparsable by git range-diff\n>\n>\n> * don't use negative string precision, e.g. \"%.*s\", -5, \"foo\"\n>\n>\n>\n> Changes since v1:\n>\n> * Fixed a typo in the commit message (found by Taylor Blau)\n>\n> Vasil Dimov (2):\n> range-diff: fix a crash in parsing git-log output\n> range-diff: avoid negative string precision\n>\n> range-diff.c | 18 +++++++++++++++++-\n> t/t3206-range-diff.sh | 10 ++++++++++\n> 2 files changed, 27 insertions(+), 1 deletion(-)\n>\n> [snip]\n> --\n> gitgitgadget\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"}]}