Re: [BUG] incorrect line numbers reported in git am
Duy Nguyen writes: > On Thu, Oct 3, 2019 at 7:52 AM Junio C Hamano wrote: >> > In fact, running `git am --show-current-patch` shows the whole mail, not >> > only the 'patch' file so users would have no reason to expect the line >> > numbers to refer to the 'patch' file. >> >> Yeah, show-current-patch was a misguided attempt to hide useful >> information from the users. > > Not so much hiding as not having the information to present, at least > not the easy way, since the mail is split at the beginning of git-am > and never stored in $GIT_DIR. By the time this command is run, the > mail is already gone. Someone could of course update git-am to keep a > copy of the mail and improve this option. By "hiding", I meant "rob from the users an opportunity to learn where the useful patch file is stored". You seem to be doubly confused in this case, in that (1) you seem to have mistaken that I was complaining about show-current-patch not giving the full information contained in the original e-mail, and (2) you seem to think show-current-patch gives the contents of the patch witout other e-mail cruft. Both are incorrect. The first thing the command does is to feed the input to mailsplit and store the results in numbered files "%04d", and they are not removed until truly done. When you need to inspect the patch that does not apply, they are still there. Even emails for those steps that have been successfully applied before the current one are also there (the split files are all gone, though, but they no longer matter as they have been applied fine). I wouldn't have been so critical if "git am --show-current-patch" were implemented as "cat $GIT_DIR/rebase-apply/patch", but it does an equivalent of "cd $GIT_DIR/rebase-apply; cat $(cat next)" which is much less useful when trying to fix up the patch text that does not apply.
Re: [BUG] incorrect line numbers reported in git am
On Thu, Oct 3, 2019 at 7:52 AM Junio C Hamano wrote: > > In fact, running `git am --show-current-patch` shows the whole mail, not > > only the 'patch' file so users would have no reason to expect the line > > numbers to refer to the 'patch' file. > > Yeah, show-current-patch was a misguided attempt to hide useful > information from the users. Not so much hiding as not having the information to present, at least not the easy way, since the mail is split at the beginning of git-am and never stored in $GIT_DIR. By the time this command is run, the mail is already gone. Someone could of course update git-am to keep a copy of the mail and improve this option. -- Duy
Re: [BUG] incorrect line numbers reported in git am
Denton Liu writes: > On Thu, Oct 03, 2019 at 05:03:14AM +0900, Junio C Hamano wrote: >> Denton Liu writes: >> >> > which is in the middle of the log message. I expect the line to be >> > reported as something in the range of 198-203,... >> >> That comes from not knowing who is complaining and what it is >> reading. In this case, "git apply" issues a warning because it is >> fed .git/rebase-apply/patch file, which is the output of mailinfo >> that parses header & log message out, leaves the message in a >> separate 'msg' file in the same directory and stores the rest in >> that 'patch' file. And it is line 87 that has problems. > > In this case, I would still regard this as a bug since users would > expect the line 87 to refer to their input file. I think most users > don't even realise that a .git/rebase-apply/patch file exists. (I > certainly didn't.) In any case, if the error message required me to look anywhere outside the patch file, it would make it impossible for me to work. 100% of the time, I just pipe the entire message from MUA to "git am", and I wouldn't know which line it is complaining if it counted the long run of mail headers like Received:, etc., because I do not have such an entire message anywhere in a single file (only my MUA has it, so I'd need to pipe it to "cat >tempfile" again after seeing a failure). > In fact, running `git am --show-current-patch` shows the whole mail, not > only the 'patch' file so users would have no reason to expect the line > numbers to refer to the 'patch' file. Yeah, show-current-patch was a misguided attempt to hide useful information from the users.
Re: [BUG] incorrect line numbers reported in git am
On Thu, Oct 03, 2019 at 05:03:14AM +0900, Junio C Hamano wrote: > Denton Liu writes: > > > which is in the middle of the log message. I expect the line to be > > reported as something in the range of 198-203,... > > That comes from not knowing who is complaining and what it is > reading. In this case, "git apply" issues a warning because it is > fed .git/rebase-apply/patch file, which is the output of mailinfo > that parses header & log message out, leaves the message in a > separate 'msg' file in the same directory and stores the rest in > that 'patch' file. And it is line 87 that has problems. In this case, I would still regard this as a bug since users would expect the line 87 to refer to their input file. I think most users don't even realise that a .git/rebase-apply/patch file exists. (I certainly didn't.) In fact, running `git am --show-current-patch` shows the whole mail, not only the 'patch' file so users would have no reason to expect the line numbers to refer to the 'patch' file. I think it would make sense to pass the number of lines skipped by mailinfo to the apply step so that more accurate line numbers can be reported to users.
Re: [BUG] incorrect line numbers reported in git am
On Thu, Oct 03, 2019 at 04:44:55AM +0900, Junio C Hamano wrote: > Denton Liu writes: > > > Applying: dir: special case check for the possibility that pathspec is > > NULL > > error: corrupt patch at line 87 > > This refers to line 87 of the input file, not a line that begins > with "@@ -87,count...", doesn't it? Correct, it refers to line 87 of the input file. Since the whole mail is 202 lines long and the faulty hunk comes at the end of the whole mail, I'd expect the faulty line number to say something like line 198 or something that's near the end of the mail. Line 87 is somewhere in the middle of the log message in the mail. I think the problem comes from line number being expressed as an offset from the "---" (begin diff) line as opposed to an offset from the actual beginning of the mail. > If the sender hand edits a > patch without correcting the number of lines recorded in the hunk > header, the parser may not see the next hunk that begins with "@@" > or run out of the input before it reads the required number of lines > given the last hunk header. Correct, but I think that's orthogonal to the main issue. It makes sense why the error is being reported but what doesn't make sense is the fact that the line numbers reported are so far off from what a user would expect. > > We might be able to notice when the input file is shorter than the > last hunk wants it to be, in which case we should be able to say > 'premature end of input at line 87' or something like that. Yep, I noticed this bug while I was writing a patch to do exactly that. > >
Re: [BUG] incorrect line numbers reported in git am
Denton Liu writes: > which is in the middle of the log message. I expect the line to be > reported as something in the range of 198-203,... That comes from not knowing who is complaining and what it is reading. In this case, "git apply" issues a warning because it is fed .git/rebase-apply/patch file, which is the output of mailinfo that parses header & log message out, leaves the message in a separate 'msg' file in the same directory and stores the rest in that 'patch' file. And it is line 87 that has problems.
Re: [BUG] incorrect line numbers reported in git am
Denton Liu writes: > Applying: dir: special case check for the possibility that pathspec is > NULL > error: corrupt patch at line 87 This refers to line 87 of the input file, not a line that begins with "@@ -87,count...", doesn't it? If the sender hand edits a patch without correcting the number of lines recorded in the hunk header, the parser may not see the next hunk that begins with "@@" or run out of the input before it reads the required number of lines given the last hunk header. We might be able to notice when the input file is shorter than the last hunk wants it to be, in which case we should be able to say 'premature end of input at line 87' or something like that.
[BUG] incorrect line numbers reported in git am
Hello all, I found a bug where the line numbers in git am are being reported incorrectly in the case where a patch fails to apply cleanly. The test case for this is pretty simple: $ wget https://public-inbox.org/git/20191001185524.18772-1-new...@gmail.com/raw $ git am raw And the output for this is: Applying: dir: special case check for the possibility that pathspec is NULL error: corrupt patch at line 87 Patch failed at 0001 dir: special case check for the possibility that pathspec is NULL hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". In this case, the path is indeed corrupt. The final hunk header gives 25 lines after instead of 24 lines. As a result, it is erroring out correctly. However, the line offsets are off. Line 87, as it reports, is the following: to avoid a segfault. which is in the middle of the log message. I expect the line to be reported as something in the range of 198-203, where the end of the hunk actually is. Indeed, if you take an 87 line offset from the cutoff "---", we can see that it gives us line 201, which appears at the end of the corrupt hunk. So it appears that the bug is a result of the the apply process not taking into account the number of lines from the mail parsing step. Thanks, Denton
