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

This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

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

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

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

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

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

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 inline comments.
mercurial/cmdutil.py
126–127

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
126–127

-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
126–127

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.

This revision was automatically updated to reflect the committed changes.