show whitespace problems by default in diff-mode

show whitespace problems by default in diff-mode

[Dan Nicolaescu]

diff-mode is set up to show trailing whitespace problems, but it's not
doing it by default, to enable this one has to do:

(add-hook 'diff-mode-hook 'whitespace-mode)

Vinicius also posted a patch to edit files to clean the trailing
whitespace problems based on the diff.

Why not turn on whitespace-mode by default in diff-mode?  It makes
spotting whitespace problems very easy, and will make it also easy to
fix.

RE: show whitespace problems by default in diff-mode

[Drew Adams]

> diff-mode is set up to show trailing whitespace problems, but it's not
> doing it by default, to enable this one has to do:
> 
> (add-hook 'diff-mode-hook 'whitespace-mode)
> 
> Vinicius also posted a patch to edit files to clean the trailing
> whitespace problems based on the diff.
> 
> Why not turn on whitespace-mode by default in diff-mode?  It makes
> spotting whitespace problems very easy, and will make it also easy to
> fix.

Perhaps for the same reason that whitespace diffs are ignored by default by
`diff' - because whitespace differences (including trailing whitespace) are
often _not_ so interesting.

What's the problem with setting `diff' switches and adding `whitespace-mode' to
the hook the way you want? Why should the default value change? When you "make
spotting whitespace problems very easy" you also make ignoring whitespace
problems more difficult.

I don't have a particular axe to grind, here. It's just not clear that most
people will want to show whitespace diffs (or even trailing whitespace) most of
the time. If you can support that claim, then sure, go for it.

Re: show whitespace problems by default in diff-mode

[Dan Nicolaescu]

"Drew Adams" <drew.adams@oracle.com> writes:

  > > diff-mode is set up to show trailing whitespace problems, but it's not
  > > doing it by default, to enable this one has to do:
  > > 
  > > (add-hook 'diff-mode-hook 'whitespace-mode)
  > > 
  > > Vinicius also posted a patch to edit files to clean the trailing
  > > whitespace problems based on the diff.
  > > 
  > > Why not turn on whitespace-mode by default in diff-mode?  It makes
  > > spotting whitespace problems very easy, and will make it also easy to
  > > fix.
  > 
  > Perhaps for the same reason that whitespace diffs are ignored by default by
  > `diff' - because whitespace differences (including trailing whitespace) are
  > often _not_ so interesting.

Trailing whitespace is an annoyance most of the time, and it's not to be
conflated with the rest of the whitespace differences (which diff-mode
can ignore just fine -- C-c C-w).

  > I don't have a particular axe to grind, here. It's just not clear that most
  > people will want to show whitespace diffs (or even trailing whitespace) most of
  > the time. If you can support that claim, then sure, go for it.

I have yet to see anyone that wants to have trailing whitespace (which
are the only thing that might post is concerned with), but it's common
to see patches being rejected, or people asked to fix the patches
because of trailing whitespace problems.  Making it easy to spot and fix
such issues just helps our users.

RE: show whitespace problems by default in diff-mode

[Drew Adams]

> Trailing whitespace is an annoyance most of the time,

That's the question - is it? For most Emacs users most of the time? Dunno; just
asking.

> and it's not to be conflated with the rest of the whitespace
> differences (which diff-mode can ignore just fine -- C-c C-w).

Yes, I know, which is why I didn't conflate them. I mentioned both, in case
there is also some desire to deal with whitespace in general this way.

There could be a `diff' switch (or a pseudo-`diff' switch, handled only by
Emacs) for trailing whitespace. Just as patch submitters need to get the `diff'
switches right for context diff etc. (`-c -w' or whatever), so could they set
the switch to show trailing whitespace.

>   > I don't have a particular axe to grind, here. It's just 
>   > not clear that most people will want to show whitespace
>   > diffs (or even trailing whitespace) most of the time.
>   > If you can support that claim, then sure, go for it.
> 
> I have yet to see anyone that wants to have trailing whitespace (which
> are the only thing that might post is concerned with), but it's common
> to see patches being rejected, or people asked to fix the patches
> because of trailing whitespace problems.  Making it easy to 
> spot and fix such issues just helps our users.

As I see it, the claim is still not supported. The question is about most Emacs
users of `diff' most of the time. You seem to be addressing the population of
most submitters of Emacs patches most of the time that they submit an Emacs
patch (or other patch where trailing whitespace is a no-no). Those aren't the
same thing.

Again, I don't know whether your claim might also be true for most Emacs users
of `diff' most of the time. But you haven't supported that, at least.

(And yes, I'm likely to be bit by this gotcha, myself. The question still should
be about Emacs users of `diff' in general, not about Emacs patch submitters.)

Re: show whitespace problems by default in diff-mode

[Dan Nicolaescu]

"Drew Adams" <drew.adams@oracle.com> writes:

  > > Trailing whitespace is an annoyance most of the time,
  > 
  > That's the question - is it? For most Emacs users most of the time? Dunno; just
  > asking.

Yes, it is, please go to your favorite search engine and search for 
trailing whitespace patch or trailing whitespace diff and see you many
hits you get.

  > > and it's not to be conflated with the rest of the whitespace
  > > differences (which diff-mode can ignore just fine -- C-c C-w).
  > 
  > Yes, I know, which is why I didn't conflate them. I mentioned both, in case
  > there is also some desire to deal with whitespace in general this way.

No, there isn't, I intentionally limited the topic to one thing because
of not wanting to have a long discussion that ends nowhere.  So please
stick to the topic at hand.

  > There could be a `diff' switch (or a pseudo-`diff' switch, handled only by
  > Emacs) for trailing whitespace. Just as patch submitters need to get the `diff'
  > switches right for context diff etc. (`-c -w' or whatever), so could they set
  > the switch to show trailing whitespace.

There isn't such a flag, and more, users might not use diff, but
vc-diff.

Re: show whitespace problems by default in diff-mode

[Miles Bader]

Dan Nicolaescu <dann@ics.uci.edu> writes:
> Why not turn on whitespace-mode by default in diff-mode?  It makes
> spotting whitespace problems very easy, and will make it also easy to
> fix.

I guess, it's the same as with normal files:  some people are very
concerned with whitespace problems, but many do not care; for the
latter, whitespace indication would simply be annoying noise.

It's good if it's _easy_ to turn on, but it seems far less obvious that
it should be turned on by default.

[I know many in the git community seem quite obsessed with catching
whitespace problems in patches etc, but I've found git's various
whitespace-detecting frobs etc simply annoying (I don't know if it's
still turned on by default, but I had to actively disable it).]

-Miles

-- 
Fast, small, soon; pick any 2.

Re: show whitespace problems by default in diff-mode

[Dan Nicolaescu]

Miles Bader <miles@gnu.org> writes:

  > Dan Nicolaescu <dann@ics.uci.edu> writes:
  > > Why not turn on whitespace-mode by default in diff-mode?  It makes
  > > spotting whitespace problems very easy, and will make it also easy to
  > > fix.
  > 
  > I guess, it's the same as with normal files:  some people are very
  > concerned with whitespace problems, but many do not care; for the
  > latter, whitespace indication would simply be annoying noise.

The question is if the latter outnumber the former.   Given the amount
of rumbling about trailing whitespace, and the number of scripts/tools
etc. that deal with it, then I would say they don't...

Re: show whitespace problems by default in diff-mode

[Miles Bader]

Dan Nicolaescu <dann@ics.uci.edu> writes:
>   > I guess, it's the same as with normal files:  some people are very
>   > concerned with whitespace problems, but many do not care; for the
>   > latter, whitespace indication would simply be annoying noise.
>
> The question is if the latter outnumber the former.   Given the amount
> of rumbling about trailing whitespace, and the number of scripts/tools
> etc. that deal with it, then I would say they don't...

Huh?  "The amount of rumbling"?  You must be reading different dev lists
than I do, because I rarely see it even mentioned; the git list has a
higher number than usual, I'd wager, but otoh, the people there tend to
be much more fussy (aka pedantic) than usual about lots of things.

There are clearly some people that really, really, care about it, and
they are indeed quite vocal -- but AFAICT, most people just don't care.

-Miles

-- 
Education, n. That which discloses to the wise and disguises from the foolish
their lack of understanding.

Re: show whitespace problems by default in diff-mode

[Óscar Fuentes]

Miles Bader <miles@gnu.org> writes:

> Dan Nicolaescu <dann@ics.uci.edu> writes:
>>   > I guess, it's the same as with normal files:  some people are very
>>   > concerned with whitespace problems, but many do not care; for the
>>   > latter, whitespace indication would simply be annoying noise.
>>
>> The question is if the latter outnumber the former.   Given the amount
>> of rumbling about trailing whitespace, and the number of scripts/tools
>> etc. that deal with it, then I would say they don't...
>
> Huh?  "The amount of rumbling"?  You must be reading different dev lists
> than I do, because I rarely see it even mentioned; the git list has a
> higher number than usual, I'd wager, but otoh, the people there tend to
> be much more fussy (aka pedantic) than usual about lots of things.
>
> There are clearly some people that really, really, care about it, and
> they are indeed quite vocal -- but AFAICT, most people just don't care.

It's not only about pedantry. The worst thing about trailing whitespace
is that it tends to come and go while editing the file and when finally
the changes are committed it creates noise on VCS diffs and is
considered antisocial. This is why on some sites trailing whitespace is
forbidden and people there would appreciate a method for easy detection
and some tools for deletion.

Maybe not changing the defaults, but some highly visible thing somewhere
(a submenu item of Diff's menu, for instance) for activating an
"anti-whitespace" group of tools and settings would be justified by the
importance this features have for some users.

-- 
Óscar

Re: show whitespace problems by default in diff-mode

[Miles Bader]

Óscar Fuentes <ofv@wanadoo.es> writes:
> It's not only about pedantry. The worst thing about trailing whitespace
> is that it tends to come and go while editing the file and when finally
> the changes are committed it creates noise on VCS diffs and is
> considered antisocial. This is why on some sites trailing whitespace is
> forbidden and people there would appreciate a method for easy detection
> and some tools for deletion.

No doubt.  But that's a specific special case: when you're viewing a
diff of changes over which one has easy control (e.g., vc-diff of the
working tree).

I can see an argument for turning on highlighting of "introduced
trailing whitespace" (not in the context lines) by default for vc-diff
of the working tree, but more general ("pedantic") highlighting
shouldn't be on by default.

-Miles

-- 
My spirit felt washed.  With blood.  [Eli Shin, on "The Passion of the Christ"]

Re: show whitespace problems by default in diff-mode

[Dan Nicolaescu]

Miles Bader <miles@gnu.org> writes:

  > Óscar Fuentes <ofv@wanadoo.es> writes:
  > > It's not only about pedantry. The worst thing about trailing whitespace
  > > is that it tends to come and go while editing the file and when finally
  > > the changes are committed it creates noise on VCS diffs and is
  > > considered antisocial. This is why on some sites trailing whitespace is
  > > forbidden and people there would appreciate a method for easy detection
  > > and some tools for deletion.
  > 
  > No doubt.  But that's a specific special case: when you're viewing a
  > diff of changes over which one has easy control (e.g., vc-diff of the
  > working tree).
  > 
  > I can see an argument for turning on highlighting of "introduced
  > trailing whitespace" (not in the context lines) by default for vc-diff
  > of the working tree, but more general ("pedantic") highlighting
  > shouldn't be on by default.

The feature we are talking about here is exclusively about highlighting
the "introduced trailing whitespace"...

Re: show whitespace problems by default in diff-mode

[Óscar Fuentes]

Dan Nicolaescu <dann@ics.uci.edu> writes:

> The feature we are talking about here is exclusively about
> highlighting the "introduced trailing whitespace"...

More precisely, the highlighting affects additions *and* removals of
trailing whitespace. The idea is that the diff shall not contain noise
due to alterations on the amount of trailing whitespaces contained in
the file. Avoid the addition of new trailing whitespace is a special
case of this scenario.

-- 
Óscar

Re: show whitespace problems by default in diff-mode

[Miles Bader]

Dan Nicolaescu <dann@ics.uci.edu> writes:
>   > I can see an argument for turning on highlighting of "introduced
>   > trailing whitespace" (not in the context lines) by default for vc-diff
>   > of the working tree, but more general ("pedantic") highlighting
>   > shouldn't be on by default.
>
> The feature we are talking about here is exclusively about highlighting
> the "introduced trailing whitespace"...

Only in vc-diff against the working tree?

I don't like trailing whitespace either, and would be happy if there was
an unobtrusive and convenient way to reduce it -- but I also look at
lots of diffs where such indications would simply be noise.

[It seems worth noting that diff-mode generally goes to a fair bit of
trouble to be restrained in its presentation...]

-Miles

-- 
Education, n. That which discloses to the wise and disguises from the foolish
their lack of understanding.

Re: show whitespace problems by default in diff-mode

[Dan Nicolaescu]

Miles Bader <miles@gnu.org> writes:

  > Dan Nicolaescu <dann@ics.uci.edu> writes:
  > >   > I can see an argument for turning on highlighting of "introduced
  > >   > trailing whitespace" (not in the context lines) by default for vc-diff
  > >   > of the working tree, but more general ("pedantic") highlighting
  > >   > shouldn't be on by default.
  > >
  > > The feature we are talking about here is exclusively about highlighting
  > > the "introduced trailing whitespace"...
  > 
  > Only in vc-diff against the working tree?

Always, we don't have anything to distinguish between vc-diff and diff
files.

  > I don't like trailing whitespace either, and would be happy if there was
  > an unobtrusive and convenient way to reduce it -- but I also look at
  > lots of diffs where such indications would simply be noise.
  > 
  > [It seems worth noting that diff-mode generally goes to a fair bit of
  > trouble to be restrained in its presentation...]

Which is great if you are a radiologist, so you can distinguish
thousands of nuances of gray.

Re: show whitespace problems by default in diff-mode

[Miles Bader]

Dan Nicolaescu <dann@ics.uci.edu> writes:
>   > Only in vc-diff against the working tree?
>
> Always, we don't have anything to distinguish between vc-diff and diff
> files.

Surely vc-diff actually invokes diff-mode somehow, so it can let-bind
something or whatever then.

<rummage rummage>

Ah, here it is, in `vc-diff-internal':  .... (diff-mode)

We could even add a `vc-diff-mode' hook, to be run after the call to
(diff-mode) returns.

-Miles

-- 
Is it true that nothing can be known?  If so how do we know this?  -Woody Allen

Re: show whitespace problems by default in diff-mode

[Óscar Fuentes]

Dan Nicolaescu <dann@ics.uci.edu> writes:

> diff-mode is set up to show trailing whitespace problems, but it's not
> doing it by default, to enable this one has to do:
>
> (add-hook 'diff-mode-hook 'whitespace-mode)
>
> Vinicius also posted a patch to edit files to clean the trailing
> whitespace problems based on the diff.
>
> Why not turn on whitespace-mode by default in diff-mode?  It makes
> spotting whitespace problems very easy, and will make it also easy to
> fix.

Time ago I posted a patch for removing new trailing whitespace from
files based on their diffs. Vinicius suggested several improvements, so
maybe this code is the one that you mention. Aparently the maintainers
forgot about it and never was incorporated to Emacs. Just in case, here
is the version I'm using now:

(defun diff-delete-new-trailing-whitespace ()
  "When on a buffer that contains a diff, inspects the
differences and removes trailing whitespace (spaces, tabs) from
the lines modified or introduced by this diff. Shows a message
with the name of the altered buffers, which are unsaved.  If a
file referenced on the diff has no buffer and needs to be fixed,
a buffer visiting that file is created."
  (interactive)
  (goto-char (point-min))
  ;; We assume that the diff header has no trailing whitespace.
  (setq modified-buffers nil)
  (setq white-positions nil)
  (while (re-search-forward "^[+!>]" (point-max) t)
    (save-excursion
      (destructuring-bind (buf line-offset pos src dst &optional switched)
	  (diff-find-source-location t t)
	(when line-offset
	  (with-current-buffer buf
	    (goto-char (+ (car pos) (cdr src)))
	    (beginning-of-line)
	    (when (re-search-forward "\\([ \t]+\\)$" (line-end-position) t)
	      (when (not (member buf modified-buffers))
		(push buf modified-buffers))
	      (goto-char (match-end 0))
	      (push (point-marker) white-positions)
	      (goto-char (match-beginning 0))
	      (push (point-marker) white-positions)
	      (push buf white-positions)))))))
  (while white-positions
    (with-current-buffer (pop white-positions)
      (delete-region (pop white-positions) (pop white-positions))))
  (if modified-buffers
      (let ((msg "Deleted new trailing whitespace from:"))
	(dolist (f modified-buffers)
	  (setq msg (concat msg " `" (buffer-name f) "'")))
	(message "%s" msg))
    (message "No fixes needed.")))

It has a bug: if the last chunk of the diff does not end with a newline
(because the original file lacks it) `diff-find-source-location' returns
`line-offset' as nil and hence no whitespace is cleaned. No idea why.

-- 
Óscar

Re: show whitespace problems by default in diff-mode

[Dan Nicolaescu]

Óscar Fuentes <ofv@wanadoo.es> writes:

  > Dan Nicolaescu <dann@ics.uci.edu> writes:
  > 
  > > diff-mode is set up to show trailing whitespace problems, but it's not
  > > doing it by default, to enable this one has to do:
  > >
  > > (add-hook 'diff-mode-hook 'whitespace-mode)
  > >
  > > Vinicius also posted a patch to edit files to clean the trailing
  > > whitespace problems based on the diff.
  > >
  > > Why not turn on whitespace-mode by default in diff-mode?  It makes
  > > spotting whitespace problems very easy, and will make it also easy to
  > > fix.
  > 
  > Time ago I posted a patch for removing new trailing whitespace from
  > files based on their diffs. Vinicius suggested several improvements, so
  > maybe this code is the one that you mention. Aparently the maintainers
  > forgot about it and never was incorporated to Emacs. 

We had a feature freeze for a while, it could be incorporated.  Now it
can.  You can ask Stefan to check it in now.

Re: show whitespace problems by default in diff-mode

[Stefan Monnier]

> Why not turn on whitespace-mode by default in diff-mode?

Those people who care enough about trailing whitespace will have no
difficulty finding it.  They're usually obsessive enough that they will
go through an extensive search for the best way to eradicate
whitespace in their tools.


        Stefan