Page MenuHomePhabricator

commit: remove ignore whitespace option on --interactive (issue6042)
AbandonedPublic

Authored by navaneeth.suresh on Jan 4 2019, 5:23 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

hg commit --interactive don't see changes to whitespace on having ignorews=True under [diff] in .hgrc. This is derived from record extension according to 486a1fe09422. I removed whitespace=True from patch.difffeatureopts() in cmdutils.dorecord.recordfunc() and got this to work without failing any tests. I'll be sending follow up patches to this if this fails to work on any other cases which don't have tests written. Perhaps calling cmdutils.dorecord with additional arguments only for record extension et al be a nice fix then.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit added a subscriber: pulkit.Jan 4 2019, 12:28 PM

I am not sure whether this change is correct or not, but please add more description to your commit message explaining what this patch is doing, what the issue is about and how it fixes that.

navaneeth.suresh edited the summary of this revision. (Show Details)Jan 5 2019, 4:43 AM
In D5490#81294, @pulkit wrote:

I am not sure whether this change is correct or not, but please add more description to your commit message explaining what this patch is doing, what the issue is about and how it fixes that.

Done @pulkit

durin42 added a subscriber: durin42.Jan 9 2019, 2:47 PM

Could you add a test?

(Bonus points if you add a test that shows the wrong behavior and then fix it in this change.)

Could you add a test?
(Bonus points if you add a test that shows the wrong behavior and then fix it in this change.)

Sure. Will do.

@durin42 I've added test for the current revision. Unfortunately, I'm unable to write a test for the wrong behaviour. I'll try writing it again. Any hints from your side?

yuja added a subscriber: yuja.Jan 11 2019, 7:51 AM
  • diffopts = patch.difffeatureopts(ui, opts=opts, whitespace=True)

+ diffopts = patch.difffeatureopts(ui, opts=opts)

Sorry for late, but doesn't it break hg record --ignore-all-space, etc.?
It was intentionally added by 3f1dccea9510 (with no tests.)

Sorry for late, but doesn't it break hg record --ignore-all-space, etc.?

Yes. Exactly! Thanks for pointing that out.

It was intentionally added by 3f1dccea9510 (with no tests.)

Do you want me to add tests for that? If yes, please mention the things I need to take care of on
writing those tests.

I've updated the revision and things are working fine. There is still whitespace=True existing
on calling patch.difffeatureopts() in fastannotate, diffutil.diffallopts(),
webutil.difffeatureopts() and `cmdutil._performrevert(). Is there anything
to be done on that?

yuja added a comment.Jan 12 2019, 8:55 PM
  • a/mercurial/cmdutil.py

+++ b/mercurial/cmdutil.py
@@ -237,6 +237,7 @@
def dorecord(ui, repo, commitfunc, cmdsuggest, backupall,

        filterfn, *pats, **opts):
opts = pycompat.byteskwargs(opts)

+ ignorews = opts.get('ignorews', False)

It isn't nice to mix command options and internal flags. Instead, maybe we can
first change record() to not call commands.commit(), and pass in whitespace
option to cmdutil.dorecord().

def record(ui, repo, *pats, **opts):
    opts = pycompat.byteskwargs(opts)
    ...
    with repo.wlock(), repo.lock():
        ret = cmdutil.dorecord(ui, repo, commands.commit, ..., whitespace=True,
                               pats, opts)
        ...

And I don't think ignorews is good name. It doesn't mean whitespace is
ignored.

def qrefresh(origfn, ui, repo, *pats, **opts):

if not opts[r'interactive']:

@@ -89,7 +89,7 @@

    1. backup all changed files cmdutil.dorecord(ui, repo, committomq, None, True,
  • cmdutil.recordfilter, *pats, **opts)

+ cmdutil.recordfilter, *pats, ignorews=True, **opts)

Perhaps, this one should be ignorefs=False since qrefresh has no diffwsopts.

yuja added a comment.Jan 12 2019, 9:10 PM
> It was intentionally added by https://phab.mercurial-scm.org/rHG3f1dccea9510c122cf9ab0e7a5a19ceed3600a7f (with no tests.)
Do you want me to add tests for that?  If yes, please mention the things I need to take care of on
 writing those tests.

I think you can reuse the test you've written for commit -i.

I've updated the revision and things are working fine. There is still `whitespace=True` existing
on calling `patch.difffeatureopts()` in `fastannotate`, `diffutil.diffallopts()`,
`webutil.difffeatureopts()` and ``cmdutil._performrevert()`. Is there anything
to be done on that?

Good questionn. Maybe we can't simply fix the issue6042 by turning off the
whitespace options?

_performrevert() would be in the same boat, but it was explicitly flagged on
at f37a69ec3f47. This implies that the diff.ignorews option would be used
in practice to exclude whitespace changes while interactive commit/revert.
So disabling any whitespace options would break someone's workflow.

_performrevert() would be in the same boat, but it was explicitly flagged on
at f37a69ec3f47. This implies that the diff.ignorews option would be used
in practice to exclude whitespace changes while interactive commit/revert.
So disabling any whitespace options would break someone's workflow.

I'm confused after this comment. How do you want me to move forward?

yuja added a comment.Jan 13 2019, 10:58 PM
> `_performrevert()` would be in the same boat, but it was explicitly flagged on
>  at https://phab.mercurial-scm.org/rHGf37a69ec3f4717fdb4f00699ca06c225f106696c. This implies that the `diff.ignorews` option would be used
>  in practice to exclude whitespace changes while interactive commit/revert.
>  So disabling any whitespace options would break someone's workflow.
I'm confused after this comment. How do you want me to move forward?

Yeah, I'm also confused about the current state. Honestly, I'm out of ideas.
We could add diff options to commit and revert, but I'm not sure if they
are widely used enough to being options of these commands.

yuja added a comment.Feb 7 2019, 8:06 AM
@spectral @yuja Can I close this?

Probably yes.

navaneeth.suresh abandoned this revision.Feb 7 2019, 11:46 AM

rHG66399f2e92aa solves this issue, many thanks @spectral. Closing this.