* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. @ 2014-05-18 10:50 Alan Mackenzie 2014-05-19 15:39 ` Glenn Morris 2014-05-20 13:54 ` Stefan Monnier 0 siblings, 2 replies; 11+ messages in thread From: Alan Mackenzie @ 2014-05-18 10:50 UTC (permalink / raw) To: 17522 Hi, Emacs. On GNU Emacs 24.4.50.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.8.7) of 2014-04-19 on acm.acm , I have a diff which has been corrupted during transmission by email. I have loaded this diff into a Diff Mode buffer. At the head of one of the hunks appears: @@ -3155,115 +3160,38 @ , and three times in the hunk a long line has got split somehow, like this: - (message "Eval error in the `c-lang-defvar' or `c-lang-setvar' for `%s'%s: %S" . When I attempt to correct the patch by removing the spurious line feed using M-^, the header line is (silently) corrupted by replacing the two "number of lines" fields with zeros, thusly: @@ -3155,0 +3160,0 @ . This is surely a bug. The workaround seems to be to use Fundamental Mode. -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-18 10:50 bug#17522: diff-mode frustrates attempt to correct corrupted diff file Alan Mackenzie @ 2014-05-19 15:39 ` Glenn Morris 2014-05-20 13:54 ` Stefan Monnier 1 sibling, 0 replies; 11+ messages in thread From: Glenn Morris @ 2014-05-19 15:39 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 17522 It's presumably a useful thing to do when the diff is not corrupt, so diff-mode would have to recognise when a diff is corrupt and turn off this behaviour. A non-corrupt diff seems the more likely case? ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-18 10:50 bug#17522: diff-mode frustrates attempt to correct corrupted diff file Alan Mackenzie 2014-05-19 15:39 ` Glenn Morris @ 2014-05-20 13:54 ` Stefan Monnier 2014-05-21 21:56 ` Alan Mackenzie 1 sibling, 1 reply; 11+ messages in thread From: Stefan Monnier @ 2014-05-20 13:54 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 17522 > @@ -3155,0 +3160,0 @ > . This is surely a bug. The workaround seems to be to use Fundamental > Mode. Maybe not a bug, but a misfeature: the ",0" is probably because the first line after the @...@ is empty, which normally "can't" be part of a hunk, so this empty line is taken as an "end of hunk". If you add a space on that line, the count should be updated again and start looking more sane. Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-20 13:54 ` Stefan Monnier @ 2014-05-21 21:56 ` Alan Mackenzie 2014-05-22 1:32 ` Stefan Monnier 0 siblings, 1 reply; 11+ messages in thread From: Alan Mackenzie @ 2014-05-21 21:56 UTC (permalink / raw) To: Stefan Monnier; +Cc: 17522 Hi, Stefan. On Tue, May 20, 2014 at 09:54:02AM -0400, Stefan Monnier wrote: > > @@ -3155,0 +3160,0 @ > > . This is surely a bug. The workaround seems to be to use Fundamental > > Mode. > Maybe not a bug, but a misfeature: the ",0" is probably because the first > line after the @...@ is empty, which normally "can't" be part of a hunk, > so this empty line is taken as an "end of hunk". OK. But patch appears to accept a blank line (in a unified diff) without complaint. > If you add a space on that line, the count should be updated again and > start looking more sane. This is all besides the point. I did not edit the hunk header, therefore I don't expect it to be changed behind my back. If I need the header to be recalculated, surely there should be a command for that. (There is, but it doesn't have a key binding.) At the very least, surely Diff Mode should _ask_ me before splatting my file. Or at the very, very least even _inform_ me that it has done so, rather than leaving it up to patch to issue its bewildering error message. Why do people hand edit patches anyway? Clearly, because patches sometimes get corrupted, e.g. by email software, as happened to me. For this case, it doesn't make sense to recalculate the header. But for other reasons? Why would anybody edit a patch hunk other than to repair it? It's not something I can imagine myself wanting to do. > Stefan -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-21 21:56 ` Alan Mackenzie @ 2014-05-22 1:32 ` Stefan Monnier 2014-05-22 21:14 ` Alan Mackenzie 2014-05-25 16:07 ` Alan Mackenzie 0 siblings, 2 replies; 11+ messages in thread From: Stefan Monnier @ 2014-05-22 1:32 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 17522 >> Maybe not a bug, but a misfeature: the ",0" is probably because the first >> line after the @...@ is empty, which normally "can't" be part of a hunk, >> so this empty line is taken as an "end of hunk". > OK. But patch appears to accept a blank line (in a unified diff) > without complaint. Indeed, "patch" at some point was changed so as to accept empty lines. diff-mode.el was partly changed to accommodate that looser format, but not 100% (and in the case of updating line counts while editing the patch, it's a bit more delicate since, contrary to "patch" we can't rely on the line counts to decide whether an empty line marks the end of a hunk or not). >> If you add a space on that line, the count should be updated again and >> start looking more sane. > This is all besides the point. I did not edit the hunk header, > therefore I don't expect it to be changed behind my back. If I need > the header to be recalculated, surely there should be a command > for that. diff-mode tries to be fancier and do that transparently. You're just bumping into a bug of that code (regardless of how the empty-line is handled, a line count of 0 for both sides of the hunk is clearly not right). > Why do people hand edit patches anyway? All kinds of reasons. The case of corrupted patches was definitely not the main motivation for the line-count-update feature (after all, for corrupted patches, you don't want to mess with the line counts). BTW, I remember writing some kind of "fix corrupted hunk" code. Oh, yes, it's in diff-sanity-check-hunk. Can you try to see if it can auto-fix your corrupted patch? M-x diff-goto-source RET is probably the easiest way to trigger it (sadly, it's not provided as a separate command). > Clearly, because patches sometimes get corrupted, e.g. by email > software, as happened to me. For this case, it doesn't make sense to > recalculate the header. But for other reasons? It can be easier to take a hunk and edit it to remove some of the changes it performs, then it is to change the source code to then re-generate a new hunk. Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-22 1:32 ` Stefan Monnier @ 2014-05-22 21:14 ` Alan Mackenzie 2014-05-23 2:07 ` Stefan Monnier 2014-05-25 16:07 ` Alan Mackenzie 1 sibling, 1 reply; 11+ messages in thread From: Alan Mackenzie @ 2014-05-22 21:14 UTC (permalink / raw) To: Stefan Monnier; +Cc: 17522 Hi, Stefan. On Wed, May 21, 2014 at 09:32:11PM -0400, Stefan Monnier wrote: > >> If you add a space on that line, the count should be updated again and > >> start looking more sane. > > This is all besides the point. I did not edit the hunk header, > > therefore I don't expect it to be changed behind my back. If I need > > the header to be recalculated, surely there should be a command > > for that. > diff-mode tries to be fancier and do that transparently. Yes, but it sometimes fails, and then this "transparently" becomes a euphemism for "fouls it up in a manner causing the user the maximum difficulty in seeing what has happened". At this point you snipped the main point of my last post, which was this: > > At the very least, surely Diff Mode should _ask_ me before splatting > > my file. Or at the very, very least even _inform_ me that it has > > done so, rather than leaving it up to patch to issue its bewildering > > error message. So why can't we have a `y-or-n-p' or a `message' when diff-mode is about to change a hunk header, possibly fouling it up? It is the "fanciness" and "transparency" which caused me so much grief, the not realising that the hunk header had been automatically changed. It never occurred to me that Diff Mode might do something like this (except for a dim memory that something like this happened to me once before). > You're just bumping into a bug of that code (regardless of how the > empty-line is handled, a line count of 0 for both sides of the hunk is > clearly not right). Yes, but a line count of 1 for both sides, which might have happened if the blank line was the second line in the hunk rather than the first, would be equally not right. > BTW, I remember writing some kind of "fix corrupted hunk" code. > Oh, yes, it's in diff-sanity-check-hunk. Can you try to see if it can > auto-fix your corrupted patch? It's getting late. I'll have a look at this some time soon. > Stefan -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-22 21:14 ` Alan Mackenzie @ 2014-05-23 2:07 ` Stefan Monnier 2014-05-23 20:43 ` Alan Mackenzie 0 siblings, 1 reply; 11+ messages in thread From: Stefan Monnier @ 2014-05-23 2:07 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 17522 > So why can't we have a `y-or-n-p' or a `message' when diff-mode is about > to change a hunk header, possibly fouling it up? Because we don't agree on UI issues. Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-23 2:07 ` Stefan Monnier @ 2014-05-23 20:43 ` Alan Mackenzie 2014-05-24 3:19 ` Stefan Monnier 0 siblings, 1 reply; 11+ messages in thread From: Alan Mackenzie @ 2014-05-23 20:43 UTC (permalink / raw) To: Stefan Monnier; +Cc: 17522 Hi, Stefan. On Thu, May 22, 2014 at 10:07:17PM -0400, Stefan Monnier wrote: > > So why can't we have a `y-or-n-p' or a `message' when diff-mode is about > > to change a hunk header, possibly fouling it up? > Because we don't agree on UI issues. Pardon? What has our alleged disagreement about some UI issues (which ones, by the way?) got to do with solving this bug? To restore some context, what happened was that whilst editing a patch file (which had become corrupted by email software) in Diff Mode, Diff Mode, without seeking permission from or even informing the user, corrupted a hunk header by writing 0 into the "number of lines" fields, thus leaving the patch file unusable. Do you think it is possible to amend the hunk header editing routine so that it can't make such mistakes? I'm sceptical. Just to emphasise once more, it was the surreptitious nature of Diff Mode's corruption that caused the pain. If it had been open about its changes, it would have been so much easier to reverse them. Do you have a better proposal for a fix to this bug than my proposal? > Stefan -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-23 20:43 ` Alan Mackenzie @ 2014-05-24 3:19 ` Stefan Monnier 0 siblings, 0 replies; 11+ messages in thread From: Stefan Monnier @ 2014-05-24 3:19 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 17522 > Do you have a better proposal for a fix to this bug than my proposal? Of course, I suggested to make the line-count-update more robust. Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-22 1:32 ` Stefan Monnier 2014-05-22 21:14 ` Alan Mackenzie @ 2014-05-25 16:07 ` Alan Mackenzie 2014-05-25 20:18 ` Stefan Monnier 1 sibling, 1 reply; 11+ messages in thread From: Alan Mackenzie @ 2014-05-25 16:07 UTC (permalink / raw) To: Stefan Monnier; +Cc: 17522 Hello, Stefan. On Wed, May 21, 2014 at 09:32:11PM -0400, Stefan Monnier wrote: > >> Maybe not a bug, but a misfeature: the ",0" is probably because the first > >> line after the @...@ is empty, which normally "can't" be part of a hunk, > >> so this empty line is taken as an "end of hunk". > > OK. But patch appears to accept a blank line (in a unified diff) > > without complaint. > Indeed, "patch" at some point was changed so as to accept empty lines. > diff-mode.el was partly changed to accommodate that looser format, but > not 100% (and in the case of updating line counts while editing the > patch, it's a bit more delicate since, contrary to "patch" we can't > rely on the line counts to decide whether an empty line marks the end of > a hunk or not). Why not? If you're going to carry on making surreptitious changes to hunk headers, you'll need to determine the start and end of hunks reliably, especially corrupted hunks. This can't be done with 100% certainty, and is going to involve heuristics. A good way might be to scan a hunk _forwards_ counting lines before the first change to that hunk. We know that, in broken unified patches, empty lines are liable to have their single space removed, and long lines are liable to be split. > >> If you add a space on that line, the count should be updated again and > >> start looking more sane. > > This is all besides the point. I did not edit the hunk header, > > therefore I don't expect it to be changed behind my back. If I need > > the header to be recalculated, surely there should be a command > > for that. > diff-mode tries to be fancier and do that transparently. You're just > bumping into a bug of that code (regardless of how the empty-line is > handled, a line count of 0 for both sides of the hunk is clearly not > right). [ .... ] > BTW, I remember writing some kind of "fix corrupted hunk" code. > Oh, yes, it's in diff-sanity-check-hunk. Can you try to see if it can > auto-fix your corrupted patch? The code for diff-sanity-check-hunk is unfinished. It asks "Try to auto-fix word wrap damage?", but does nothing when you say yes. It also doesn't check the line counts, at least, not for unified diffs. > M-x diff-goto-source RET is probably the easiest way to trigger it > (sadly, it's not provided as a separate command). > Stefan -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#17522: diff-mode frustrates attempt to correct corrupted diff file. 2014-05-25 16:07 ` Alan Mackenzie @ 2014-05-25 20:18 ` Stefan Monnier 0 siblings, 0 replies; 11+ messages in thread From: Stefan Monnier @ 2014-05-25 20:18 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 17522 >> diff-mode.el was partly changed to accommodate that looser format, but >> not 100%. > Why not? The usual: lack of time/motivation/... > If you're going to carry on making surreptitious changes to > hunk headers, you'll need to determine the start and end of hunks > reliably, especially corrupted hunks. This can't be done with 100% > certainty, and is going to involve heuristics. A good way might be to > scan a hunk _forwards_ counting lines before the first change to that > hunk. Exactly, with a before-change-function. And if the hunk is found to be borked, then try to at least not make things worse (and maybe suggest the use of diff-sanity-check-hunk, after we make it into a command). Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-05-25 20:18 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-18 10:50 bug#17522: diff-mode frustrates attempt to correct corrupted diff file Alan Mackenzie 2014-05-19 15:39 ` Glenn Morris 2014-05-20 13:54 ` Stefan Monnier 2014-05-21 21:56 ` Alan Mackenzie 2014-05-22 1:32 ` Stefan Monnier 2014-05-22 21:14 ` Alan Mackenzie 2014-05-23 2:07 ` Stefan Monnier 2014-05-23 20:43 ` Alan Mackenzie 2014-05-24 3:19 ` Stefan Monnier 2014-05-25 16:07 ` Alan Mackenzie 2014-05-25 20:18 ` Stefan Monnier
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).