diff-mode misinterprets empty lines.

diff-mode misinterprets empty lines.

[Sergei Organov]

Hello,

In GNU Emacs 22.1.1 (i686-pc-linux-gnu, GTK+ Version 2.8.20).

GNU diff since version 2.8.7 outputs empty lines in the unified diff
format for empty context lines that don't change (instead of single
space followed by newline). See, e.g., the thread starting here:

<http://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html>

Emacs diff-mode interprets empty lines as patch hunk separators, in
particular leading to wrong calculation of the length put into hunk
header. Another symptom is "C-c C-n" (diff-restrict-view) wrongly
restricts hunks with empty line(s).

Here is a patch hunk to test on:

@@ -1,7 +1,7 @@
 l1

 l3
-	  abc
+	  cba
 l5
 l6
 l7


-- 
Sergei Organov.

diff-mode misinterprets empty lines.

[Richard Stallman]

This is important to fix.  Would someone please fix it in Emacs 22,
then ack?


X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY 
	autolearn=failed version=3.1.0
To: bug-gnu-emacs@gnu.org
From: Sergei Organov <osv@javad.com>
Date: Tue, 27 Nov 2007 21:04:17 +0300
Message-ID: <8763zno99a.fsf@osv.gnss.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: diff-mode misinterprets empty lines.

Hello,

In GNU Emacs 22.1.1 (i686-pc-linux-gnu, GTK+ Version 2.8.20).

GNU diff since version 2.8.7 outputs empty lines in the unified diff
format for empty context lines that don't change (instead of single
space followed by newline). See, e.g., the thread starting here:

<http://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html>

Emacs diff-mode interprets empty lines as patch hunk separators, in
particular leading to wrong calculation of the length put into hunk
header. Another symptom is "C-c C-n" (diff-restrict-view) wrongly
restricts hunks with empty line(s).

Here is a patch hunk to test on:

@@ -1,7 +1,7 @@
 l1

 l3
-	  abc
+	  cba
 l5
 l6
 l7


-- 
Sergei Organov.

Re: diff-mode misinterprets empty lines.

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

> This is important to fix.  Would someone please fix it in Emacs 22,
> then ack?

[...]

> GNU diff since version 2.8.7 outputs empty lines in the unified diff
> format for empty context lines that don't change (instead of single
> space followed by newline). See, e.g., the thread starting here:
>
> <http://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html>

I have to agree with those thinking it is a terrible idea to prune
trailing spaces that are part of the _diff_ format.

When people don't want trailing whitespace warnings to trigger in their
editor, they should obviously amend diff-mode for that.  But changing
the diff format instead is insane.  And that patch happens to have
tolerance code intended to deal with utterly broken mailers is no excuse
for that.

There are lots of other tools working with diffs and applying patches.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: diff-mode misinterprets empty lines.

[Stefan Monnier]

>> GNU diff since version 2.8.7 outputs empty lines in the unified diff
>> format for empty context lines that don't change (instead of single
>> space followed by newline). See, e.g., the thread starting here:
>> 
>> <http://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html>

> I have to agree with those thinking it is a terrible idea to prune
> trailing spaces that are part of the _diff_ format.

Agreed as well, this is a terrible idea.  Making tools somewhat robust
in the face of lost trailing whitespace is good (diff-mode does that),
but actively removing them is bad.

What's the benefit, really?
The original post said "This doesn't really matter..." and I agree with
this part.  So breaking every/most program that parses diffs just
because "when the only trailing blanks in a diff are those in context headers,
it's a little annoying" seems gratuitous.

Could you please revert this change?


        Stefan "Author of Emacs's diff-mode"

Re: diff-mode misinterprets empty lines.

[Paul Eggert]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Could you please revert this change?

I did so, by installing the following patch into diffutils.  The old
behavior (with trailing blanks) is now the default, and the new
behavior (without them) is enabled only if the new
--suppress-blank-empty option is given.

However, the next POSIX version specifies a "diff -u" format in which
those trailing blanks are optional, and apparently some "diff"
implementations in the wild omit them even though no official or test
version of GNU diff ever did.  So, as a practical matter, programs
that parse "diff" output should not assume the trailing blanks are
present, regardless of what GNU diff does.

2007-12-04  Paul Eggert  <eggert@cs.ucla.edu>

	* NEWS: New diff option --suppress-blank-empty (no longer -u default).
	* doc/diff.texi (Detailed Unified): Trailing spaces are no longer
	omitted.
	(Trailing Blanks): New section.
	(diff Options) Mention new option.
	* src/diff.h (suppress_blank_entry): New decl.
	* src/context.c (pr_unidiff_hunk): Support --suppress-blank-empty.
	* src/util.c (print_1_line): Likewise.
	* src/diff.c (longopts, main, option_help_msgid): Likewise.
	(SUPPRESS_BLANK_EMPTY_OPTION): New constant.

Index: NEWS
===================================================================
RCS file: /cvsroot/diffutils/diffutils/NEWS,v
retrieving revision 1.27
diff -u -p -r1.27 NEWS
--- NEWS	19 Jul 2007 17:45:26 -0000	1.27
+++ NEWS	5 Dec 2007 07:24:10 -0000
@@ -1,7 +1,6 @@
 User-visible changes since 2.8.7 (in "version" 2.8.7-cvs):

-* When reporting an unchanged empty line, diff -u now outputs an
-  empty line instead of a line consisting of a single space character.
+* New diff option --suppress-blank-empty.

 * Bring back support for `diff -NUM', where NUM is a number,
   even when conforming to POSIX 1003.1-2001.  This change reverts to
Index: doc/diff.texi
===================================================================
RCS file: /cvsroot/diffutils/diffutils/doc/diff.texi,v
retrieving revision 1.33
diff -u -p -r1.33 diff.texi
--- doc/diff.texi	15 Aug 2007 19:33:36 -0000	1.33
+++ doc/diff.texi	5 Dec 2007 07:24:11 -0000
@@ -842,8 +842,7 @@ line numbers look like @samp{@var{start}
 its end line number appears.  An empty hunk is considered to end at
 the line that precedes the hunk.

-Lines common to both files begin with a space character, except that
-the space is omitted if its line is empty.  Lines
+The lines common to both files begin with a space character.  The lines
 that actually differ between the two files have one of the following
 indicator characters in the left print column:

@@ -1854,8 +1853,9 @@ specified patterns.
 These adjustments can be applied to any output format.

 @menu
-* Tabs::       Preserving the alignment of tab stops.
-* Pagination:: Page numbering and time-stamping @command{diff} output.
+* Tabs::            Preserving the alignment of tab stops.
+* Trailing Blanks:: Suppressing blanks before empty output lines.
+* Pagination::      Page numbering and time-stamping @command{diff} output.
 @end menu

 @node Tabs
@@ -1890,6 +1890,29 @@ output format, which does not have a spa
 type indicator character.  Select this method with the @option{-T} or
 @option{--initial-tab} option.

+@node Trailing Blanks
+@section Omitting trailing blanks
+@cindex trailing blanks
+When outputting lines in normal or context format, or outputting an
+unchanged line in unified format, @command{diff} normally outputs a
+blank just before each line.  If the line is empty, the output of
+@command{diff} therefore contains trailing blanks even though the
+input does not contain them.  For example, when outputting an
+unchanged empty line in context format, @command{diff} normally
+outputs a line with two leading spaces.
+
+Some text editors and email agents routinely delete trailing blanks,
+so it can be a problem to deal with diff output files that contain
+them.  You can avoid this problem with the
+@option{--suppress-blank-empty} option.  It causes @command{diff} to
+omit trailing blanks at the end of output lines in normal, context,
+and unified format, unless the trailing blanks were already present in
+the input.  This changes the output format slightly, so that output
+lines are guaranteed to never end in a blank unless an input line ends
+in a blank.  This format is less likely to be munged by text editors
+or by transmission via email.  It is accepted by @acronym{GNU}
+@command{patch} as well.
+
 @node Pagination
 @section Paginating @command{diff} Output
 @cindex paginating @command{diff} output
@@ -3878,6 +3901,11 @@ normal.  @xref{Tabs}.
 Assume that tab stops are set every @var{columns} (default 8) print
 columns.  @xref{Tabs}.

+@item --suppress-blank-empty
+Suppress any blanks before newlines when printing the representation
+of an empty line, when outputting normal, context, or unified format.
+@xref{Trailing Blanks}.
+
 @item --to-file=@var{file}
 Compare each operand to @var{file}; @var{file} may be a directory.

Index: src/diff.h
===================================================================
RCS file: /cvsroot/diffutils/diffutils/src/diff.h,v
retrieving revision 1.28
diff -u -p -r1.28 diff.h
--- src/diff.h	19 Jul 2007 17:45:29 -0000	1.28
+++ src/diff.h	5 Dec 2007 07:24:11 -0000
@@ -150,6 +150,9 @@ XTERN size_t tabsize;
    without changing the characters in it (-T).  */
 XTERN bool initial_tab;

+/* Do not output an initial space or tab before the text of an empty line.  */
+XTERN bool suppress_blank_empty;
+
 /* Remove trailing carriage returns from input.  */
 XTERN bool strip_trailing_cr;

Index: src/context.c
===================================================================
RCS file: /cvsroot/diffutils/diffutils/src/context.c,v
retrieving revision 1.23
diff -u -p -r1.23 context.c
--- src/context.c	19 Jul 2007 17:45:28 -0000	1.23
+++ src/context.c	5 Dec 2007 07:24:11 -0000
@@ -350,7 +350,7 @@ pr_unidiff_hunk (struct change *hunk)
       if (!next || i < next->line0)
 	{
 	  char const *const *line = &files[0].linbuf[i++];
-	  if (**line != '\n')
+	  if (! (suppress_blank_empty && **line == '\n'))
 	    putc (initial_tab ? '\t' : ' ', out);
 	  print_1_line (NULL, line);
 	  j++;
@@ -362,10 +362,11 @@ pr_unidiff_hunk (struct change *hunk)
 	  k = next->deleted;
 	  while (k--)
 	    {
+	      char const * const *line = &files[0].linbuf[i++];
 	      putc ('-', out);
-	      if (initial_tab)
+	      if (initial_tab && ! (suppress_blank_empty && **line == '\n'))
 		putc ('\t', out);
-	      print_1_line (NULL, &files[0].linbuf[i++]);
+	      print_1_line (NULL, line);
 	    }

 	  /* Then output the inserted part. */
@@ -373,10 +374,11 @@ pr_unidiff_hunk (struct change *hunk)
 	  k = next->inserted;
 	  while (k--)
 	    {
+	      char const * const *line = &files[1].linbuf[j++];
 	      putc ('+', out);
-	      if (initial_tab)
+	      if (initial_tab && ! (suppress_blank_empty && **line == '\n'))
 		putc ('\t', out);
-	      print_1_line (NULL, &files[1].linbuf[j++]);
+	      print_1_line (NULL, line);
 	    }

 	  /* We're done with this hunk, so on to the next! */
Index: src/util.c
===================================================================
RCS file: /cvsroot/diffutils/diffutils/src/util.c,v
retrieving revision 1.37
diff -u -p -r1.37 util.c
--- src/util.c	15 Aug 2007 19:32:22 -0000	1.37
+++ src/util.c	5 Dec 2007 07:24:11 -0000
@@ -506,7 +506,8 @@ print_script (struct change *script,
 \f
 /* Print the text of a single line LINE,
    flagging it with the characters in LINE_FLAG (which say whether
-   the line is inserted, deleted, changed, etc.).  */
+   the line is inserted, deleted, changed, etc.).  LINE_FLAG must not
+   end in a blank, unless it is a single blank.  */

 void
 print_1_line (char const *line_flag, char const *const *line)
@@ -517,12 +518,25 @@ print_1_line (char const *line_flag, cha

   /* If -T was specified, use a Tab between the line-flag and the text.
      Otherwise use a Space (as Unix diff does).
-     Print neither space nor tab if line-flags are empty.  */
+     Print neither space nor tab if line-flags are empty.
+     But omit trailing blanks if requested.  */

   if (line_flag && *line_flag)
     {
-      flag_format = initial_tab ? "%s\t" : "%s ";
-      fprintf (out, flag_format, line_flag);
+      char const *flag_format_1 = flag_format = initial_tab ? "%s\t" : "%s ";
+      char const *line_flag_1 = line_flag;
+
+      if (suppress_blank_empty && **line == '\n')
+	{
+	  flag_format_1 = "%s";
+
+	  /* This hack to omit trailing blanks takes advantage of the
+	     fact that the only way that LINE_FLAG can end in a blank
+	     is when LINE_FLAG consists of a single blank.  */
+	  line_flag_1 += *line_flag_1 == ' ';
+	}
+
+      fprintf (out, flag_format_1, line_flag_1);
     }

   output_1_line (base, limit, flag_format, line_flag);
Index: src/diff.c
===================================================================
RCS file: /cvsroot/diffutils/diffutils/src/diff.c,v
retrieving revision 1.46
diff -u -p -r1.46 diff.c
--- src/diff.c	15 Aug 2007 19:32:22 -0000	1.46
+++ src/diff.c	5 Dec 2007 07:24:11 -0000
@@ -110,6 +110,7 @@ enum
   NORMAL_OPTION,
   SDIFF_MERGE_ASSIST_OPTION,
   STRIP_TRAILING_CR_OPTION,
+  SUPPRESS_BLANK_EMPTY_OPTION,
   SUPPRESS_COMMON_LINES_OPTION,
   TABSIZE_OPTION,
   TO_FILE_OPTION,
@@ -187,6 +188,7 @@ static struct option const longopts[] =
   {"speed-large-files", 0, 0, 'H'},
   {"starting-file", 1, 0, 'S'},
   {"strip-trailing-cr", 0, 0, STRIP_TRAILING_CR_OPTION},
+  {"suppress-blank-empty", 0, 0, SUPPRESS_BLANK_EMPTY_OPTION},
   {"suppress-common-lines", 0, 0, SUPPRESS_COMMON_LINES_OPTION},
   {"tabsize", 1, 0, TABSIZE_OPTION},
   {"text", 0, 0, 'a'},
@@ -565,6 +567,10 @@ main (int argc, char **argv)
 	  strip_trailing_cr = true;
 	  break;

+	case SUPPRESS_BLANK_EMPTY_OPTION:
+	  suppress_blank_empty = true;
+	  break;
+
 	case SUPPRESS_COMMON_LINES_OPTION:
 	  suppress_common_lines = true;
 	  break;
@@ -886,6 +892,7 @@ static char const * const option_help_ms
   N_("-t  --expand-tabs  Expand tabs to spaces in output."),
   N_("-T  --initial-tab  Make tabs line up by prepending a tab."),
   N_("--tabsize=NUM  Tab stops are every NUM (default 8) print columns."),
+  N_("--suppress-blank-empty  Suppress space or tab before empty output lines."),
   "",
   N_("-r  --recursive  Recursively compare any subdirectories found."),
   N_("-N  --new-file  Treat absent files as empty."),

Re: diff-mode misinterprets empty lines.

[Jim Meyering]

Paul Eggert <eggert@CS.UCLA.EDU> wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> Could you please revert this change?
>
> I did so, by installing the following patch into diffutils.  The old
> behavior (with trailing blanks) is now the default, and the new
> behavior (without them) is enabled only if the new
> --suppress-blank-empty option is given.
>
> However, the next POSIX version specifies a "diff -u" format in which
> those trailing blanks are optional, and apparently some "diff"
> implementations in the wild omit them even though no official or test
> version of GNU diff ever did.  So, as a practical matter, programs
> that parse "diff" output should not assume the trailing blanks are
> present, regardless of what GNU diff does.
>
> 2007-12-04  Paul Eggert  <eggert@cs.ucla.edu>
>
> 	* NEWS: New diff option --suppress-blank-empty (no longer -u default).
> 	* doc/diff.texi (Detailed Unified): Trailing spaces are no longer
> 	omitted.
> 	(Trailing Blanks): New section.
> 	(diff Options) Mention new option.
> 	* src/diff.h (suppress_blank_entry): New decl.
> 	* src/context.c (pr_unidiff_hunk): Support --suppress-blank-empty.
> 	* src/util.c (print_1_line): Likewise.
> 	* src/diff.c (longopts, main, option_help_msgid): Likewise.
> 	(SUPPRESS_BLANK_EMPTY_OPTION): New constant.

Hi Paul,

That looks fine.
Since I was thinking of using that new option always, I considered
changing the source to make the default in my private binary be to enable
the new option.  Since I'd rather not have to make private changes, what
do you think about having a compile-time option to change the default?
Not even a configure-time flag.

Since Emacs may eventually change to accept the new format, it may make
sense to change the default and to provide a --no-suppress-blank-empty
option.

Re: diff-mode misinterprets empty lines.

[David Kastrup]

Jim Meyering <jim@meyering.net> writes:

> Since I was thinking of using that new option always, I considered
> changing the source to make the default in my private binary be to
> enable the new option.  Since I'd rather not have to make private
> changes, what do you think about having a compile-time option to
> change the default?  Not even a configure-time flag.
>
> Since Emacs may eventually change to accept the new format, it may
> make sense to change the default and to provide a
> --no-suppress-blank-empty option.

No, it does not make sense to change the default.  Emacs is just one of
many tools processing diff output.  I remember gratuitous breakage of
git, too.  There are far too many tools relying on the _documented_ diff
output formats (please read
(info "(diff) Context")
for a detailed explanation of the diff formats) that it makes sense
breaking things all around for no tangible benefit.

I don't understand why this change was made in the first place, and I
don't understand why people would want to gratuitously make the format
less reliable to parse by tools, when the main usage _is_ the
application by independent tools.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: diff-mode misinterprets empty lines.

[Jim Meyering]

David Kastrup <dak@gnu.org> wrote:

> Jim Meyering <jim@meyering.net> writes:
>
>> Since I was thinking of using that new option always, I considered
>> changing the source to make the default in my private binary be to
>> enable the new option.  Since I'd rather not have to make private
>> changes, what do you think about having a compile-time option to
>> change the default?  Not even a configure-time flag.
>>
>> Since Emacs may eventually change to accept the new format, it may
>> make sense to change the default and to provide a
>> --no-suppress-blank-empty option.
>
> No, it does not make sense to change the default.  Emacs is just one of
> many tools processing diff output.  I remember gratuitous breakage of
> git, too.  There are far too many tools relying on the _documented_ diff
> output formats (please read
> (info "(diff) Context")

Hi David,

My sole interest is in the change to the *unidiff* format.
And that was not documented, back then.

> for a detailed explanation of the diff formats) that it makes sense
> breaking things all around for no tangible benefit.

The benefit was tangible enough to me to propose the change, and to Paul
to allow and extend it.  I'm sure you know that git tools have been
accepting the trailing-blank-free format for over a year, so they too
saw some benefit in accepting the new format.

Too many tools these days can automatically remove trailing blanks.
If I keep my code free of trailing blanks (and I do), those tools should
have no affect on my code.  I want the same benefit for diffs of my code.
IMHO, it's an improvement at least to allow a trailing-blank-free diff format.

> I don't understand why this change was made in the first place, and I
> don't understand why people would want to gratuitously make the format
> less reliable to parse by tools, when the main usage _is_ the
> application by independent tools.

You seem to underestimate Paul's concern for portability.  Git was young
at the time, and the format they cared about was unidiff, so it wasn't
*that* big a deal to fix it.  If we had known about the incompatibility
with diff-mode back then, I'm sure the new behavior would never have
been made the default.

Re: diff-mode misinterprets empty lines.

[Andreas Schwab]

Jim Meyering <jim@meyering.net> writes:

> My sole interest is in the change to the *unidiff* format.
> And that was not documented, back then.

(diff)Detailed Unified:

   The lines common to both files begin with a space character.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-mode misinterprets empty lines.

[Jim Meyering]

Andreas Schwab <schwab@suse.de> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> My sole interest is in the change to the *unidiff* format.
>> And that was not documented, back then.
>
> (diff)Detailed Unified:
>
>    The lines common to both files begin with a space character.

Oh.  So it did.

I suppose I was remembering that aspect was not specified by POSIX,
but now it is.

Re: diff-mode misinterprets empty lines.

[David Kastrup]

Jim Meyering <jim@meyering.net> writes:

> David Kastrup <dak@gnu.org> wrote:
>
>> No, it does not make sense to change the default.  Emacs is just one of
>> many tools processing diff output.  I remember gratuitous breakage of
>> git, too.  There are far too many tools relying on the _documented_ diff
>> output formats (please read
>> (info "(diff) Context")
>
> My sole interest is in the change to the *unidiff* format.
> And that was not documented, back then.

Huh?  What makes you say that?

>> for a detailed explanation of the diff formats) that it makes sense
>> breaking things all around for no tangible benefit.
>
> The benefit was tangible enough to me to propose the change, and to
> Paul to allow and extend it.  I'm sure you know that git tools have
> been accepting the trailing-blank-free format for over a year, so they
> too saw some benefit in accepting the new format.

Huh?  What tangible benefit is "does not break in newer versions but
only gets less reliable"?

> Too many tools these days can automatically remove trailing blanks.

Why would you apply them to a _diff_?

> If I keep my code free of trailing blanks (and I do), those tools
> should have no affect on my code.

But your code is written as _diffs_.  diffs are _output_ formats, not
input formats.

> I want the same benefit for diffs of my code.

What benefit?

> IMHO, it's an improvement at least to allow a trailing-blank-free diff
> format.

We are not talking about whether it makes sense for Emacs diff to be
able to work around the output of broken file transfers.  We are talking
about whether it makes sense to break the output in the first place.
And a "trailing-blank-free diff format" gained in this manner is an
illusion, anyway, since diff must be able to represent lines differing
in trailing space.

>> I don't understand why this change was made in the first place, and I
>> don't understand why people would want to gratuitously make the
>> format less reliable to parse by tools, when the main usage _is_ the
>> application by independent tools.
>
> You seem to underestimate Paul's concern for portability.

Huh?  How does one gain portability by breaking output formats?

> Git was young at the time, and the format they cared about was
> unidiff, so it wasn't *that* big a deal to fix it.

Huh?  What do you mean by "fix it"?

> If we had known about the incompatibility with diff-mode back then,
> I'm sure the new behavior would never have been made the default.

We are not talking about an "incompatibility with diff-mode".  We are
talking about breaking the format specification.  That concerns _any_
tool reading the output of diff, not just diff-mode.

So again: where is the tangible benefit of breaking diff output in the
first place?  Because broken communication channels might break it, too?
What kind of benefit is it to prebreak it?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: diff-mode misinterprets empty lines.

[Paul Eggert]

David Kastrup <dak@gnu.org> writes:

>> Too many tools these days can automatically remove trailing blanks.
>
> Why would you apply them to a _diff_?

We're beating a dead horse here, since the default behavior has been
changed back, but since you're interested in doing an autopsy....

I regularly apply many tools to a diff.  I use Emacs, and other text
editors.  I send diffs via email, and I store them as part of other
text files.  The most recent email I sent to this forum contained a
diff, for example.

In environments that use diffs a lot, trailing white space can cause
problems.  Here's one example.  Emacs has a way of highlighting
trailing white space.  This feature is a nice thing to have in
general, since it highlights white space that in some applications can
cause harm (C source code, makefiles, etc.) and that the user normally
cannot see.  But if 'diff' output contains unnecessary trailing white
space, the highlighted white space turns into false alarms on your
screen.  In general, a false alarm is a bad thing: either it causes
you to turn off the alarm mechanism, or it causes you to ignore
similar alarms in the future.  So that is at least one good motivation
for suppressing unnecessary trailing white space.

I'm not saying that it is always the right thing to suppress trailing
blanks; I'm only saying that there are good reasons to do so in some
cases.

By the way, the diff that I sent in my previous email
<http://lists.gnu.org/archive/html/bug-gnu-utils/2007-12/msg00026.html>
suppressed the trailing white space in question.  Did you notice?
I've used this format for many months now, for patches that I send via
email.  No problems have been reported with these patches.  And I send
out my fair share of patches.

Re: diff-mode misinterprets empty lines.

[David Kastrup]

Paul Eggert <eggert@CS.UCLA.EDU> writes:

> David Kastrup <dak@gnu.org> writes:
>
>>> Too many tools these days can automatically remove trailing blanks.
>>
>> Why would you apply them to a _diff_?
>
> We're beating a dead horse here, since the default behavior has been
> changed back, but since you're interested in doing an autopsy....
>
> I regularly apply many tools to a diff.  I use Emacs, and other text
> editors.  I send diffs via email, and I store them as part of other
> text files.  The most recent email I sent to this forum contained a
> diff, for example.
>
> In environments that use diffs a lot, trailing white space can cause
> problems.

No.  In environments that use diffs, trailing white space needs to be
preserved.  Otherwise you could, for example, never have a diff that
removes trailing white space.

What _you_ mean is that in environments that _don't_ use diffs but
rather interpret them as naked text, inconveniences might occur.  But
those will occur with any patch dealing with trailing white space in any
manner, anyhow.

> Here's one example.  Emacs has a way of highlighting trailing white
> space.  This feature is a nice thing to have in general, since it
> highlights white space that in some applications can cause harm (C
> source code, makefiles, etc.) and that the user normally cannot see.
> But if 'diff' output contains unnecessary trailing white space, the
> highlighted white space turns into false alarms on your screen.

Which only means that it is is wrong to interpret diff output with an
unmodified trailing whitespace detection mode.  Because diff files are
not merely text.  They carry a meaning.

> So that is at least one good motivation for suppressing unnecessary
> trailing white space.

The trailing white space is not unnecessary since it is part of the
format.

> I'm not saying that it is always the right thing to suppress trailing
> blanks; I'm only saying that there are good reasons to do so in some
> cases.

diff-mode clearly is not such a one.

> By the way, the diff that I sent in my previous email
> <http://lists.gnu.org/archive/html/bug-gnu-utils/2007-12/msg00026.html>
> suppressed the trailing white space in question.  Did you notice?

I happen not to be a program.  If I had used Emacs for actually dealing
with the diff, I would have noticed.

Isn't it quite nonsensical to ask this question?  Would you be bothering
messing with trailing whitespace in the first place if it was
human-visible normally?

> I've used this format for many months now, for patches that I send via
> email.  No problems have been reported with these patches.

You mean, the mailing list thread on the Emacs developer list (which the
request to change this back originated on) is just a figment of my
imagination?  No problem report triggered it?

> And I send out my fair share of patches.

The "patch" program deals with this format violation.  Not everything
else does, and people often repair patches instead of reporting them.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: diff-mode misinterprets empty lines.

[Paul Eggert]

David Kastrup <dak@gnu.org> writes:

> In environments that use diffs, trailing white space needs to be
> preserved.  Otherwise you could, for example, never have a diff that
> removes trailing white space.

Yes, of course, but the trailing white space you're talking about is
unaffected by the change, so this is not the issue here.  We are
talking only about trailing white space generated by the diff format
itself, not about trailing white space in the data.

> it is is wrong to interpret diff output with an
> unmodified trailing whitespace detection mode.

Not if --suppress-blank-empty is used.  With that option, all the
trailing white space will be about changes to data, which is normally
something I want to see.  It's much easier to see changings that
affect only trailing white space that way.  That is an advantage of
suppressing the blanks in question.

If Emacs could be improved to mark only important trailing white
space; then --suppress-blank-empty would be less useful (at least,
less useful to Emacs users).  But I don't see how to do that in
general, when diffs are mixed in with other text.

> The trailing white space is not unnecessary since it is part of the
> format.

Not necessarily.  diff outputs several formats.  The definition of the
format is up to us, and depends on utility; it is not carved in stone.
Unless you count draft POSIX as the "stone"; in that case, the
trailing blanks of diff -u format are indeed unnecessary.

> Would you be bothering
> messing with trailing whitespace in the first place if it was
> human-visible normally?

Yes.  Human-visible trailing white space was one of the motivations
for this change.

Re: diff-mode misinterprets empty lines.

[Andreas Schwab]

Paul Eggert <eggert@CS.UCLA.EDU> writes:

> Not necessarily.  diff outputs several formats.  The definition of the
> format is up to us, and depends on utility; it is not carved in stone.

But was defined and documented several years ago.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-mode misinterprets empty lines.

[Juanma Barranquero]

On Dec 5, 2007 6:45 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> But if 'diff' output contains unnecessary trailing white
> space, the highlighted white space turns into false alarms on your
> screen.  In general, a false alarm is a bad thing: either it causes
> you to turn off the alarm mechanism, or it causes you to ignore
> similar alarms in the future.

Or it makes you add

 (add-hook 'diff-mode-hook (lambda () (setq show-trailing-whitespace nil)))

to your .emacs if it really bothers you. This is not "to turn off the
alarm mechanism", but rather than having it activated for patches is a
mistake, because patches are not text (as David has said).

> I've used this format for many months now, for patches that I send via
> email.  No problems have been reported with these patches.  And I send
> out my fair share of patches.

Again, I agree with David: I've installed quite a few patches, and
I've never reported back problems with them if I could trivially fix
them. Lack of complains is not a positive proof.

             Juanma

Re: diff-mode misinterprets empty lines.

[Stefan Monnier]

> Again, I agree with David: I've installed quite a few patches, and
> I've never reported back problems with them if I could trivially fix
> them. Lack of complains is not a positive proof.

Also Emacs's diff-mdoe already has code to try and fix missing trailing
whitespace (as well as a few other common problems introduced by brain
dead mailers).  This support was good enough when only occasional broken
patches had those problems.


        Stefan

Re: diff-mode misinterprets empty lines.

[Glenn Morris]

In an effort to clear FOR-RELEASE, here is a simple-minded attempt to
deal with this. It seems to fix the original problem at least.


*** diff-mode.el.~1.122.~	2007-10-21 13:00:12.000000000 -0700
--- diff-mode.el	2008-01-05 15:58:41.000000000 -0800
***************
*** 403,409 ****
    (setq style (diff-hunk-style style))
    (let ((end (and (re-search-forward (case style
  				       ;; A `unified' header is ambiguous.
! 				       (unified (concat "^[^-+# \\]\\|"
  							diff-file-header-re))
  				       (context "^[^-+#! \\]")
  				       (normal "^[^<>#\\]")
--- 403,409 ----
    (setq style (diff-hunk-style style))
    (let ((end (and (re-search-forward (case style
  				       ;; A `unified' header is ambiguous.
! 				       (unified (concat "^[^-+# \\\n]\\|"
  							diff-file-header-re))
  				       (context "^[^-+#! \\]")
  				       (normal "^[^<>#\\]")

Re: diff-mode misinterprets empty lines.

[Richard Stallman]

Thank you for working on this.
There are just a few bugs in FOR-RELEASE; with a little work
we can fix them all.

Re: diff-mode misinterprets empty lines.

[Stefan Monnier]

> In an effort to clear FOR-RELEASE, here is a simple-minded attempt to
> deal with this. It seems to fix the original problem at least.

It is problematic when you try to apply a hunk via C-c C-a because any
empty line following your hunk (e.g. the last hunk in a C-x v =) will be
taken as being part of the hunk and C-c C-a will think that the original
text was just missing a newline and will add it (as a result of its
fuzzy matching feature).


        Stefan


> *** diff-mode.el.~1.122.~	2007-10-21 13:00:12.000000000 -0700
> --- diff-mode.el	2008-01-05 15:58:41.000000000 -0800
> ***************
> *** 403,409 ****
>     (setq style (diff-hunk-style style))
>     (let ((end (and (re-search-forward (case style
>   				       ;; A `unified' header is ambiguous.
> ! 				       (unified (concat "^[^-+# \\]\\|"
>   							diff-file-header-re))
>   				       (context "^[^-+#! \\]")
>   				       (normal "^[^<>#\\]")
> --- 403,409 ----
>     (setq style (diff-hunk-style style))
>     (let ((end (and (re-search-forward (case style
>   				       ;; A `unified' header is ambiguous.
> ! 				       (unified (concat "^[^-+# \\\n]\\|"
>   							diff-file-header-re))
>   				       (context "^[^-+#! \\]")
>   				       (normal "^[^<>#\\]")

Re: diff-mode misinterprets empty lines.

[Glenn Morris]

Stefan Monnier wrote:

>> In an effort to clear FOR-RELEASE, here is a simple-minded attempt to
>> deal with this. It seems to fix the original problem at least.
>
> It is problematic when you try to apply a hunk via C-c C-a because any
> empty line following your hunk (e.g. the last hunk in a C-x v =) will be
> taken as being part of the hunk and C-c C-a will think that the original
> text was just missing a newline and will add it (as a result of its
> fuzzy matching feature).

Oh dear. Would it suffice to back up over empty lines at the end of a
unified diff? Ie treat empty-lines as part of a hunk unless they are
at the very end of the diff?

Re: diff-mode misinterprets empty lines.

[Glenn Morris]

Glenn Morris wrote:

> Would it suffice to back up over empty lines at the end of a unified
> diff? Ie treat empty-lines as part of a hunk unless they are at the
> very end of the diff?

Sigh, that won't work in the CVS trunk, where the end of vc-diff
output looks like:

<last line of diff hunk>
<empty line>
<empty line>
Diffs between working revision and workfile end here.

I don't personally like that last line anyway...

Re: diff-mode misinterprets empty lines.

[Dan Nicolaescu]

Glenn Morris <rgm@gnu.org> writes:

  > Glenn Morris wrote:
  > 
  > > Would it suffice to back up over empty lines at the end of a unified
  > > diff? Ie treat empty-lines as part of a hunk unless they are at the
  > > very end of the diff?
  > 
  > Sigh, that won't work in the CVS trunk, where the end of vc-diff
  > output looks like:
  > 
  > <last line of diff hunk>
  > <empty line>
  > <empty line>
  > Diffs between working revision and workfile end here.
  > 
  > I don't personally like that last line anyway...

I would vote to take out that line and the extra empty lines. 

They are annoying when you want to save the diff to a file and archive
it, or mail it to someone else, etc.

Re: diff-mode misinterprets empty lines.

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> In an effort to clear FOR-RELEASE, here is a simple-minded attempt to
>> deal with this. It seems to fix the original problem at least.
>
> It is problematic when you try to apply a hunk via C-c C-a because any
> empty line following your hunk (e.g. the last hunk in a C-x v =) will be
> taken as being part of the hunk and C-c C-a will think that the original
> text was just missing a newline and will add it (as a result of its
> fuzzy matching feature).

Is there any progress on this issue, or is help needed?

It seems to me that this is the last bug remaining the FOR-RELEASE,
although the release process is so opaque and confusing that I have
little idea what's going on.

Re: diff-mode misinterprets empty lines.

[Stefan Monnier]

>>> In an effort to clear FOR-RELEASE, here is a simple-minded attempt to
>>> deal with this. It seems to fix the original problem at least.
>> 
>> It is problematic when you try to apply a hunk via C-c C-a because any
>> empty line following your hunk (e.g. the last hunk in a C-x v =) will be
>> taken as being part of the hunk and C-c C-a will think that the original
>> text was just missing a newline and will add it (as a result of its
>> fuzzy matching feature).

> Is there any progress on this issue, or is help needed?

No progress.  Help would be welcome.  I think it mostly involves trying
to use diff-sanity-checks (either the function itself or the ideas) at
more places.

E.g. diff-end-of-hunk should check that the place it found is consistent
with the hunk header's numbers and if not (and if looking at an empty
line) keep going further.

But be careful also that diff-end-of-hunk (and other functions) is/are
used in diff-fixup-modifs where the hunk header is presumed broken.

> It seems to me that this is the last bug remaining the FOR-RELEASE,
> although the release process is so opaque and confusing that I have
> little idea what's going on.

It was decided that this bug should not prevent the pretest.  I would
personally not hold the release for it either, so we already have
diff-sanity-checks which takes care of the most serious problems already.


        Stefan

Re: diff-mode misinterprets empty lines.

[Stefan Monnier]

> No progress.  Help would be welcome.  I think it mostly involves trying

Actually, as you may have seen, I've found the time to do it today,


        Stefan

Re: diff-mode misinterprets empty lines.

[Richard Stallman]

There is no need to argue.
We do need to change Emacs to cope if that space is missing.
And GNU diff should output the spaces by default as it did in the past.

Re: diff-mode misinterprets empty lines.

[Paul Eggert]

Jim Meyering <jim@meyering.net> writes:

> Since I was thinking of using that new option always, I considered
> changing the source to make the default in my private binary be to enable
> the new option.

Could you instead use a shell script?

#!/bin/sh
exec /path/to/actual/diff/program --suppress-blank-empty "$@"

Re: diff-mode misinterprets empty lines.

[Jim Meyering]

Paul Eggert <eggert@CS.UCLA.EDU> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> Since I was thinking of using that new option always, I considered
>> changing the source to make the default in my private binary be to enable
>> the new option.
>
> Could you instead use a shell script?
>
> #!/bin/sh
> exec /path/to/actual/diff/program --suppress-blank-empty "$@"

Of course :-)
Thanks.

Re: diff-mode misinterprets empty lines.

[Richard Stallman]

    When people don't want trailing whitespace warnings to trigger in their
    editor, they should obviously amend diff-mode for that.  But changing
    the diff format instead is insane.  And that patch happens to have
    tolerance code intended to deal with utterly broken mailers is no excuse
    for that.

Are you suggesting I should ask the diff maintainers to revert that
change?  That might be a good idea.

But in the mean time let's make Emacs work with the format generated
by diff 2.8.7.  Even if diff 2.8.8 undoes this change, 2.8.7 won't
disappear.  Would someone please do that and ack?

Re: diff-mode misinterprets empty lines.

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

>     When people don't want trailing whitespace warnings to trigger in their
>     editor, they should obviously amend diff-mode for that.  But changing
>     the diff format instead is insane.  And that patch happens to have
>     tolerance code intended to deal with utterly broken mailers is no excuse
>     for that.
>
> Are you suggesting I should ask the diff maintainers to revert that
> change?  That might be a good idea.

Personally, I would consider this a good idea.  The "patch" program has
apparently been made robust in the presence of mail-mangled patches,
cut&paste carnage and other damage.  But "patch works with it" is, in my
opinion, not a positive definition of the diff format (while "patch
fails" is a different issue): patch is not the only program working with
diffs, and some version control systems have a diff-based workflow
without using "patch" for it.

Here is the format definition actually delivered with diff itself for
context diffs:

<URL:http://www.gnu.org/software/diffutils/manual/html_node/Detailed-Context.html#Detailed%20Context>

And here another for unified diffs:

<URL:http://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html#Detailed%20Unified>

It appears quite definite from the description that no whitespace
reduction is intended, neither for content nor for formatting
characters.

So yes, I would ask the diff maintainers to revert the change.  I can't
think of a good reason for it at all, but maybe they can, so it would
probably be a good idea to keep emacs-devel in the list (after all,
Emacs is one of the applications broken by this, for now): that way we
might together reach consensus on the best course.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: diff-mode misinterprets empty lines.

[Stefan Monnier]

>     When people don't want trailing whitespace warnings to trigger in their
>     editor, they should obviously amend diff-mode for that.  But changing
>     the diff format instead is insane.  And that patch happens to have
>     tolerance code intended to deal with utterly broken mailers is no excuse
>     for that.

> Are you suggesting I should ask the diff maintainers to revert that
> change?  That might be a good idea.

Yes, please.


        Stefan