This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5834

commit/revert: if interactive, look elsewhere for whitespace settings (BC)
ClosedPublic

Authored by spectral on Feb 4 2019, 3:39 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG66399f2e92aa: commit: if interactive, look elsewhere for whitespace settings (BC)
Summary

Previously, when doing commit -i, we respected diff.ignorews and other
whitespace-related settings, which is probably unexpected. The primary reason
for this is to support hgext.record's commandline options, it's probably
accidental that the [diff] settings were also considered. See comments on
issue6042 and D5490. This can cause problems (issue5839, issue6042).

It is assumed by the author that the [diff] section is primarily for *viewing*
diffs, and that it is unlikely what people intend when attempting to commit or
revert.

With this change, if a user wants the behavior, they can clone their [diff]
settings to commands.commit.interactive.<setting>. This is thus a mild BC
change, but one I suspect is not going to be relied on by anyone.

Note: while doing a partial commit/revert, we do not know what command the user
is actually running. This means that the split extension, which ends up calling
into this code, will respect the commands.commit.interactive.<setting>
settings, and not a hypothetical commands.split.interactive.<setting>. This
*also* means that setting commands.commit.interactive.ignoreblanklines, for
example, will still cause issue5839. Considering the highly unlikely chance that
a user actually sets commands.commit.interactive.<setting>, the author deems
this risk acceptable.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

spectral created this revision.Feb 4 2019, 3:39 PM
yuja added a subscriber: yuja.Feb 5 2019, 7:03 AM

Updated the tests added at d1d3094b54f9 and queued, thanks.

Closed by commit rHG66399f2e92aa: commit: if interactive, look elsewhere for whitespace settings (BC) (authored by spectral). · Explain WhyFeb 5 2019, 7:12 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 13804

mercurial/cmdutil.py

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) diffopts = patch.difffeatureopts(ui, opts=opts, whitespace=True,
section='commands',
configprefix='commit.interactive.')
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:
else: else:
normal = repo.dirstate.normal normal = repo.dirstate.normal
newlyaddedandmodifiedfiles = set() newlyaddedandmodifiedfiles = set()
if interactive: if interactive:
# Prompt the user for changes to revert # Prompt the user for changes to revert
torevert = [f for f in actions['revert'][0] if f not in excluded_files] torevert = [f for f in actions['revert'][0] if f not in excluded_files]
m = scmutil.matchfiles(repo, torevert) m = scmutil.matchfiles(repo, torevert)
diffopts = patch.difffeatureopts(repo.ui) diffopts = patch.difffeatureopts(repo.ui, whitespace=True,
section='commands',
configprefix='revert.interactive.')
diffopts.nodates = True diffopts.nodates = True
diffopts.git = True diffopts.git = True
operation = 'discard' operation = 'discard'
reversehunks = True reversehunks = True
if node != parent: if node != parent:
operation = 'apply' operation = 'apply'
reversehunks = False reversehunks = False
if reversehunks: if reversehunks:

mercurial/configitems.py

generic=True, generic=True,
) )
coreconfigitem('color', 'mode', coreconfigitem('color', 'mode',
default='auto', default='auto',
) )
coreconfigitem('color', 'pagermode', coreconfigitem('color', 'pagermode',
default=dynamicdefault, default=dynamicdefault,
) )
_registerdiffopts(section='commands', configprefix='commit.interactive.')
coreconfigitem('commands', 'grep.all-files', coreconfigitem('commands', 'grep.all-files',
default=False, default=False,
) )
coreconfigitem('commands', 'resolve.confirm', coreconfigitem('commands', 'resolve.confirm',
default=False, default=False,
) )
coreconfigitem('commands', 'resolve.explicit-re-merge', coreconfigitem('commands', 'resolve.explicit-re-merge',
default=False, default=False,
) )
coreconfigitem('commands', 'resolve.mark-check', coreconfigitem('commands', 'resolve.mark-check',
default='none', default='none',
) )
_registerdiffopts(section='commands', configprefix='revert.interactive.')
coreconfigitem('commands', 'show.aliasprefix', coreconfigitem('commands', 'show.aliasprefix',
default=list, default=list,
) )
coreconfigitem('commands', 'status.relative', coreconfigitem('commands', 'status.relative',
default=False, default=False,
) )
coreconfigitem('commands', 'status.skipstates', coreconfigitem('commands', 'status.skipstates',
default=[], default=[],

tests/test-commit-interactive.t

> 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
Test diff.unified=0 Test commands.commit.interactive.unified=0
$ hg init $TESTTMP/b $ hg init $TESTTMP/b
$ cd $TESTTMP/b $ cd $TESTTMP/b
$ cat > foo <<EOF $ cat > foo <<EOF
> 1 > 1
> 2 > 2
> 3 > 3
> 4 > 4
> 5 > 5
> EOF > EOF
$ hg ci -qAm initial $ hg ci -qAm initial
$ cat > foo <<EOF $ cat > foo <<EOF
> 1 > 1
> change1 > change1
> 2 > 2
> 3 > 3
> change2 > change2
> 4 > 4
> 5 > 5
> EOF > EOF
$ printf 'y\ny\ny\n' | hg ci -im initial --config diff.unified=0 $ printf 'y\ny\ny\n' | hg ci -im initial --config commands.commit.interactive.unified=0
diff --git a/foo b/foo diff --git a/foo b/foo
2 hunks, 2 lines changed 2 hunks, 2 lines changed
examine changes to 'foo'? [Ynesfdaq?] y examine changes to 'foo'? [Ynesfdaq?] y
@@ -1,0 +2,1 @@ 1 @@ -1,0 +2,1 @@ 1
+change1 +change1
record change 1/2 to 'foo'? [Ynesfdaq?] y record change 1/2 to 'foo'? [Ynesfdaq?] y

tests/test-split.t

> split= > split=
> [ui] > [ui]
> interactive=1 > interactive=1
> color=no > color=no
> paginate=never > paginate=never
> [diff] > [diff]
> git=1 > git=1
> unified=0 > unified=0
> [commands]
> commit.interactive.unified=0
> [alias] > [alias]
> glog=log -G -T '{rev}:{node|short} {desc} {bookmarks}\n' > glog=log -G -T '{rev}:{node|short} {desc} {bookmarks}\n'
> EOF > EOF
#if obsstore-on #if obsstore-on
$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [experimental] > [experimental]
> evolution=all > evolution=all
$ hg bookmark r3 $ hg bookmark r3
$ hg split 'all()' $ hg split 'all()'
abort: cannot split multiple revisions abort: cannot split multiple revisions
[255] [255]
This function splits a bit strangely primarily to avoid changing the behavior of This function splits a bit strangely primarily to avoid changing the behavior of
the test after a bug was fixed with how split/commit --interactive handled the test after a bug was fixed with how split/commit --interactive handled
`diff.unified=0`: when there were no context lines, it kept only the last diff `commands.commit.interactive.unified=0`: when there were no context lines,
hunk. When running split, this meant that runsplit was always recording three commits, it kept only the last diff hunk. When running split, this meant that runsplit
one for each diff hunk, in reverse order (the base commit was the last diff hunk was always recording three commits, one for each diff hunk, in reverse order
in the file). (the base commit was the last diff hunk in the file).
$ runsplit() { $ runsplit() {
> cat > $TESTTMP/messages <<EOF > cat > $TESTTMP/messages <<EOF
> split 1 > split 1
> -- > --
> split 2 > split 2
> -- > --
> split 3 > split 3
> EOF > EOF