{"thread":{"id":"51959","subject":"[BUG] incorrect line numbers reported in git am","startedAt":"2019-10-02T18:45:51Z","lastAt":"2019-10-05T22:51:24Z","messageCount":12,"participants":["Denton Liu","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"383293","messageId":"20191002184546.GA22174@generichostname","threadId":"51959","inReplyTo":null,"subject":"[BUG] incorrect line numbers reported in git am","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-02T18:45:46Z","receivedAt":"2019-10-02T18:45:51Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hello all,\n\nI found a bug where the line numbers in git am are being reported\nincorrectly in the case where a patch fails to apply cleanly.\n\nThe test case for this is pretty simple:\n\n\t$ wget https://public-inbox.org/git/20191001185524.18772-1-newren@gmail.com/raw\n\t$ git am raw\n\nAnd the output for this is:\n\n\tApplying: dir: special case check for the possibility that pathspec is NULL\n\terror: corrupt patch at line 87\n\tPatch failed at 0001 dir: special case check for the possibility that pathspec is NULL\n\thint: Use 'git am --show-current-patch' to see the failed patch\n\tWhen you have resolved this problem, run \"git am --continue\".\n\tIf you prefer to skip this patch, run \"git am --skip\" instead.\n\tTo restore the original branch and stop patching, run \"git am --abort\".\n\nIn this case, the path is indeed corrupt. The final hunk header gives 25\nlines after instead of 24 lines. As a result, it is erroring out\ncorrectly.\n\nHowever, the line offsets are off. Line 87, as it reports, is the\nfollowing:\n\n\tto avoid a segfault.\n\nwhich is in the middle of the log message. I expect the line to be\nreported as something in the range of 198-203, where the end of the hunk\nactually is.\n\nIndeed, if you take an 87 line offset from the cutoff \"---\", we can see\nthat it gives us line 201, which appears at the end of the corrupt hunk.\n\nSo it appears that the bug is a result of the the apply process not\ntaking into account the number of lines from the mail parsing step.\n\nThanks,\n\nDenton\n"},{"id":"383295","messageId":"xmqqd0ffdj9k.fsf@gitster-ct.c.googlers.com","threadId":"51959","inReplyTo":"20191002184546.GA22174@generichostname","subject":"Re: [BUG] incorrect line numbers reported in git am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T19:44:55Z","receivedAt":"2019-10-02T19:45:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> \tApplying: dir: special case check for the possibility that pathspec is NULL\n> \terror: corrupt patch at line 87\n\nThis refers to line 87 of the input file, not a line that begins\nwith \"@@ -87,count...\", doesn't it? If the sender hand edits a\npatch without correcting the number of lines recorded in the hunk\nheader, the parser may not see the next hunk that begins with \"@@\"\nor run out of the input before it reads the required number of lines\ngiven the last hunk header.\n\nWe might be able to notice when the input file is shorter than the\nlast hunk wants it to be, in which case we should be able to say\n'premature end of input at line 87' or something like that.\n\n\n"},{"id":"383297","messageId":"xmqq8sq2ewzh.fsf@gitster-ct.c.googlers.com","threadId":"51959","inReplyTo":"20191002184546.GA22174@generichostname","subject":"Re: [BUG] incorrect line numbers reported in git am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T20:03:14Z","receivedAt":"2019-10-02T20:03:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> which is in the middle of the log message. I expect the line to be\n> reported as something in the range of 198-203,...\n\nThat comes from not knowing who is complaining and what it is\nreading. In this case, \"git apply\" issues a warning because it is\nfed .git/rebase-apply/patch file, which is the output of mailinfo\nthat parses header & log message out, leaves the message in a\nseparate 'msg' file in the same directory and stores the rest in\nthat 'patch' file. And it is line 87 that has problems.\n"},{"id":"383300","messageId":"20191002200836.GA24697@generichostname","threadId":"51959","inReplyTo":"xmqqd0ffdj9k.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] incorrect line numbers reported in git am","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-02T20:08:36Z","receivedAt":"2019-10-02T20:08:43Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Thu, Oct 03, 2019 at 04:44:55AM +0900, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > \tApplying: dir: special case check for the possibility that pathspec is NULL\n> > \terror: corrupt patch at line 87\n> \n> This refers to line 87 of the input file, not a line that begins\n> with \"@@ -87,count...\", doesn't it?\n\nCorrect, it refers to line 87 of the input file. Since the whole mail is\n202 lines long and the faulty hunk comes at the end of the whole mail,\nI'd expect the faulty line number to say something like line 198 or\nsomething that's near the end of the mail. Line 87 is somewhere in the\nmiddle of the log message in the mail.\n\nI think the problem comes from line number being expressed as an offset\nfrom the \"---\" (begin diff) line as opposed to an offset from the actual\nbeginning of the mail.\n\n> If the sender hand edits a\n> patch without correcting the number of lines recorded in the hunk\n> header, the parser may not see the next hunk that begins with \"@@\"\n> or run out of the input before it reads the required number of lines\n> given the last hunk header.\n\nCorrect, but I think that's orthogonal to the main issue. It makes sense\nwhy the error is being reported but what doesn't make sense is the fact\nthat the line numbers reported are so far off from what a user would\nexpect.\n\n> \n> We might be able to notice when the input file is shorter than the\n> last hunk wants it to be, in which case we should be able to say\n> 'premature end of input at line 87' or something like that.\n\nYep, I noticed this bug while I was writing a patch to do exactly that.\n\n> \n> \n"},{"id":"383302","messageId":"20191002201659.GB24697@generichostname","threadId":"51959","inReplyTo":"xmqq8sq2ewzh.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] incorrect line numbers reported in git am","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-02T20:16:59Z","receivedAt":"2019-10-02T20:17:04Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Thu, Oct 03, 2019 at 05:03:14AM +0900, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > which is in the middle of the log message. I expect the line to be\n> > reported as something in the range of 198-203,...\n> \n> That comes from not knowing who is complaining and what it is\n> reading. In this case, \"git apply\" issues a warning because it is\n> fed .git/rebase-apply/patch file, which is the output of mailinfo\n> that parses header & log message out, leaves the message in a\n> separate 'msg' file in the same directory and stores the rest in\n> that 'patch' file. And it is line 87 that has problems.\n\nIn this case, I would still regard this as a bug since users would\nexpect the line 87 to refer to their input file. I think most users\ndon't even realise that a .git/rebase-apply/patch file exists. (I\ncertainly didn't.)\n\nIn fact, running `git am --show-current-patch` shows the whole mail, not\nonly the 'patch' file so users would have no reason to expect the line\nnumbers to refer to the 'patch' file.\n\nI think it would make sense to pass the number of lines skipped by\nmailinfo to the apply step so that more accurate line numbers can be\nreported to users.\n"},{"id":"383318","messageId":"xmqqmueid508.fsf@gitster-ct.c.googlers.com","threadId":"51959","inReplyTo":"20191002201659.GB24697@generichostname","subject":"Re: [BUG] incorrect line numbers reported in git am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-03T00:52:55Z","receivedAt":"2019-10-03T00:53:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> On Thu, Oct 03, 2019 at 05:03:14AM +0900, Junio C Hamano wrote:\n>> Denton Liu writes:\n>> \n>> > which is in the middle of the log message. I expect the line to be\n>> > reported as something in the range of 198-203,...\n>> \n>> That comes from not knowing who is complaining and what it is\n>> reading. In this case, \"git apply\" issues a warning because it is\n>> fed .git/rebase-apply/patch file, which is the output of mailinfo\n>> that parses header & log message out, leaves the message in a\n>> separate 'msg' file in the same directory and stores the rest in\n>> that 'patch' file. And it is line 87 that has problems.\n>\n> In this case, I would still regard this as a bug since users would\n> expect the line 87 to refer to their input file. I think most users\n> don't even realise that a .git/rebase-apply/patch file exists. (I\n> certainly didn't.)\n\nIn any case, if the error message required me to look anywhere\noutside the patch file, it would make it impossible for me to work.\n\n100% of the time, I just pipe the entire message from MUA to \"git\nam\", and I wouldn't know which line it is complaining if it counted\nthe long run of mail headers like Received:, etc., because I do not\nhave such an entire message anywhere in a single file (only my MUA\nhas it, so I'd need to pipe it to \"cat >tempfile\" again after seeing\na failure).\n\n> In fact, running `git am --show-current-patch` shows the whole mail, not\n> only the 'patch' file so users would have no reason to expect the line\n> numbers to refer to the 'patch' file.\n\nYeah, show-current-patch was a misguided attempt to hide useful\ninformation from the users.\n"},{"id":"383326","messageId":"CACsJy8CZCdF-jPeYaAxzpnSbtxbRX42ScSJyYFCfmxZ0YBhZGg@mail.gmail.com","threadId":"51959","inReplyTo":"xmqqmueid508.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] incorrect line numbers reported in git am","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-10-03T06:17:08Z","receivedAt":"2019-10-03T06:17:37Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 3, 2019 at 7:52 AM Junio C Hamano wrote:\n> > In fact, running `git am --show-current-patch` shows the whole mail, not\n> > only the 'patch' file so users would have no reason to expect the line\n> > numbers to refer to the 'patch' file.\n>\n> Yeah, show-current-patch was a misguided attempt to hide useful\n> information from the users.\n\nNot so much hiding as not having the information to present, at least\nnot the easy way, since the mail is split at the beginning of git-am\nand never stored in $GIT_DIR. By the time this command is run, the\nmail is already gone. Someone could of course update git-am to keep a\ncopy of the mail and improve this option.\n-- \nDuy\n"},{"id":"383376","messageId":"xmqqpnjda16c.fsf@gitster-ct.c.googlers.com","threadId":"51959","inReplyTo":"CACsJy8CZCdF-jPeYaAxzpnSbtxbRX42ScSJyYFCfmxZ0YBhZGg@mail.gmail.com","subject":"Re: [BUG] incorrect line numbers reported in git am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-03T22:56:11Z","receivedAt":"2019-10-03T22:56:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Thu, Oct 3, 2019 at 7:52 AM Junio C Hamano wrote:\n>> > In fact, running `git am --show-current-patch` shows the whole mail, not\n>> > only the 'patch' file so users would have no reason to expect the line\n>> > numbers to refer to the 'patch' file.\n>>\n>> Yeah, show-current-patch was a misguided attempt to hide useful\n>> information from the users.\n>\n> Not so much hiding as not having the information to present, at least\n> not the easy way, since the mail is split at the beginning of git-am\n> and never stored in $GIT_DIR. By the time this command is run, the\n> mail is already gone. Someone could of course update git-am to keep a\n> copy of the mail and improve this option.\n\nBy \"hiding\", I meant \"rob from the users an opportunity to learn\nwhere the useful patch file is stored\". \n\nYou seem to be doubly confused in this case, in that (1) you seem to\nhave mistaken that I was complaining about show-current-patch not\ngiving the full information contained in the original e-mail, and\n(2) you seem to think show-current-patch gives the contents of the\npatch witout other e-mail cruft. Both are incorrect.\n\nThe first thing the command does is to feed the input to mailsplit\nand store the results in numbered files \"%04d\", and they are not\nremoved until truly done. When you need to inspect the patch that\ndoes not apply, they are still there. Even emails for those steps\nthat have been successfully applied before the current one are also\nthere (the split files are all gone, though, but they no longer\nmatter as they have been applied fine).\n\nI wouldn't have been so critical if \"git am --show-current-patch\"\nwere implemented as \"cat $GIT_DIR/rebase-apply/patch\", but it\ndoes an equivalent of \"cd $GIT_DIR/rebase-apply; cat $(cat next)\"\nwhich is much less useful when trying to fix up the patch text that\ndoes not apply.\n"},{"id":"383447","messageId":"ec38908d05f0d40190173158ef3f0753fa9f1184.1570226253.git.liu.denton@gmail.com","threadId":"51959","inReplyTo":"20191002184546.GA22174@generichostname","subject":"[PATCH] apply: tell user location of corrupted patch file","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-04T21:59:03Z","receivedAt":"2019-10-04T21:59:09Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When `git am` runs into a corrupt patch, it'll error out and give a\nmessage such as,\n\n\terror: corrupt patch at line 87\n\nCasual users of am may assume that this line number refers to the \nfile that they provided on the command-line. This assumption, however,\nis incorrect. The line count really refers to the\n.git/rebase-apply/patch file genrated by am.\n\nTeach am to print the location of corrupted patch files so that users of\nthe tool will know where to look when fixing their corrupted patch. Thus\nthe error message will look like this:\n\n\terror: corrupt patch at .git/rebase-apply/patch:87\n\nAn alternate design was considered which involved printing the line\nnumbers relative to the output of `git am --show-current-patch` (in\nother words, the actual mail file that's provided to am). This design\nwas not chosen because am does not store the whole mail and instead,\nsplits the mail into several files. As a result of this, this would\nbreak existing users' workflow if they piped their mail directly to am\nfrom their mail client, the whole mail would not exist in any file and\nthey would have to manually recreate the mail to see the line number.\n\nLet's avoid breaking Junio's workflow since he's probably one of the\nmost frequent user of `git am` in the world. ;)\n\nSigned-off-by: Denton Liu \n---\n apply.c | 2 +-\n t/t4012-diff-binary.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 57a61f2881..b0ba2e7b1a 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1785,7 +1785,7 @@ static int parse_single_patch(struct apply_state *state,\n \t\tlen = parse_fragment(state, line, size, patch, fragment);\n \t\tif (len <= 0) {\n \t\t\tfree(fragment);\n-\t\t\treturn error(_(\"corrupt patch at line %d\"), state->linenr);\n+\t\t\treturn error(_(\"corrupt patch at %s:%d\"), state->patch_input_file, state->linenr);\n \t\t}\n \t\tfragment->patch = line;\n \t\tfragment->size = len;\ndiff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\nindex 6579c81216..42cb2dd404 100755\n--- a/t/t4012-diff-binary.sh\n+++ b/t/t4012-diff-binary.sh\n@@ -68,7 +68,7 @@ test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt patch correctly' '\n \tsed -e \"s/-CIT/xCIT/\" broken &&\n \ttest_must_fail git apply --stat --summary broken 2>detected &&\n \tdetected=$(cat detected) &&\n-\tdetected=$(expr \"$detected\" : \"error.*at line \\\\([0-9]*\\\\)\\$\") &&\n+\tdetected=$(expr \"$detected\" : \"error.*at broken:\\\\([0-9]*\\\\)\\$\") &&\n \tdetected=$(sed -ne \"${detected}p\" broken) &&\n \ttest \"$detected\" = xCIT\n '\n@@ -77,7 +77,7 @@ test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt patch correctly' '\n \tgit diff --binary | sed -e \"s/-CIT/xCIT/\" >broken &&\n \ttest_must_fail git apply --stat --summary broken 2>detected &&\n \tdetected=$(cat detected) &&\n-\tdetected=$(expr \"$detected\" : \"error.*at line \\\\([0-9]*\\\\)\\$\") &&\n+\tdetected=$(expr \"$detected\" : \"error.*at broken:\\\\([0-9]*\\\\)\\$\") &&\n \tdetected=$(sed -ne \"${detected}p\" broken) &&\n \ttest \"$detected\" = xCIT\n '\n-- \n2.23.0.687.g391267506c\n\n"},{"id":"383459","messageId":"xmqqv9t37fsw.fsf@gitster-ct.c.googlers.com","threadId":"51959","inReplyTo":"ec38908d05f0d40190173158ef3f0753fa9f1184.1570226253.git.liu.denton@gmail.com","subject":"Re: [PATCH] apply: tell user location of corrupted patch file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-05T08:33:03Z","receivedAt":"2019-10-05T08:33:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> When `git am` runs into a corrupt patch, it'll error out and give a\n> message such as,\n>\n> \terror: corrupt patch at line 87\n>\n> Casual users of am may assume that this line number refers to the \n> file that they provided on the command-line. This assumption, however,\n> is incorrect. The line count really refers to the\n> .git/rebase-apply/patch file genrated by am.\n>\n> Teach am to print the location of corrupted patch files so that users of\n\ns/corrupted/corrupt/;\n\n> the tool will know where to look when fixing their corrupted patch. Thus\n\nLikewise.\n\n> the error message will look like this:\n>\n> \terror: corrupt patch at .git/rebase-apply/patch:87\n>\n> An alternate design was considered which involved printing the line\n> numbers relative to the output of `git am --show-current-patch` (in\n> other words, the actual mail file that's provided to am). This design\n> was not chosen because am does not store the whole mail and instead,\n> splits the mail into several files. As a result of this, this would\n> break existing users' workflow if they piped their mail directly to am\n> from their mail client, the whole mail would not exist in any file and\n> they would have to manually recreate the mail to see the line number.\n\nMore importantly, a change to apply.c (hence \"git apply\", not \"git\nam\") will mean the tool can only talk about its input. If you\nrun, instead of \"git am mbox\", \"git apply mbox\" (assuming you are\nlucky and the piece of mail does not use any fancy features like\nMIME or RFC 1342) may work just as well and it would report the\ncorrupt patch relative to the entire mail text.\n\n>\n> Let's avoid breaking Junio's workflow since he's probably one of the\n> most frequent user of `git am` in the world. ;)\n\nDon't name me.\n\n> \t\tif (len <= 0) {\n> \t\t\tfree(fragment);\n> -\t\t\treturn error(_(\"corrupt patch at line %d\"), state->linenr);\n> +\t\t\treturn error(_(\"corrupt patch at %s:%d\"), state->patch_input_file, state->linenr);\n> \t\t}\n\nDo not forget that you can run \"git apply\" and feed the patch from\nits standard input, e.g.\n\n\t$ git apply patch_input_file is a reasonable string before\nconsidering this.\n\nAlso, if you have a mbox file\n\n\t$ cd sub/direc/tory\n\t$ git am -s /var/tmp/mbox\n\nThe \"git apply\" process thatis run inside \"git am\" would be running\nat the top level of the working tree, so state->patch_input_file may\nsay \".git/rebase-apply/patch\" (i.e. relative pathname) that is not\nrelative to where the end user is in. I personally do not thinkg it\nmatters too much, but some people may complain.\n\nOther than that, looks good. I am kind-of surprised that there is\nonly one place that we report an unusable input with a line number.\nNicely found.\n\n> diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\n> index 6579c81216..42cb2dd404 100755\n> --- a/t/t4012-diff-binary.sh\n> +++ b/t/t4012-diff-binary.sh\n> @@ -68,7 +68,7 @@ test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt patch correctly' '\n> \tsed -e \"s/-CIT/xCIT/\" broken &&\n> \ttest_must_fail git apply --stat --summary broken 2>detected &&\n> \tdetected=$(cat detected) &&\n> -\tdetected=$(expr \"$detected\" : \"error.*at line \\\\([0-9]*\\\\)\\$\") &&\n> +\tdetected=$(expr \"$detected\" : \"error.*at broken:\\\\([0-9]*\\\\)\\$\") &&\n> \tdetected=$(sed -ne \"${detected}p\" broken) &&\n> \ttest \"$detected\" = xCIT\n> '\n> @@ -77,7 +77,7 @@ test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt patch correctly' '\n> \tgit diff --binary | sed -e \"s/-CIT/xCIT/\" >broken &&\n> \ttest_must_fail git apply --stat --summary broken 2>detected &&\n> \tdetected=$(cat detected) &&\n> -\tdetected=$(expr \"$detected\" : \"error.*at line \\\\([0-9]*\\\\)\\$\") &&\n> +\tdetected=$(expr \"$detected\" : \"error.*at broken:\\\\([0-9]*\\\\)\\$\") &&\n> \tdetected=$(sed -ne \"${detected}p\" broken) &&\n> \ttest \"$detected\" = xCIT\n> '\n\nThese existing tests can serve a good test for this new feature, but\nI think you'd also need a case where \"apply\" is fed the patch from\nthe standard input, and possibly another case where it is run from a\nsubdirectory of a working tree.\n\nThanks.\n\n"},{"id":"383499","messageId":"xmqqbluu7qxn.fsf@gitster-ct.c.googlers.com","threadId":"51959","inReplyTo":"xmqqv9t37fsw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] apply: tell user location of corrupted patch file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-05T22:44:52Z","receivedAt":"2019-10-05T22:45:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> An alternate design was considered which involved printing the line\n>> numbers relative to the output of `git am --show-current-patch` (in\n>> other words, the actual mail file that's provided to am). This design\n>> was not chosen because am does not store the whole mail and instead,\n>> splits the mail into several files. As a result of this, this would\n>> break existing users' workflow if they piped their mail directly to am\n>> from their mail client, the whole mail would not exist in any file and\n>> they would have to manually recreate the mail to see the line number.\n>\n> More importantly,...\n\nAddendum.\n\nI think the primary reason why the \"alternate design\" will not fly\nis *NOT* that it breaks existing users (which it would), but giving\na line number in the original mbox file is not always possible.\n\nImagine the message you received was munged by the sending mailer,\nor a relaying mailer, and what you received is encoded in base64 ;-)\n\n\n"},{"id":"383500","messageId":"xmqq4l0m7qmv.fsf@gitster-ct.c.googlers.com","threadId":"51959","inReplyTo":"xmqqv9t37fsw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] apply: tell user location of corrupted patch file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-05T22:51:20Z","receivedAt":"2019-10-05T22:51:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> \t\tif (len <= 0) {\n>> \t\t\tfree(fragment);\n>> -\t\t\treturn error(_(\"corrupt patch at line %d\"), state->linenr);\n>> +\t\t\treturn error(_(\"corrupt patch at %s:%d\"), state->patch_input_file, state->linenr);\n>> \t\t}\n>\n> Do not forget that you can run \"git apply\" and feed the patch from\n> its standard input, e.g.\n>\n> \t$ git apply \t$ git show -R | git apply\n>\n> Make sure state->patch_input_file is a reasonable string before\n> considering this.\n\nI think what the patch does is safe in this case; callsites of\napply_patch(), which sets the .patch_input_file field, pass the\nstring \"\", so you'd say\n\n\terror: corrupt patch at :43\n\nWe lost the word \"line\" in the message, but it would be picked up\nrather quickly by users that colon + integer is a line number, so\nI think it is OK.\n\n> Also, if you have a mbox file\n>\n> \t$ cd sub/direc/tory\n> \t$ git am -s /var/tmp/mbox\n>\n> The \"git apply\" process thatis run inside \"git am\" would be running\n> at the top level of the working tree, so state->patch_input_file may\n> say \".git/rebase-apply/patch\" (i.e. relative pathname) that is not\n> relative to where the end user is in. I personally do not thinkg it\n> matters too much, but some people may complain.\n>\n> Other than that, looks good. I am kind-of surprised that there is\n> only one place that we report an unusable input with a line number.\n> Nicely found.\n\nI still do not know if we have a relative-path problem, how severe\nit would be if there is, or if it is fixable if we wanted to and\nhow, though.\n\n\nThanks.\n\n"}]}