Page MenuHomePhabricator

CodeReview's diff shouldn't show whitespace changes (ViewVC doesn't either)
Closed, DeclinedPublic

Description

Compare this: http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/skins/common/mwsuggest.js?r1=42730&r2=42729&pathrev=42730

With the diff section of this: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/42730

The second is too messy. In fact, there are many lines like this:

  • // remove descriptor

+ // remove descriptor

Which are pointless. That revision doesn't change those lines, and the diff is actually mentioning an unchanged line.


Version: unspecified
Severity: enhancement

Details

Reference
bz16161

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:23 PM
bzimport set Reference to bz16161.
bzimport added a subscriber: Unknown Object (MLST).

(In reply to comment #0)

The second is too messy. In fact, there are many lines like this:

  • // remove descriptor

+ // remove descriptor

Which are pointless. That revision doesn't change those lines, and the diff is
actually mentioning an unchanged line.

Actually, those are whitespace changes. Select the diff text and you'll see the - line has trailing whitespace while the + line doesn't.

Hmm, may close this as wontfix then.

  • Bug 24547 has been marked as a duplicate of this bug. ***

This isn't a WONTFIX. It's an actual issue that needs to be addressed. Re-opening.

Closely related to this issue is bug 18143. It's been added here as a see also.

Bryan.TongMinh wrote:

Needs fixing in the PECL extension. In svn.c diff_options should be something
like {"-w", 0}.

But whitespace is important, it shouldn't be disabled unconditionally.

Just use '-x -wu' to ignore whitespace when svn diffing. I use the following script to review code:

$ cat ~/bin/mw_review
#!/bin/bash
if [ $# -ne 1 ]
then

	echo "Usage: $0 <revision number>"
	exit 1

fi
pushd .
svn diff -c $1 -x -wu | less -R
popd

Usage:
mw_review 75333

This should be a bug against the PECL extension since it doesn't allow you to pass libsvn arguments.

For shell diff we can add it on... "svn diff -r%d:%d %s %s"

So, from what I can see, this is a bug in (or choise by) ViewVC, not in CodeReview ?

bug 27375 was fixed in MediaWiki.org. Whitespace changes are now identifiable (in addition to being included) in diffs in CodeReview. Which makes it's inclusion no longer useless.

Marking this as WONTFIX.

See also bug 18143 which request an option to hide them in CodeReview.