{"thread":{"id":"51985","subject":"Regression in v2.23","startedAt":"2019-10-07T11:06:48Z","lastAt":"2019-10-09T07:42:04Z","messageCount":10,"participants":["Uwe Kleine-König","Thomas Gummerer","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"383576","messageId":"20191007110645.7eljju2h6g7ts7lf@pengutronix.de","threadId":"51985","inReplyTo":null,"subject":"Regression in v2.23","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2019-10-07T11:06:45Z","receivedAt":"2019-10-07T11:06:48Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\nWith git 2.23.0 I have:\n\n\tuwe@taurus:~/tmp/rangediff-segfault$ git init\n\tInitialized empty Git repository in /home/uwe/tmp/rangediff-segfault/.git/\n\tuwe@taurus:~/tmp/rangediff-segfault$ echo root > root\n\tuwe@taurus:~/tmp/rangediff-segfault$ git add root\n\tuwe@taurus:~/tmp/rangediff-segfault$ git commit -m root\n\t[master (root-commit) b0feddb2dee8] root\n\t 1 file changed, 1 insertion(+)\n\t create mode 100644 root\n\tuwe@taurus:~/tmp/rangediff-segfault$ echo content > file\n\tuwe@taurus:~/tmp/rangediff-segfault$ chmod +x file\n\tuwe@taurus:~/tmp/rangediff-segfault$ git add file\n\tuwe@taurus:~/tmp/rangediff-segfault$ git commit -m file\n\t[master 45b547c57acd] file\n\t 1 file changed, 1 insertion(+)\n\t create mode 100755 file\n\tuwe@taurus:~/tmp/rangediff-segfault$ chmod -x file\n\tuwe@taurus:~/tmp/rangediff-segfault$ git add file\n\tuwe@taurus:~/tmp/rangediff-segfault$ git commit -m 'chmod -x'\n\t[master eaa5d3b98caa] chmod -x\n\t 1 file changed, 0 insertions(+), 0 deletions(-)\n\t mode change 100755 => 100644 file\n\tuwe@taurus:~/tmp/rangediff-segfault$ git range-diff @~2..@~ @~2..\n\tSegmentation fault (core dumped)\n\nBisecting points to b66885a30cb84fc61986bc4eea805a31fdbea79a, current master\n(b744c3af07a15aaeb1b82fab689995fd5528f120) segfaults in the same way.\n\nThis is somehow similar to\nhttps://public-inbox.org/git/20190923101929.GA18205@kitsune.suse.cz/ but\nthe patch by Johannes Schindelin sent in\nhttps://public-inbox.org/git/pull.373.git.gitgitgadget@gmail.com/\ndoesn't help me.\n\nFor me the segfault also happens in\n\n\tstrbuf_addstr(&buf, patch.new_name);\n\nwith patch.new_name being NULL.\n\nThe matching backtrace and patch object looks as follows:\n\n\t(gdb) bt\n\t#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65\n\t#1  0x0000555cc448949c in strbuf_addstr (s=<optimized out>, sb=0x7ffcd1d9ef00)\n\t    at strbuf.h:292\n\t#2  read_patches (range=range@entry=0x555cc5dc2b70 \"@~2..\", \n\t    list=list@entry=0x7ffcd1d9f280) at range-diff.c:126\n\t#3  0x0000555cc44898a8 in show_range_diff (range1=0x555cc5dc2b50 \"@~2..@~\", \n\t    range2=0x555cc5dc2b70 \"@~2..\", creation_factor=60, dual_color=1, \n\t    diffopt=diffopt@entry=0x7ffcd1d9f680) at range-diff.c:507\n\t#4  0x0000555cc4397aa6 in cmd_range_diff (argc=<optimized out>, \n\t    argv=<optimized out>, prefix=<optimized out>) at builtin/range-diff.c:80\n\t#5  0x0000555cc4328494 in run_builtin (argv=<optimized out>, \n\t    argc=<optimized out>, p=<optimized out>) at git.c:445\n\t#6  handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:674\n\t#7  0x0000555cc4329554 in run_argv (argv=0x7ffcd1d9f9e0, argcp=0x7ffcd1d9f9ec)\n\t    at git.c:741\n\t#8  cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:872\n\t#9  0x0000555cc432803a in main (argc=4, argv=0x7ffcd1d9fc78)\n\t    at common-main.c:52\n\t(gdb) up 2\n\t#2  read_patches (range=range@entry=0x555cc5dc2b70 \"@~2..\", \n\t    list=list@entry=0x7ffcd1d9f280) at range-diff.c:126\n\t126\trange-diff.c: No such file or directory.\n\t(gdb) print patch\n\t$1 = {new_name = 0x0, old_name = 0x0, def_name = 0x555cc5dc98c0 \"file\", \n\t  old_mode = 33261, new_mode = 33188, is_new = 0, is_delete = 0, rejected = 0, \n\t  ws_rule = 0, lines_added = 0, lines_deleted = 0, score = 0, \n\t  extension_linenr = 0, is_toplevel_relative = 0, inaccurate_eof = 0, \n\t  is_binary = 0, is_copy = 0, is_rename = 0, recount = 0, \n\t  conflicted_threeway = 0, direct_to_threeway = 0, crlf_in_old = 0, \n\t  fragments = 0x0, result = 0x0, resultsize = 0, \n\t  old_oid_prefix = '\\000' <repeats 64 times>, \n\t  new_oid_prefix = '\\000' <repeats 64 times>, next = 0x0, threeway_stage = {{\n\t      hash = '\\000' <repeats 31 times>}, {hash = '\\000' <repeats 31 times>}, {\n\t      hash = '\\000' <repeats 31 times>}}}\n\nI guess you are able to work out the details with this information. If you need\nmore input, please Cc: me on replies.\n\nBest regards\nUwe\n\n-- \nPengutronix e.K.                           | Uwe Kleine-König            |\nIndustrial Linux Solutions                 | http://www.pengutronix.de/  |\n"},{"id":"383577","messageId":"20191007134831.GA74671@cat","threadId":"51985","inReplyTo":"20191007110645.7eljju2h6g7ts7lf@pengutronix.de","subject":"Re: Regression in v2.23","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-10-07T13:48:31Z","receivedAt":"2019-10-07T13:48:38Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 10/07, Uwe Kleine-König wrote:\n> Hello,\n> \n> With git 2.23.0 I have:\n> \n> \tuwe@taurus:~/tmp/rangediff-segfault$ git init\n> \tInitialized empty Git repository in /home/uwe/tmp/rangediff-segfault/.git/\n> \tuwe@taurus:~/tmp/rangediff-segfault$ echo root > root\n> \tuwe@taurus:~/tmp/rangediff-segfault$ git add root\n> \tuwe@taurus:~/tmp/rangediff-segfault$ git commit -m root\n> \t[master (root-commit) b0feddb2dee8] root\n> \t 1 file changed, 1 insertion(+)\n> \t create mode 100644 root\n> \tuwe@taurus:~/tmp/rangediff-segfault$ echo content > file\n> \tuwe@taurus:~/tmp/rangediff-segfault$ chmod +x file\n> \tuwe@taurus:~/tmp/rangediff-segfault$ git add file\n> \tuwe@taurus:~/tmp/rangediff-segfault$ git commit -m file\n> \t[master 45b547c57acd] file\n> \t 1 file changed, 1 insertion(+)\n> \t create mode 100755 file\n> \tuwe@taurus:~/tmp/rangediff-segfault$ chmod -x file\n> \tuwe@taurus:~/tmp/rangediff-segfault$ git add file\n> \tuwe@taurus:~/tmp/rangediff-segfault$ git commit -m 'chmod -x'\n> \t[master eaa5d3b98caa] chmod -x\n> \t 1 file changed, 0 insertions(+), 0 deletions(-)\n> \t mode change 100755 => 100644 file\n> \tuwe@taurus:~/tmp/rangediff-segfault$ git range-diff @~2..@~ @~2..\n> \tSegmentation fault (core dumped)\n> \n> Bisecting points to b66885a30cb84fc61986bc4eea805a31fdbea79a, current master\n> (b744c3af07a15aaeb1b82fab689995fd5528f120) segfaults in the same way.\n> \n> This is somehow similar to\n> https://public-inbox.org/git/20190923101929.GA18205@kitsune.suse.cz/ but\n> the patch by Johannes Schindelin sent in\n> https://public-inbox.org/git/pull.373.git.gitgitgadget@gmail.com/\n> doesn't help me.\n\nThanks for the report, and testing if those patches help.\n\n> For me the segfault also happens in\n> \n> \tstrbuf_addstr(&buf, patch.new_name);\n> \n> with patch.new_name being NULL.\n> \n> The matching backtrace and patch object looks as follows:\n> \n> \t(gdb) bt\n> \t#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65\n> \t#1  0x0000555cc448949c in strbuf_addstr (s=<optimized out>, sb=0x7ffcd1d9ef00)\n> \t    at strbuf.h:292\n> \t#2  read_patches (range=range@entry=0x555cc5dc2b70 \"@~2..\", \n> \t    list=list@entry=0x7ffcd1d9f280) at range-diff.c:126\n> \t#3  0x0000555cc44898a8 in show_range_diff (range1=0x555cc5dc2b50 \"@~2..@~\", \n> \t    range2=0x555cc5dc2b70 \"@~2..\", creation_factor=60, dual_color=1, \n> \t    diffopt=diffopt@entry=0x7ffcd1d9f680) at range-diff.c:507\n> \t#4  0x0000555cc4397aa6 in cmd_range_diff (argc=<optimized out>, \n> \t    argv=<optimized out>, prefix=<optimized out>) at builtin/range-diff.c:80\n> \t#5  0x0000555cc4328494 in run_builtin (argv=<optimized out>, \n> \t    argc=<optimized out>, p=<optimized out>) at git.c:445\n> \t#6  handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:674\n> \t#7  0x0000555cc4329554 in run_argv (argv=0x7ffcd1d9f9e0, argcp=0x7ffcd1d9f9ec)\n> \t    at git.c:741\n> \t#8  cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:872\n> \t#9  0x0000555cc432803a in main (argc=4, argv=0x7ffcd1d9fc78)\n> \t    at common-main.c:52\n> \t(gdb) up 2\n> \t#2  read_patches (range=range@entry=0x555cc5dc2b70 \"@~2..\", \n> \t    list=list@entry=0x7ffcd1d9f280) at range-diff.c:126\n> \t126\trange-diff.c: No such file or directory.\n> \t(gdb) print patch\n> \t$1 = {new_name = 0x0, old_name = 0x0, def_name = 0x555cc5dc98c0 \"file\", \n> \t  old_mode = 33261, new_mode = 33188, is_new = 0, is_delete = 0, rejected = 0, \n> \t  ws_rule = 0, lines_added = 0, lines_deleted = 0, score = 0, \n> \t  extension_linenr = 0, is_toplevel_relative = 0, inaccurate_eof = 0, \n> \t  is_binary = 0, is_copy = 0, is_rename = 0, recount = 0, \n> \t  conflicted_threeway = 0, direct_to_threeway = 0, crlf_in_old = 0, \n> \t  fragments = 0x0, result = 0x0, resultsize = 0, \n> \t  old_oid_prefix = '\\000' <repeats 64 times>, \n> \t  new_oid_prefix = '\\000' <repeats 64 times>, next = 0x0, threeway_stage = {{\n> \t      hash = '\\000' <repeats 31 times>}, {hash = '\\000' <repeats 31 times>}, {\n> \t      hash = '\\000' <repeats 31 times>}}}\n> \n> I guess you are able to work out the details with this information. If you need\n> more input, please Cc: me on replies.\n\nIndeed, I was able to figure out what's going wrong from the\nbacktrace.  Here's a patch.  It's not ready for inclusion, as I still\nneed to write some tests, and make sure I'm not breaking something\nelse.\n\nI can hopefully do some more testing and write automated tests later\ntoday or tomorrow.  In the meantime it would be awesome if you could\nconfirm if this patch fixes the problem you were seeing.\n\nI have pushed this to GitHub as well if you prefer that to applying\nthe patch yourself: https://github.com/tgummerer/git tg/range-diff-mode-only-change\n\n--- >8 ---\nSubject: [PATCH] range-diff: don't segfault with mode-only changes\n\nIf we don't have a new file, deleted file or renamed file in a diff,\nwe currently add 'patch.new_name' to the range-diff header.  This\nworks well for files that are changed.  However if we have a pure mode\nchange, 'patch.new_name' is NULL, and thus range-diff segfaults.\n\nWe can however rely on 'patch.def_name' in that case, which is\nextracted from the 'diff --git' line and should be equal to\n'patch.new_name'.  Use that instead to avoid the segfault.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n range-diff.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex ba1e9a4265..d8d906b3c6 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -116,20 +116,20 @@ static int read_patches(const char *range, struct string_list *list)\n \t\t\tif (len < 0)\n \t\t\t\tdie(_(\"could not parse git header '%.*s'\"), (int)len, line);\n \t\t\tstrbuf_addstr(&buf, \" ## \");\n-\t\t\tif (patch.is_new > 0)\n+\t\t\tfree(current_filename);\n+\t\t\tif (patch.is_new > 0) {\n \t\t\t\tstrbuf_addf(&buf, \"%s (new)\", patch.new_name);\n-\t\t\telse if (patch.is_delete > 0)\n+\t\t\t\tcurrent_filename = xstrdup(patch.new_name);\n+\t\t\t} else if (patch.is_delete > 0) {\n \t\t\t\tstrbuf_addf(&buf, \"%s (deleted)\", patch.old_name);\n-\t\t\telse if (patch.is_rename)\n-\t\t\t\tstrbuf_addf(&buf, \"%s => %s\", patch.old_name, patch.new_name);\n-\t\t\telse\n-\t\t\t\tstrbuf_addstr(&buf, patch.new_name);\n-\n-\t\t\tfree(current_filename);\n-\t\t\tif (patch.is_delete > 0)\n \t\t\t\tcurrent_filename = xstrdup(patch.old_name);\n-\t\t\telse\n+\t\t\t} else if (patch.is_rename) {\n+\t\t\t\tstrbuf_addf(&buf, \"%s => %s\", patch.old_name, patch.new_name);\n \t\t\t\tcurrent_filename = xstrdup(patch.new_name);\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addstr(&buf, patch.def_name);\n+\t\t\t\tcurrent_filename = xstrdup(patch.def_name);\n+\t\t\t}\n \n \t\t\tif (patch.new_mode && patch.old_mode &&\n \t\t\t    patch.old_mode != patch.new_mode)\n-- \n2.23.0.501.gb744c3af07\n\n"},{"id":"383646","messageId":"xmqqh84knd7l.fsf@gitster-ct.c.googlers.com","threadId":"51985","inReplyTo":"20191007134831.GA74671@cat","subject":"Re: Regression in v2.23","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-08T03:11:26Z","receivedAt":"2019-10-08T03:11:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> We can however rely on 'patch.def_name' in that case, which is\n> extracted from the 'diff --git' line and should be equal to\n> 'patch.new_name'.  Use that instead to avoid the segfault.\n\nThis patch makes the way this function calls parse_git_diff_header()\nmore in line with the way how it is used by its original caller in\napply.c::find_header(), but not quite.\n\nI have to wonder if we want to move a bit of code around so that\ncallers of parse_git_diff_header() do not have to worry about\ndef_name and can rely on new_name and old_name fields correctly\nfilled.\n\nThere was only one caller of the parse_git_diff_header() function\nbefore range-diff.  The division of labour between find_header() and\nparse_git_diff_header() did not make any difference to the consumers\nof the new/old_name fields.  They only cared that they do not have\nto worry about def_name.  But by calling parse_git_diff_header()\nthat forces the caller to worry about def_name (which is done by\nfind_header() to free its callers from doing so), range-diff took\nresponsibility of caring, which was suboptimal.  The interface could\nhave been a bit more cleaned up before we started to reuse it in the\nnew caller, and as this bug shows, it may be time to do so now, no?\n\nPerhaps before returing, parse_git_diff_header() should fill the two\nnames with xstrdup() of def_name if (!old_name && !new_name &&\n!!def_name); all other cases the existing caller and this new caller\nwould work unchanged correctly, no?\n"},{"id":"383653","messageId":"20191008062452.fdueej5clof7r2m5@pengutronix.de","threadId":"51985","inReplyTo":"20191007134831.GA74671@cat","subject":"Re: Regression in v2.23","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2019-10-08T06:24:52Z","receivedAt":"2019-10-08T06:24:55Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello Thomas,\n\nOn Mon, Oct 07, 2019 at 02:48:31PM +0100, Thomas Gummerer wrote:\n> I can hopefully do some more testing and write automated tests later\n> today or tomorrow.  In the meantime it would be awesome if you could\n> confirm if this patch fixes the problem you were seeing.\n\nI assume you already tested on my constructed example from the report. I\njust tested on the original and there is works fine, too.\n\nBest regards\nUwe\n\n-- \nPengutronix e.K.                           | Uwe Kleine-König            |\nIndustrial Linux Solutions                 | http://www.pengutronix.de/  |\n"},{"id":"383657","messageId":"nycvar.QRO.7.76.6.1910080943100.46@tvgsbejvaqbjf.bet","threadId":"51985","inReplyTo":"xmqqh84knd7l.fsf@gitster-ct.c.googlers.com","subject":"Re: Regression in v2.23","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-08T07:43:28Z","receivedAt":"2019-10-08T07:43:39Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 8 Oct 2019, Junio C Hamano wrote:\n\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n>\n> > We can however rely on 'patch.def_name' in that case, which is\n> > extracted from the 'diff --git' line and should be equal to\n> > 'patch.new_name'.  Use that instead to avoid the segfault.\n>\n> This patch makes the way this function calls parse_git_diff_header()\n> more in line with the way how it is used by its original caller in\n> apply.c::find_header(), but not quite.\n>\n> I have to wonder if we want to move a bit of code around so that\n> callers of parse_git_diff_header() do not have to worry about\n> def_name and can rely on new_name and old_name fields correctly\n> filled.\n>\n> There was only one caller of the parse_git_diff_header() function\n> before range-diff.  The division of labour between find_header() and\n> parse_git_diff_header() did not make any difference to the consumers\n> of the new/old_name fields.  They only cared that they do not have\n> to worry about def_name.  But by calling parse_git_diff_header()\n> that forces the caller to worry about def_name (which is done by\n> find_header() to free its callers from doing so), range-diff took\n> responsibility of caring, which was suboptimal.  The interface could\n> have been a bit more cleaned up before we started to reuse it in the\n> new caller, and as this bug shows, it may be time to do so now, no?\n>\n> Perhaps before returing, parse_git_diff_header() should fill the two\n> names with xstrdup() of def_name if (!old_name && !new_name &&\n> !!def_name); all other cases the existing caller and this new caller\n> would work unchanged correctly, no?\n\nFWIW I totally agree.\n\nCiao,\nDscho\n"},{"id":"383658","messageId":"nycvar.QRO.7.76.6.1910080932560.46@tvgsbejvaqbjf.bet","threadId":"51985","inReplyTo":"20191007134831.GA74671@cat","subject":"Re: Regression in v2.23","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-08T07:44:36Z","receivedAt":"2019-10-08T07:44:40Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Mon, 7 Oct 2019, Thomas Gummerer wrote:\n\n> Subject: [PATCH] range-diff: don't segfault with mode-only changes\n>\n> If we don't have a new file, deleted file or renamed file in a diff,\n> we currently add 'patch.new_name' to the range-diff header.  This\n> works well for files that are changed.  However if we have a pure mode\n> change, 'patch.new_name' is NULL, and thus range-diff segfaults.\n>\n> We can however rely on 'patch.def_name' in that case, which is\n> extracted from the 'diff --git' line and should be equal to\n> 'patch.new_name'.  Use that instead to avoid the segfault.\n>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  range-diff.c | 20 ++++++++++----------\n>  1 file changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/range-diff.c b/range-diff.c\n> index ba1e9a4265..d8d906b3c6 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -116,20 +116,20 @@ static int read_patches(const char *range, struct string_list *list)\n>  \t\t\tif (len < 0)\n>  \t\t\t\tdie(_(\"could not parse git header '%.*s'\"), (int)len, line);\n>  \t\t\tstrbuf_addstr(&buf, \" ## \");\n> -\t\t\tif (patch.is_new > 0)\n> +\t\t\tfree(current_filename);\n> +\t\t\tif (patch.is_new > 0) {\n>  \t\t\t\tstrbuf_addf(&buf, \"%s (new)\", patch.new_name);\n> -\t\t\telse if (patch.is_delete > 0)\n> +\t\t\t\tcurrent_filename = xstrdup(patch.new_name);\n> +\t\t\t} else if (patch.is_delete > 0) {\n>  \t\t\t\tstrbuf_addf(&buf, \"%s (deleted)\", patch.old_name);\n> -\t\t\telse if (patch.is_rename)\n> -\t\t\t\tstrbuf_addf(&buf, \"%s => %s\", patch.old_name, patch.new_name);\n> -\t\t\telse\n> -\t\t\t\tstrbuf_addstr(&buf, patch.new_name);\n> -\n> -\t\t\tfree(current_filename);\n> -\t\t\tif (patch.is_delete > 0)\n>  \t\t\t\tcurrent_filename = xstrdup(patch.old_name);\n> -\t\t\telse\n> +\t\t\t} else if (patch.is_rename) {\n> +\t\t\t\tstrbuf_addf(&buf, \"%s => %s\", patch.old_name, patch.new_name);\n>  \t\t\t\tcurrent_filename = xstrdup(patch.new_name);\n> +\t\t\t} else {\n> +\t\t\t\tstrbuf_addstr(&buf, patch.def_name);\n> +\t\t\t\tcurrent_filename = xstrdup(patch.def_name);\n> +\t\t\t}\n>\n>  \t\t\tif (patch.new_mode && patch.old_mode &&\n>  \t\t\t    patch.old_mode != patch.new_mode)\n> --\n\nI am not quite sure that this fixes it... Here is my regression test case:\n\n-- snipsnap --\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex ec548654ce1..6aca7f5a5b1 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -354,4 +354,18 @@ test_expect_success 'format-patch --range-diff as commentary' '\n \tgrep \"> 1: .* new message\" 0001-*\n '\n\n+test_expect_success 'range-diff and mode-only changes' '\n+\tgit switch -c mode-only &&\n+\n+\ttest_commit mode-only &&\n+\n+\t: pretend it is executable &&\n+\tgit add --chmod=+x mode-only.t &&\n+\tchmod a+x mode-only.t &&\n+\ttest_tick &&\n+\tgit commit -m mode-only &&\n+\n+\tgit range-diff @^...\n+'\n+\n test_done\n\n\n"},{"id":"383659","messageId":"nycvar.QRO.7.76.6.1910080947070.46@tvgsbejvaqbjf.bet","threadId":"51985","inReplyTo":"nycvar.QRO.7.76.6.1910080932560.46@tvgsbejvaqbjf.bet","subject":"Re: Regression in v2.23","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-08T07:49:37Z","receivedAt":"2019-10-08T07:49:42Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Tue, 8 Oct 2019, Johannes Schindelin wrote:\n\n> On Mon, 7 Oct 2019, Thomas Gummerer wrote:\n>\n> > Subject: [PATCH] range-diff: don't segfault with mode-only changes\n> >\n> > If we don't have a new file, deleted file or renamed file in a diff,\n> > we currently add 'patch.new_name' to the range-diff header.  This\n> > works well for files that are changed.  However if we have a pure mode\n> > change, 'patch.new_name' is NULL, and thus range-diff segfaults.\n> >\n> > We can however rely on 'patch.def_name' in that case, which is\n> > extracted from the 'diff --git' line and should be equal to\n> > 'patch.new_name'.  Use that instead to avoid the segfault.\n> >\n> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> > ---\n> >  range-diff.c | 20 ++++++++++----------\n> >  1 file changed, 10 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/range-diff.c b/range-diff.c\n> > index ba1e9a4265..d8d906b3c6 100644\n> > --- a/range-diff.c\n> > +++ b/range-diff.c\n> > @@ -116,20 +116,20 @@ static int read_patches(const char *range, struct string_list *list)\n> >  \t\t\tif (len < 0)\n> >  \t\t\t\tdie(_(\"could not parse git header '%.*s'\"), (int)len, line);\n> >  \t\t\tstrbuf_addstr(&buf, \" ## \");\n> > -\t\t\tif (patch.is_new > 0)\n> > +\t\t\tfree(current_filename);\n> > +\t\t\tif (patch.is_new > 0) {\n> >  \t\t\t\tstrbuf_addf(&buf, \"%s (new)\", patch.new_name);\n> > -\t\t\telse if (patch.is_delete > 0)\n> > +\t\t\t\tcurrent_filename = xstrdup(patch.new_name);\n> > +\t\t\t} else if (patch.is_delete > 0) {\n> >  \t\t\t\tstrbuf_addf(&buf, \"%s (deleted)\", patch.old_name);\n> > -\t\t\telse if (patch.is_rename)\n> > -\t\t\t\tstrbuf_addf(&buf, \"%s => %s\", patch.old_name, patch.new_name);\n> > -\t\t\telse\n> > -\t\t\t\tstrbuf_addstr(&buf, patch.new_name);\n> > -\n> > -\t\t\tfree(current_filename);\n> > -\t\t\tif (patch.is_delete > 0)\n> >  \t\t\t\tcurrent_filename = xstrdup(patch.old_name);\n> > -\t\t\telse\n> > +\t\t\t} else if (patch.is_rename) {\n> > +\t\t\t\tstrbuf_addf(&buf, \"%s => %s\", patch.old_name, patch.new_name);\n> >  \t\t\t\tcurrent_filename = xstrdup(patch.new_name);\n> > +\t\t\t} else {\n> > +\t\t\t\tstrbuf_addstr(&buf, patch.def_name);\n> > +\t\t\t\tcurrent_filename = xstrdup(patch.def_name);\n> > +\t\t\t}\n> >\n> >  \t\t\tif (patch.new_mode && patch.old_mode &&\n> >  \t\t\t    patch.old_mode != patch.new_mode)\n> > --\n>\n> I am not quite sure that this fixes it...\n\nWhoops. I should learn to distrust `git apply` claiming success when\nrunning in `t/`. (I tried to apply your patch, but nothing was actually\napplied before I ran `make`.)\n\nSo it totally fixes the issue (feel free to just pick up the regression\ntest case).\n\nHaving said that, I would agree with Junio that it'd be nicer to make\n`parse_git_diff_header()` more useful to all of its callers, including\nfuture ones.\n\nSorry for the misreport, and thanks for all the patch,\nDscho\n\n> Here is my regression test case:\n>\n> -- snipsnap --\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index ec548654ce1..6aca7f5a5b1 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -354,4 +354,18 @@ test_expect_success 'format-patch --range-diff as commentary' '\n>  \tgrep \"> 1: .* new message\" 0001-*\n>  '\n>\n> +test_expect_success 'range-diff and mode-only changes' '\n> +\tgit switch -c mode-only &&\n> +\n> +\ttest_commit mode-only &&\n> +\n> +\t: pretend it is executable &&\n> +\tgit add --chmod=+x mode-only.t &&\n> +\tchmod a+x mode-only.t &&\n> +\ttest_tick &&\n> +\tgit commit -m mode-only &&\n> +\n> +\tgit range-diff @^...\n> +'\n> +\n>  test_done\n>\n>\n>\n"},{"id":"383692","messageId":"20191008173843.GC74671@cat","threadId":"51985","inReplyTo":"20191007134831.GA74671@cat","subject":"[PATCH v2] range-diff: don't segfault with mode-only changes","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-10-08T17:38:43Z","receivedAt":"2019-10-08T17:38:52Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"In ef283b3699 (\"apply: make parse_git_diff_header public\", 2019-07-11)\nthe 'parse_git_diff_header' function was made public and useable by\ncallers outside of apply.c.\n\nHowever it was missed that its (then) only caller, 'find_header' did\nsome error handling, and completing 'struct patch' appropriately.\n\nrange-diff then started using this function, and tried to handle this\nappropriately itself, but fell short in some cases.  This in turn\nwould lead to range-diff segfaulting when there are mode-only changes\nin a range.\n\nMove the error handling and completing of the struct into the\n'parse_git_diff_header' function, so other callers can take advantage\nof it.  This fixes the segfault in 'git range-diff'.\n\nReported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n\nThanks Junio and Dscho for your reviews.  I decided to lift the whole\nerror handling behaviour from find_header into parse_git_diff_header,\ninstead of just filling the two names with xstrdup(def_name) if\n(!old_name && !new_name && !!def_name).  I think the additional\ninformation presented there can be useful.  For example we would have\ngotten some \"error: git diff header lacks filename information\"\ninstead of a segfault for the problem described in\nhttps://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/T/#me576615d7a151cf2ed46186c482fbd88f9959914.\n\nDscho, I didn't re-use your test case here as I had already written\none, and think what I have is slightly nicer in that it follows what\nmost other range-diff tests do in using the fast-exported history.  It\nalso expands the test coverage slightly, as we currently don't have\nany coverage of the mode-change header, but will with this test.\n\nThe downside is of course that the fast export script is harder to\nunderstand than the test you had, at least for me, but I think the\ntradeoff of having the additional test coverage, and having it similar\nto the rest of the test script is worth it.  If you strongly prefer\nyour test though I'm not going to be unhappy to use that :)\n\n apply.c                | 43 +++++++++++++++++++++---------------------\n t/t3206-range-diff.sh  | 40 +++++++++++++++++++++++++++++++++++++++\n t/t3206/history.export | 31 +++++++++++++++++++++++++++++-\n 3 files changed, 92 insertions(+), 22 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 57a61f2881..f8a046a6a5 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1361,11 +1361,32 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\tif (check_header_line(*linenr, patch))\n \t\t\t\treturn -1;\n \t\t\tif (res > 0)\n-\t\t\t\treturn offset;\n+\t\t\t\tgoto done;\n \t\t\tbreak;\n \t\t}\n \t}\n \n+done:\n+\tif (!patch->old_name && !patch->new_name) {\n+\t\tif (!patch->def_name) {\n+\t\t\terror(Q_(\"git diff header lacks filename information when removing \"\n+\t\t\t\t \"%d leading pathname component (line %d)\",\n+\t\t\t\t \"git diff header lacks filename information when removing \"\n+\t\t\t\t \"%d leading pathname components (line %d)\",\n+\t\t\t\t parse_hdr_state.p_value),\n+\t\t\t      parse_hdr_state.p_value, *linenr);\n+\t\t\treturn -128;\n+\t\t}\n+\t\tpatch->old_name = xstrdup(patch->def_name);\n+\t\tpatch->new_name = xstrdup(patch->def_name);\n+\t}\n+\tif ((!patch->new_name && !patch->is_delete) ||\n+\t    (!patch->old_name && !patch->is_new)) {\n+\t\terror(_(\"git diff header lacks filename information \"\n+\t\t\t\"(line %d)\"), *linenr);\n+\t\treturn -128;\n+\t}\n+\tpatch->is_toplevel_relative = 1;\n \treturn offset;\n }\n \n@@ -1546,26 +1567,6 @@ static int find_header(struct apply_state *state,\n \t\t\t\treturn -128;\n \t\t\tif (git_hdr_len <= len)\n \t\t\t\tcontinue;\n-\t\t\tif (!patch->old_name && !patch->new_name) {\n-\t\t\t\tif (!patch->def_name) {\n-\t\t\t\t\terror(Q_(\"git diff header lacks filename information when removing \"\n-\t\t\t\t\t\t\t\"%d leading pathname component (line %d)\",\n-\t\t\t\t\t\t\t\"git diff header lacks filename information when removing \"\n-\t\t\t\t\t\t\t\"%d leading pathname components (line %d)\",\n-\t\t\t\t\t\t\tstate->p_value),\n-\t\t\t\t\t\t     state->p_value, state->linenr);\n-\t\t\t\t\treturn -128;\n-\t\t\t\t}\n-\t\t\t\tpatch->old_name = xstrdup(patch->def_name);\n-\t\t\t\tpatch->new_name = xstrdup(patch->def_name);\n-\t\t\t}\n-\t\t\tif ((!patch->new_name && !patch->is_delete) ||\n-\t\t\t    (!patch->old_name && !patch->is_new)) {\n-\t\t\t\terror(_(\"git diff header lacks filename information \"\n-\t\t\t\t\t     \"(line %d)\"), state->linenr);\n-\t\t\t\treturn -128;\n-\t\t\t}\n-\t\t\tpatch->is_toplevel_relative = 1;\n \t\t\t*hdrsize = git_hdr_len;\n \t\t\treturn offset;\n \t\t}\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex ec548654ce..5b87fead2e 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -226,6 +226,46 @@ test_expect_success 'renamed file' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'file with mode only change' '\n+\tgit range-diff --no-color --submodule=log topic...mode-only-change >actual &&\n+\tsed s/Z/\\ /g >expected <<-EOF &&\n+\t1:  fccce22 ! 1:  4d39cb3 s/4/A/\n+\t    @@ Metadata\n+\t    ZAuthor: Thomas Rast <trast@inf.ethz.ch>\n+\t    Z\n+\t    Z ## Commit message ##\n+\t    -    s/4/A/\n+\t    +    s/4/A/ + add other-file\n+\t    Z\n+\t    Z ## file ##\n+\t    Z@@\n+\t    @@ file\n+\t    Z A\n+\t    Z 6\n+\t    Z 7\n+\t    +\n+\t    + ## other-file (new) ##\n+\t2:  147e64e ! 2:  26c107f s/11/B/\n+\t    @@ Metadata\n+\t    ZAuthor: Thomas Rast <trast@inf.ethz.ch>\n+\t    Z\n+\t    Z ## Commit message ##\n+\t    -    s/11/B/\n+\t    +    s/11/B/ + mode change other-file\n+\t    Z\n+\t    Z ## file ##\n+\t    Z@@ file: A\n+\t    @@ file: A\n+\t    Z 12\n+\t    Z 13\n+\t    Z 14\n+\t    +\n+\t    + ## other-file (mode change 100644 => 100755) ##\n+\t3:  a63e992 = 3:  4c1e0f5 s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'file added and later removed' '\n \tgit range-diff --no-color --submodule=log topic...added-removed >actual &&\n \tsed s/Z/\\ /g >expected <<-EOF &&\ndiff --git a/t/t3206/history.export b/t/t3206/history.export\nindex 7bb3814962..4c808e5b3b 100644\n--- a/t/t3206/history.export\n+++ b/t/t3206/history.export\n@@ -55,7 +55,7 @@ A\n 19\n 20\n \n-commit refs/heads/topic\n+commit refs/heads/mode-only-change\n mark :4\n author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200\n committer Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200\n@@ -678,3 +678,32 @@ s/12/B/\n from :55\n M 100644 :9 renamed-file\n \n+commit refs/heads/mode-only-change\n+mark :57\n+author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200\n+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473767 +0100\n+data 24\n+s/4/A/ + add other-file\n+from :4\n+M 100644 :5 file\n+M 100644 :49 other-file\n+\n+commit refs/heads/mode-only-change\n+mark :58\n+author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200\n+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100\n+data 33\n+s/11/B/ + mode change other-file\n+from :57\n+M 100644 :7 file\n+M 100755 :49 other-file\n+\n+commit refs/heads/mode-only-change\n+mark :59\n+author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200\n+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100\n+data 8\n+s/12/B/\n+from :58\n+M 100644 :9 file\n+\n-- \n2.23.0.501.gb744c3af07\n\n"},{"id":"383707","messageId":"nycvar.QRO.7.76.6.1910082144250.46@tvgsbejvaqbjf.bet","threadId":"51985","inReplyTo":"20191008173843.GC74671@cat","subject":"Re: [PATCH v2] range-diff: don't segfault with mode-only changes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-08T19:44:50Z","receivedAt":"2019-10-08T19:44:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Tue, 8 Oct 2019, Thomas Gummerer wrote:\n\n> In ef283b3699 (\"apply: make parse_git_diff_header public\", 2019-07-11)\n> the 'parse_git_diff_header' function was made public and useable by\n> callers outside of apply.c.\n>\n> However it was missed that its (then) only caller, 'find_header' did\n> some error handling, and completing 'struct patch' appropriately.\n>\n> range-diff then started using this function, and tried to handle this\n> appropriately itself, but fell short in some cases.  This in turn\n> would lead to range-diff segfaulting when there are mode-only changes\n> in a range.\n>\n> Move the error handling and completing of the struct into the\n> 'parse_git_diff_header' function, so other callers can take advantage\n> of it.  This fixes the segfault in 'git range-diff'.\n>\n> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>\n\nAcked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nCiao,\nDscho\n\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>\n> Thanks Junio and Dscho for your reviews.  I decided to lift the whole\n> error handling behaviour from find_header into parse_git_diff_header,\n> instead of just filling the two names with xstrdup(def_name) if\n> (!old_name && !new_name && !!def_name).  I think the additional\n> information presented there can be useful.  For example we would have\n> gotten some \"error: git diff header lacks filename information\"\n> instead of a segfault for the problem described in\n> https://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/T/#me576615d7a151cf2ed46186c482fbd88f9959914.\n>\n> Dscho, I didn't re-use your test case here as I had already written\n> one, and think what I have is slightly nicer in that it follows what\n> most other range-diff tests do in using the fast-exported history.  It\n> also expands the test coverage slightly, as we currently don't have\n> any coverage of the mode-change header, but will with this test.\n>\n> The downside is of course that the fast export script is harder to\n> understand than the test you had, at least for me, but I think the\n> tradeoff of having the additional test coverage, and having it similar\n> to the rest of the test script is worth it.  If you strongly prefer\n> your test though I'm not going to be unhappy to use that :)\n>\n>  apply.c                | 43 +++++++++++++++++++++---------------------\n>  t/t3206-range-diff.sh  | 40 +++++++++++++++++++++++++++++++++++++++\n>  t/t3206/history.export | 31 +++++++++++++++++++++++++++++-\n>  3 files changed, 92 insertions(+), 22 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 57a61f2881..f8a046a6a5 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1361,11 +1361,32 @@ int parse_git_diff_header(struct strbuf *root,\n>  \t\t\tif (check_header_line(*linenr, patch))\n>  \t\t\t\treturn -1;\n>  \t\t\tif (res > 0)\n> -\t\t\t\treturn offset;\n> +\t\t\t\tgoto done;\n>  \t\t\tbreak;\n>  \t\t}\n>  \t}\n>\n> +done:\n> +\tif (!patch->old_name && !patch->new_name) {\n> +\t\tif (!patch->def_name) {\n> +\t\t\terror(Q_(\"git diff header lacks filename information when removing \"\n> +\t\t\t\t \"%d leading pathname component (line %d)\",\n> +\t\t\t\t \"git diff header lacks filename information when removing \"\n> +\t\t\t\t \"%d leading pathname components (line %d)\",\n> +\t\t\t\t parse_hdr_state.p_value),\n> +\t\t\t      parse_hdr_state.p_value, *linenr);\n> +\t\t\treturn -128;\n> +\t\t}\n> +\t\tpatch->old_name = xstrdup(patch->def_name);\n> +\t\tpatch->new_name = xstrdup(patch->def_name);\n> +\t}\n> +\tif ((!patch->new_name && !patch->is_delete) ||\n> +\t    (!patch->old_name && !patch->is_new)) {\n> +\t\terror(_(\"git diff header lacks filename information \"\n> +\t\t\t\"(line %d)\"), *linenr);\n> +\t\treturn -128;\n> +\t}\n> +\tpatch->is_toplevel_relative = 1;\n>  \treturn offset;\n>  }\n>\n> @@ -1546,26 +1567,6 @@ static int find_header(struct apply_state *state,\n>  \t\t\t\treturn -128;\n>  \t\t\tif (git_hdr_len <= len)\n>  \t\t\t\tcontinue;\n> -\t\t\tif (!patch->old_name && !patch->new_name) {\n> -\t\t\t\tif (!patch->def_name) {\n> -\t\t\t\t\terror(Q_(\"git diff header lacks filename information when removing \"\n> -\t\t\t\t\t\t\t\"%d leading pathname component (line %d)\",\n> -\t\t\t\t\t\t\t\"git diff header lacks filename information when removing \"\n> -\t\t\t\t\t\t\t\"%d leading pathname components (line %d)\",\n> -\t\t\t\t\t\t\tstate->p_value),\n> -\t\t\t\t\t\t     state->p_value, state->linenr);\n> -\t\t\t\t\treturn -128;\n> -\t\t\t\t}\n> -\t\t\t\tpatch->old_name = xstrdup(patch->def_name);\n> -\t\t\t\tpatch->new_name = xstrdup(patch->def_name);\n> -\t\t\t}\n> -\t\t\tif ((!patch->new_name && !patch->is_delete) ||\n> -\t\t\t    (!patch->old_name && !patch->is_new)) {\n> -\t\t\t\terror(_(\"git diff header lacks filename information \"\n> -\t\t\t\t\t     \"(line %d)\"), state->linenr);\n> -\t\t\t\treturn -128;\n> -\t\t\t}\n> -\t\t\tpatch->is_toplevel_relative = 1;\n>  \t\t\t*hdrsize = git_hdr_len;\n>  \t\t\treturn offset;\n>  \t\t}\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index ec548654ce..5b87fead2e 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -226,6 +226,46 @@ test_expect_success 'renamed file' '\n>  \ttest_cmp expected actual\n>  '\n>\n> +test_expect_success 'file with mode only change' '\n> +\tgit range-diff --no-color --submodule=log topic...mode-only-change >actual &&\n> +\tsed s/Z/\\ /g >expected <<-EOF &&\n> +\t1:  fccce22 ! 1:  4d39cb3 s/4/A/\n> +\t    @@ Metadata\n> +\t    ZAuthor: Thomas Rast <trast@inf.ethz.ch>\n> +\t    Z\n> +\t    Z ## Commit message ##\n> +\t    -    s/4/A/\n> +\t    +    s/4/A/ + add other-file\n> +\t    Z\n> +\t    Z ## file ##\n> +\t    Z@@\n> +\t    @@ file\n> +\t    Z A\n> +\t    Z 6\n> +\t    Z 7\n> +\t    +\n> +\t    + ## other-file (new) ##\n> +\t2:  147e64e ! 2:  26c107f s/11/B/\n> +\t    @@ Metadata\n> +\t    ZAuthor: Thomas Rast <trast@inf.ethz.ch>\n> +\t    Z\n> +\t    Z ## Commit message ##\n> +\t    -    s/11/B/\n> +\t    +    s/11/B/ + mode change other-file\n> +\t    Z\n> +\t    Z ## file ##\n> +\t    Z@@ file: A\n> +\t    @@ file: A\n> +\t    Z 12\n> +\t    Z 13\n> +\t    Z 14\n> +\t    +\n> +\t    + ## other-file (mode change 100644 => 100755) ##\n> +\t3:  a63e992 = 3:  4c1e0f5 s/12/B/\n> +\tEOF\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_expect_success 'file added and later removed' '\n>  \tgit range-diff --no-color --submodule=log topic...added-removed >actual &&\n>  \tsed s/Z/\\ /g >expected <<-EOF &&\n> diff --git a/t/t3206/history.export b/t/t3206/history.export\n> index 7bb3814962..4c808e5b3b 100644\n> --- a/t/t3206/history.export\n> +++ b/t/t3206/history.export\n> @@ -55,7 +55,7 @@ A\n>  19\n>  20\n>\n> -commit refs/heads/topic\n> +commit refs/heads/mode-only-change\n>  mark :4\n>  author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200\n>  committer Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200\n> @@ -678,3 +678,32 @@ s/12/B/\n>  from :55\n>  M 100644 :9 renamed-file\n>\n> +commit refs/heads/mode-only-change\n> +mark :57\n> +author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200\n> +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473767 +0100\n> +data 24\n> +s/4/A/ + add other-file\n> +from :4\n> +M 100644 :5 file\n> +M 100644 :49 other-file\n> +\n> +commit refs/heads/mode-only-change\n> +mark :58\n> +author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200\n> +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100\n> +data 33\n> +s/11/B/ + mode change other-file\n> +from :57\n> +M 100644 :7 file\n> +M 100755 :49 other-file\n> +\n> +commit refs/heads/mode-only-change\n> +mark :59\n> +author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200\n> +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100\n> +data 8\n> +s/12/B/\n> +from :58\n> +M 100644 :9 file\n> +\n> --\n> 2.23.0.501.gb744c3af07\n>\n>\n"},{"id":"383734","messageId":"20191009074200.crpjyajbaecaeza6@pengutronix.de","threadId":"51985","inReplyTo":"20191008173843.GC74671@cat","subject":"Re: [PATCH v2] range-diff: don't segfault with mode-only changes","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2019-10-09T07:42:00Z","receivedAt":"2019-10-09T07:42:04Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"On Tue, Oct 08, 2019 at 06:38:43PM +0100, Thomas Gummerer wrote:\n> In ef283b3699 (\"apply: make parse_git_diff_header public\", 2019-07-11)\n> the 'parse_git_diff_header' function was made public and useable by\n> callers outside of apply.c.\n> \n> However it was missed that its (then) only caller, 'find_header' did\n> some error handling, and completing 'struct patch' appropriately.\n> \n> range-diff then started using this function, and tried to handle this\n> appropriately itself, but fell short in some cases.  This in turn\n> would lead to range-diff segfaulting when there are mode-only changes\n> in a range.\n> \n> Move the error handling and completing of the struct into the\n> 'parse_git_diff_header' function, so other callers can take advantage\n> of it.  This fixes the segfault in 'git range-diff'.\n> \n> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n\nThis patch also makes git work again for the originally problematic\nusecase.\n\nTested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>\n\nThanks for your quick reaction to my bug report,\nUwe Kleine-König\n\n-- \nPengutronix e.K.                           | Uwe Kleine-König            |\nIndustrial Linux Solutions                 | http://www.pengutronix.de/  |\n"}]}