( )⚙ D422 mdiff: add a --ignore-space-at-eol option

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D422

mdiff: add a --ignore-space-at-eol option
ClosedPublic

Authored by dsp on Aug 16 2017, 6:13 PM.

Details

Reviewers
quark
yuja
Group Reviewers
hg-reviewers
Commits
rHGda07367d683b: mdiff: add a --ignore-space-at-eol option
rHGae7308d7604e: mdiff: add a --ignore-space-at-eol option
Summary

Add an option that only ignores whitespaces at EOL. The name of the option is
the same as Git.

.. feature::

Added `--ignore-space-at-eol` diff option to ignore whitespace differences at line endings.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

dsp created this revision.Aug 16 2017, 6:13 PM
quark accepted this revision.Aug 16 2017, 8:25 PM
quark added a subscriber: quark.

Looks good to me. It seems to be a significant feature that is worth a release note. Maybe check https://www.mercurial-scm.org/wiki/ReleasenotesExtension and amend the commit message to include something like:

.. feature::

   Added `--ignore-space-at-eol` diff option to ignore whitespace differences at line endings.
mercurial/help/config.txt
317

Maybe remove only since otherwise it sounds ignorewseol must be used together with ignorews.

dsp edited the summary of this revision. (Show Details)Aug 16 2017, 9:15 PM
dsp updated this revision to Diff 1022.
dsp added a comment.Aug 16 2017, 9:17 PM

Updated the diff with your suggested changed

martinvonz added a subscriber: martinvonz.Aug 23 2017, 1:30 PM

Looks like you didn't run test-help.t. I'll fix it in flight, but please run all tests next time.

martinvonz added a comment.Aug 23 2017, 1:33 PM
In D422#7795, @martinvonz wrote:

Looks like you didn't run test-help.t. I'll fix it in flight, but please run all tests next time.

...and test-completion.t and test-qrecord.t

martinvonz added a comment.Aug 23 2017, 1:35 PM
In D422#7797, @martinvonz wrote:
In D422#7795, @martinvonz wrote:

Looks like you didn't run test-help.t. I'll fix it in flight, but please run all tests next time.

...and test-completion.t and test-qrecord.t

...and test-record.t and test-duplicateoptions.py. This is getting a bit too many, especially since the last one is not trivial, so please send an update instead.

quark requested changes to this revision.Aug 23 2017, 2:06 PM

Maybe we should set up CI on Phabricator.

This revision now requires changes to proceed.Aug 23 2017, 2:06 PM
martinvonz added a comment.Aug 23 2017, 2:19 PM
In D422#7804, @quark wrote:

Maybe we should set up CI on Phabricator.

Yes, I've also suggested that. The concern was sandboxing, so we can run tests from untrusted people. I have no experience setting something like that up, but I'd be very happy if someone else could do it. It would be really nice to not have to run tests after applying a patch (if we're willing to live with the test failures we'd get every now and then due to conflicting changes).

akushner added a subscriber: akushner.Aug 23 2017, 2:26 PM

@martinvonz - a reasonable first pass would be to whitelist known contributors for CI runs.

martinvonz added a comment.Aug 23 2017, 4:08 PM
In D422#7818, @akushner wrote:

@martinvonz - a reasonable first pass would be to whitelist known contributors for CI runs.

That sounds reasonable. Still not my area of expertise, but I'd be happy to see it done.

dsp edited edge metadata.Aug 23 2017, 10:39 PM
dsp updated this revision to Diff 1240.
dsp marked an inline comment as done.Aug 23 2017, 10:39 PM
dsp updated this revision to Diff 1241.Aug 23 2017, 10:40 PM
morisgi added a subscriber: morisgi.Aug 24 2017, 2:49 AM
morisgi added inline comments.
mercurial/cmdutil.py
125–126

Regular diff chose -Z for the short option

yuja added a subscriber: yuja.Aug 24 2017, 11:10 AM
yuja added inline comments.
mercurial/cmdutil.py
125–126

-Z sounds good, which doesn't conflict with any existing short
options.

FWIW, the long option is called as --ignore-trailing-space in
GNU diff.

dsp added inline comments.Aug 24 2017, 1:55 PM
mercurial/cmdutil.py
125–126

I am happy to change '-Z'. The used the long opt name from git. I am happy either way, but I personally would prefer the git name.

dsp updated this revision to Diff 1266.Aug 24 2017, 6:38 PM
yuja requested changes to this revision.Aug 27 2017, 7:01 AM
yuja added inline comments.
mercurial/mdiff.py
102

This matches empty lines because \s includes \n.
Perhaps it should be r'[ \t\r]+\n'.

Can you add a test for this bug?

This revision now requires changes to proceed.Aug 27 2017, 7:01 AM
dsp edited edge metadata.Aug 29 2017, 9:21 PM
dsp updated this revision to Diff 1415.
yuja accepted this revision.Aug 30 2017, 9:26 AM

Queued, thanks.

Closed by commit rHGae7308d7604e: mdiff: add a --ignore-space-at-eol option (authored by dsp). · Explain WhyAug 30 2017, 9:53 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 1022

mercurial/cmdutil.py

diffwsopts = [ diffwsopts = [
('w', 'ignore-all-space', None, ('w', 'ignore-all-space', None,
_('ignore white space when comparing lines')), _('ignore white space when comparing lines')),
('b', 'ignore-space-change', None, ('b', 'ignore-space-change', None,
_('ignore changes in the amount of white space')), _('ignore changes in the amount of white space')),
('B', 'ignore-blank-lines', None, ('B', 'ignore-blank-lines', None,
_('ignore changes whose lines are all blank')), _('ignore changes whose lines are all blank')),
('e', 'ignore-space-at-eol', None,
_('ignore changes in whitespace at EOL')),
morisgiUnsubmitted
Not Done

Regular diff chose -Z for the short option

morisgi: Regular diff chose -Z for the short option
yujaUnsubmitted
Not Done

-Z sounds good, which doesn't conflict with any existing short
options.

FWIW, the long option is called as --ignore-trailing-space in
GNU diff.

yuja: `-Z` sounds good, which doesn't conflict with any existing short options. FWIW, the long…
dspAuthorUnsubmitted
Not Done

I am happy to change '-Z'. The used the long opt name from git. I am happy either way, but I personally would prefer the git name.

dsp: I am happy to change '-Z'. The used the long opt name from git. I am happy either way, but I…
] ]
diffopts2 = [ diffopts2 = [
('', 'noprefix', None, _('omit a/ and b/ prefixes from filenames')), ('', 'noprefix', None, _('omit a/ and b/ prefixes from filenames')),
('p', 'show-function', None, _('show which function each change is in')), ('p', 'show-function', None, _('show which function each change is in')),
('', 'reverse', None, _('produce a diff that undoes the changes')), ('', 'reverse', None, _('produce a diff that undoes the changes')),
] + diffwsopts + [ ] + diffwsopts + [
('U', 'unified', '', ('U', 'unified', '',

mercurial/help/config.txt

Settings used when displaying file annotations. All values are Settings used when displaying file annotations. All values are
Booleans and default to False. See :hg:`help config.diff` for Booleans and default to False. See :hg:`help config.diff` for
related options for the diff command. related options for the diff command.
``ignorews`` ``ignorews``
Ignore white space when comparing lines. Ignore white space when comparing lines.
``ignorewseol``
Ignore white space at the end of a line when comparing lines.
quarkUnsubmitted
Done

Maybe remove only since otherwise it sounds ignorewseol must be used together with ignorews.

quark: Maybe remove `only` since otherwise it sounds `ignorewseol` must be used together with…
``ignorewsamount`` ``ignorewsamount``
Ignore changes in the amount of white space. Ignore changes in the amount of white space.
``ignoreblanklines`` ``ignoreblanklines``
Ignore changes whose lines are all blank. Ignore changes whose lines are all blank.
``auth`` ``auth``

mercurial/mdiff.py

'showfunc': False, 'showfunc': False,
'git': False, 'git': False,
'nodates': False, 'nodates': False,
'nobinary': False, 'nobinary': False,
'noprefix': False, 'noprefix': False,
'index': 0, 'index': 0,
'ignorews': False, 'ignorews': False,
'ignorewsamount': False, 'ignorewsamount': False,
'ignorewseol': False,
'ignoreblanklines': False, 'ignoreblanklines': False,
'upgrade': False, 'upgrade': False,
'showsimilarity': False, 'showsimilarity': False,
} }
def __init__(self, **opts): def __init__(self, **opts):
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
for k in self.defaults.keys(): for k in self.defaults.keys():
def wsclean(opts, text, blank=True): def wsclean(opts, text, blank=True):
if opts.ignorews: if opts.ignorews:
text = bdiff.fixws(text, 1) text = bdiff.fixws(text, 1)
elif opts.ignorewsamount: elif opts.ignorewsamount:
text = bdiff.fixws(text, 0) text = bdiff.fixws(text, 0)
if blank and opts.ignoreblanklines: if blank and opts.ignoreblanklines:
text = re.sub('\n+', '\n', text).strip('\n') text = re.sub('\n+', '\n', text).strip('\n')
if opts.ignorewseol:
text = re.sub(r'\s+\n', r'\n', text)
yujaUnsubmitted
Not Done

This matches empty lines because \s includes \n.
Perhaps it should be r'[ \t\r]+\n'.

Can you add a test for this bug?

yuja: This matches empty lines because `\s` includes `\n`. Perhaps it should be `r'[ \t\r]+\n'`. Can…
return text return text
def splitblock(base1, lines1, base2, lines2, opts): def splitblock(base1, lines1, base2, lines2, opts):
# The input lines matches except for interwoven blank lines. We # The input lines matches except for interwoven blank lines. We
# transform it into a sequence of matching blocks and blank blocks. # transform it into a sequence of matching blocks and blank blocks.
lines1 = [(wsclean(opts, l) and 1 or 0) for l in lines1] lines1 = [(wsclean(opts, l) and 1 or 0) for l in lines1]
lines2 = [(wsclean(opts, l) and 1 or 0) for l in lines2] lines2 = [(wsclean(opts, l) and 1 or 0) for l in lines2]
s1, e1 = 0, len(lines1) s1, e1 = 0, len(lines1)
line entry. type is '=' for blocks matching exactly one another line entry. type is '=' for blocks matching exactly one another
(bdiff blocks), '!' for non-matching blocks and '~' for blocks (bdiff blocks), '!' for non-matching blocks and '~' for blocks
matching only after having filtered blank lines. matching only after having filtered blank lines.
line1 and line2 are text1 and text2 split with splitnewlines() if line1 and line2 are text1 and text2 split with splitnewlines() if
they are already available. they are already available.
""" """
if opts is None: if opts is None:
opts = defaultopts opts = defaultopts
if opts.ignorews or opts.ignorewsamount: if opts.ignorews or opts.ignorewsamount or opts.ignorewseol:
text1 = wsclean(opts, text1, False) text1 = wsclean(opts, text1, False)
text2 = wsclean(opts, text2, False) text2 = wsclean(opts, text2, False)
diff = bdiff.blocks(text1, text2) diff = bdiff.blocks(text1, text2)
for i, s1 in enumerate(diff): for i, s1 in enumerate(diff):
# The first match is special. # The first match is special.
# we've either found a match starting at line 0 or a match later # we've either found a match starting at line 0 or a match later
# in the file. If it starts later, old and new below will both be # in the file. If it starts later, old and new below will both be
# empty and we'll continue to the next match. # empty and we'll continue to the next match.

mercurial/patch.py

buildopts['index'] = hlen buildopts['index'] = hlen
if whitespace: if whitespace:
buildopts['ignorews'] = get('ignore_all_space', 'ignorews') buildopts['ignorews'] = get('ignore_all_space', 'ignorews')
buildopts['ignorewsamount'] = get('ignore_space_change', buildopts['ignorewsamount'] = get('ignore_space_change',
'ignorewsamount') 'ignorewsamount')
buildopts['ignoreblanklines'] = get('ignore_blank_lines', buildopts['ignoreblanklines'] = get('ignore_blank_lines',
'ignoreblanklines') 'ignoreblanklines')
buildopts['ignorewseol'] = get('ignore_space_at_eol', 'ignorewseol')
if formatchanging: if formatchanging:
buildopts['text'] = opts and opts.get('text') buildopts['text'] = opts and opts.get('text')
binary = None if opts is None else opts.get('binary') binary = None if opts is None else opts.get('binary')
buildopts['nobinary'] = (not binary if binary is not None buildopts['nobinary'] = (not binary if binary is not None
else get('nobinary', forceplain=False)) else get('nobinary', forceplain=False))
buildopts['noprefix'] = get('noprefix', forceplain=False) buildopts['noprefix'] = get('noprefix', forceplain=False)
return mdiff.diffopts(**pycompat.strkwargs(buildopts)) return mdiff.diffopts(**pycompat.strkwargs(buildopts))

tests/test-diff-ignore-whitespace.t

@@ -1,2 +1,3 @@ @@ -1,2 +1,3 @@
-hello world -hello world
-goodbye world -goodbye world
+hello world\r (esc) +hello world\r (esc)
+\r (esc) +\r (esc)
+goodbye\r (no-eol) (esc) +goodbye\r (no-eol) (esc)
world world
Test \r (carriage return) as used in "DOS" line endings:
$ printf 'hello world \r\ngoodbye world\n' >foo
$ hg ndiff --ignore-space-at-eol
No completely blank lines to ignore: No completely blank lines to ignore:
$ printf 'hello world\r\n\r\ngoodbye\rworld\n' >foo
$ hg ndiff --ignore-blank-lines $ hg ndiff --ignore-blank-lines
diff -r 540c40a65b78 foo diff -r 540c40a65b78 foo
--- a/foo --- a/foo
+++ b/foo +++ b/foo
@@ -1,2 +1,3 @@ @@ -1,2 +1,3 @@
-hello world -hello world
-goodbye world -goodbye world
+hello world\r (esc) +hello world\r (esc)