bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Alan Mackenzie]

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).
 

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Glenn Morris]

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?

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Stefan Monnier]

>     @@ -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

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Alan Mackenzie]

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).

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Stefan Monnier]

>> 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

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Alan Mackenzie]

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).

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Stefan Monnier]

> 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

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Alan Mackenzie]

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).

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Stefan Monnier]

> 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

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Alan Mackenzie]

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).

bug#17522: diff-mode frustrates attempt to correct corrupted diff file.

[Stefan Monnier]

>> 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