This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5490

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

navaneeth.suresh added a comment.Jan 10 2019, 8:13 AM

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.

navaneeth.suresh added a comment.Jan 10 2019, 11:29 PM

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

navaneeth.suresh added a comment.Jan 11 2019, 10:13 AM

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.

navaneeth.suresh added a comment.Jan 13 2019, 9:31 AM

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

KenyaCollier added a subscriber: KenyaCollier.Wed, May 25, 1:25 PM

Your ideas are incredible. The way of encouraging the new talent is truly good and completely amazing. I have been looking for high school essay writing blogs and it's simply a wonderful time to gather the roles positions and timings.

Revision Contents
Changeset List

PathPackages
Mhgext/record.py (6 lines)
Mmercurial/cmdutil.py (3 lines)
Mtests/test-commit-interactive.t (37 lines)

Diff 13177

hgext/record.py

if not ui.interactive(): if not ui.interactive():
raise error.Abort(_('running non-interactively, use %s instead') % raise error.Abort(_('running non-interactively, use %s instead') %
'commit') 'commit')
opts[r"interactive"] = True opts[r"interactive"] = True
overrides = {('experimental', 'crecord'): False} overrides = {('experimental', 'crecord'): False}
with ui.configoverride(overrides, 'record'): with ui.configoverride(overrides, 'record'):
return commands.commit(ui, repo, *pats, **opts) return commands.commit(ui, repo, *pats, ignorews=True, **opts)
def qrefresh(origfn, ui, repo, *pats, **opts): def qrefresh(origfn, ui, repo, *pats, **opts):
if not opts[r'interactive']: if not opts[r'interactive']:
return origfn(ui, repo, *pats, **opts) return origfn(ui, repo, *pats, **opts)
mq = extensions.find('mq') mq = extensions.find('mq')
def committomq(ui, repo, *pats, **opts): def committomq(ui, repo, *pats, **opts):
# At this point the working copy contains only changes that # At this point the working copy contains only changes that
# were accepted. All other changes were reverted. # were accepted. All other changes were reverted.
# We can't pass *pats here since qrefresh will undo all other # We can't pass *pats here since qrefresh will undo all other
# changed files in the patch that aren't in pats. # changed files in the patch that aren't in pats.
mq.refresh(ui, repo, **opts) mq.refresh(ui, repo, **opts)
# backup all changed files # backup all changed files
cmdutil.dorecord(ui, repo, committomq, None, True, cmdutil.dorecord(ui, repo, committomq, None, True,
cmdutil.recordfilter, *pats, **opts) cmdutil.recordfilter, *pats, ignorews=True, **opts)
# This command registration is replaced during uisetup(). # This command registration is replaced during uisetup().
@command('qrecord', @command('qrecord',
[], [],
_('hg qrecord [OPTION]... PATCH [FILE]...'), _('hg qrecord [OPTION]... PATCH [FILE]...'),
helpcategory=command.CATEGORY_COMMITTING, helpcategory=command.CATEGORY_COMMITTING,
inferrepo=True) inferrepo=True)
def qrecord(ui, repo, patch, *pats, **opts): def qrecord(ui, repo, patch, *pats, **opts):
def committomq(ui, repo, *pats, **opts): def committomq(ui, repo, *pats, **opts):
opts[r'checkname'] = False opts[r'checkname'] = False
mq.new(ui, repo, patch, *pats, **opts) mq.new(ui, repo, patch, *pats, **opts)
overrides = {('experimental', 'crecord'): False} overrides = {('experimental', 'crecord'): False}
with ui.configoverride(overrides, 'record'): with ui.configoverride(overrides, 'record'):
cmdutil.dorecord(ui, repo, committomq, cmdsuggest, False, cmdutil.dorecord(ui, repo, committomq, cmdsuggest, False,
cmdutil.recordfilter, *pats, **opts) cmdutil.recordfilter, *pats, ignorews=True, **opts)
def qnew(origfn, ui, repo, patch, *args, **opts): def qnew(origfn, ui, repo, patch, *args, **opts):
if opts[r'interactive']: if opts[r'interactive']:
return _qrecord(None, ui, repo, patch, *args, **opts) return _qrecord(None, ui, repo, patch, *args, **opts)
return origfn(ui, repo, patch, *args, **opts) return origfn(ui, repo, patch, *args, **opts)
def uisetup(ui): def uisetup(ui):

mercurial/cmdutil.py

testfile, operation) testfile, operation)
finally: finally:
ui.write = oldwrite ui.write = oldwrite
return newchunks, newopts return newchunks, newopts
def dorecord(ui, repo, commitfunc, cmdsuggest, backupall, def dorecord(ui, repo, commitfunc, cmdsuggest, backupall,
filterfn, *pats, **opts): filterfn, *pats, **opts):
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
ignorews = opts.get('ignorews', False)
if not ui.interactive(): if not ui.interactive():
if cmdsuggest: if cmdsuggest:
msg = _('running non-interactively, use %s instead') % cmdsuggest msg = _('running non-interactively, use %s instead') % cmdsuggest
else: else:
msg = _('running non-interactively') msg = _('running non-interactively')
raise error.Abort(msg) raise error.Abort(msg)
# make sure username is set before going interactive # make sure username is set before going interactive
if not force: if not force:
vdirs = [] vdirs = []
match.explicitdir = vdirs.append match.explicitdir = vdirs.append
match.bad = fail match.bad = fail
status = repo.status(match=match) status = repo.status(match=match)
if not force: if not force:
repo.checkcommitpatterns(wctx, vdirs, match, status, fail) repo.checkcommitpatterns(wctx, vdirs, match, status, fail)
diffopts = patch.difffeatureopts(ui, opts=opts, whitespace=True) diffopts = patch.difffeatureopts(ui, opts=opts, whitespace=ignorews)
diffopts.nodates = True diffopts.nodates = True
diffopts.git = True diffopts.git = True
diffopts.showfunc = True diffopts.showfunc = True
originaldiff = patch.diff(repo, changes=status, opts=diffopts) originaldiff = patch.diff(repo, changes=status, opts=diffopts)
originalchunks = patch.parsepatch(originaldiff) originalchunks = patch.parsepatch(originaldiff)
# 1. filter patch, since we are intending to apply subset of it # 1. filter patch, since we are intending to apply subset of it
try: try:

tests/test-commit-interactive.t

> [extensions] > [extensions]
> fakepatchtime = ! > fakepatchtime = !
> EOF > EOF
$ hg debugstate | grep ' subdir/f1$' $ hg debugstate | grep ' subdir/f1$'
n 0 -1 unset subdir/f1 n 0 -1 unset subdir/f1
$ hg status -A subdir/f1 $ hg status -A subdir/f1
M subdir/f1 M subdir/f1
making --interactive not ignore whitespaces with the following hgrc:
[diff]
ignorews=True
$ hg init issue6042
$ cd issue6042
$ cat >> $HGRCPATH << EOF
> [diff]
> ignorews = True
> [extensions]
> record =
> EOF
$ echo a > a
$ hg ci -Am 'add a'
adding a
$ echo 'a ' > a
$ hg diff
$ hg commit -i -m 'add ws to a' <<EOF
> y
> y
> n
> EOF
diff --git a/a b/a
1 hunks, 1 lines changed
examine changes to 'a'? [Ynesfdaq?] y
@@ -1,1 +1,1 @@
-a
+a
record this change to 'a'? [Ynesfdaq?] y
let's check whether record extension works fine or not after the fix
$ echo 'a ' > a
$ hg record --ignore-all-space
no changes to record
[1]