This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5744

commit: ignore diff whitespace settings when doing `commit -i` (issue5839)
ClosedPublic

Authored by spectral on Jan 29 2019, 9:31 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG3a01ce246ece: commit: ignore diff whitespace settings when doing `commit -i` (issue5839)
rHG35e37315c058: commit: ignore diff whitespace settings when doing `commit -i` (issue5839)
rHG19486fc49020: commit: ignore diff whitespace settings when doing `commit -i` (issue5839)
Summary

Previously, we respected options like diff.ignoreblanklines and
diff.ignorews. This can cause problems when the user is attempting to
actually commit the blank line change. Specifically, the split extension can get
into an infinite loop because it detects that the working copy is not clean, but
when we get the diff we don't see the changes, so it just skips popping up the
chunk selection flow, saying there's no changes to record.

These options are primarily meant for viewing diffs; it is highly unlikely that
someone is actually intending to add extraneous whitespace and have it ignored
if they attempt to interactively commit (but *not* ignored if they
non-interactively commit).

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.Jan 29 2019, 9:31 PM
Closed by commit rHG19486fc49020: commit: ignore diff whitespace settings when doing `commit -i` (issue5839) (authored by spectral). · Explain WhyJan 30 2019, 2:08 PM
This revision was automatically updated to reflect the committed changes.
navaneeth.suresh added a subscriber: navaneeth.suresh.Jan 31 2019, 12:59 AM

Sorry for bumping this up. I worked on a similar issue on D5490. My initial revision was similar to this one. @spectral Don't you think it'll affect record extension which was written with zero test coverage? Please don't feel obligated. I am just being curious.

spectral added a comment.Jan 31 2019, 5:06 PM
In D5744#84781, @navaneeth.suresh wrote:

Sorry for bumping this up. I worked on a similar issue on D5490. My initial revision was similar to this one. @spectral Don't you think it'll affect record extension which was written with zero test coverage? Please don't feel obligated. I am just being curious.

I wasn't able to come up with a reason to support these but only when committing interactively (as I said in the commit description), but I guess there's justification in https://www.mercurial-scm.org/pipermail/mercurial-devel/2011-June/032316.html.

So I think I can understand why this is desired for this command. Maybe a better option is to change the difffeatureopts() call to replace the section that it looks in? i.e. change:

diffopts = patch.difffeatureopts(repo.ui, whitespace=True)

to:

diffopts = patch.difffeatureopts(repo.ui, section='commands-interactive', whitespace=True)

This way we aren't looking at the [diff] section, so the options specified on the commandline end up working, and if someone really does want this as a default, they can get that behavior.

I'm not at all attached to that section name, i.e. I could see this being something like the following, to make it look at commands.commit.interactive.ignorews (and others):

diffopts = patch.difffeatureopts(repo.ui, section='commands', configprefix='commit.interactive.', whitespace=True)

I think I like that option, I'll send a patch for it.

spectral added a comment.Jan 31 2019, 5:47 PM

I could see this being something like the following, to make it look at commands.commit.interactive.ignorews (and others):

diffopts = patch.difffeatureopts(repo.ui, section='commands', configprefix='commit.interactive.', whitespace=True)

I think I like that option, I'll send a patch for it.

When writing the commit description for that patch, I mentioned several times how I don't expect anyone to actually use the new config options I was creating. I think I'll go a different direction: just add support to difffeatureopts to ignore the config (only respect commandline flags).

yuja added a subscriber: yuja.Jan 31 2019, 5:57 PM
I wasn't able to come up with a reason to support these but only when committing interactively (as I said in the commit description), but I guess there's justification in https://www.mercurial-scm.org/pipermail/mercurial-devel/2011-June/032316.html.
So I think I can understand why this is desired for this command. Maybe a better option is to change the difffeatureopts() call to replace the section that it looks in? i.e. change:
  diffopts = patch.difffeatureopts(repo.ui, whitespace=True)
to:
  diffopts = patch.difffeatureopts(repo.ui, section='commands-interactive', whitespace=True)
This way we aren't looking at the `[diff]` section, so the options specified on the commandline end up working, and if someone really does want this as a default, they can get that behavior.
I'm not at all attached to that section name, i.e. I could see this being something like the following, to make it look at commands.commit.interactive.ignorews (and others):
  diffopts = patch.difffeatureopts(repo.ui, section='commands', configprefix='commit.interactive.', whitespace=True)
I think I like that option, I'll send a patch for it.

I can't think of a nicer section name, but I like the idea in general. We'll
also need to apply that to hg revert --interactive.

spectral added a comment.Jan 31 2019, 6:52 PM

I've sent D5787 and D5788, implementing support for whitespace='noconfig' to parse them from the commandline args, but ignore the settings from the user's config. I did not add this to revert --interactive, since that does not currently have any way of getting args specified on the commandline that affect the whitespace settings (so I'm keeping revert --interactive *ignoring* the user's diff settings).

yuja added a comment.Feb 1 2019, 8:20 AM

I did not add this to revert --interactive, since that does not currently have any way of getting args specified on the commandline that affect the whitespace settings (so I'm keeping revert --interactive *ignoring* the user's diff settings).

Well, hg revert --interactive does respect the user's diff settings. I don't
know why, but f37a69ec3f47 explicitly added whitespace=True.

spectral added a comment.Feb 1 2019, 2:20 PM
In D5744#84938, @yuja wrote:

I did not add this to revert --interactive, since that does not currently have any way of getting args specified on the commandline that affect the whitespace settings (so I'm keeping revert --interactive *ignoring* the user's diff settings).

Well, hg revert --interactive does respect the user's diff settings. I don't
know why, but f37a69ec3f47 explicitly added whitespace=True.

If I had to guess, I think that was just a copy/paste from commit --interactive without realizing the consequences; there were no tests added for revert --interactive, just for the git diffs (which was the subject of the patch). I still think that taking these from any config is super unlikely to be intended or desirable behavior, but if we want to keep it just in case, I do have an alternate series available that does my original idea of making it respect commands.{commit,revert}.interactive.{ignorews,ignoreblanklines,...}. The ultimate reason I abandoned that and went with the "only respect commandline options, not config, and don't do this for revert --interactive" patch series was because I had to include a note that said something like "if a user *does* set commands.commit.interactive.{ignorews,ignoreblanklines,...}, this will still trigger issue5839; it is unlikely that any user will want to set these options, though."

It's up to you, though. I worry I'm too ingrained in the internal-software development culture where we control what versions people have and can provide quick assistance when users have questions or encounter problems. I might not be properly considering the issues people might run into outside of an enterprise situation.

durin42 added a subscriber: durin42.Feb 1 2019, 2:24 PM
In D5744#84960, @spectral wrote:
In D5744#84938, @yuja wrote:

I did not add this to revert --interactive, since that does not currently have any way of getting args specified on the commandline that affect the whitespace settings (so I'm keeping revert --interactive *ignoring* the user's diff settings).

Well, hg revert --interactive does respect the user's diff settings. I don't
know why, but f37a69ec3f47 explicitly added whitespace=True.

If I had to guess, I think that was just a copy/paste from commit --interactive without realizing the consequences;

That would be my guess too. I'm inclined to treat that as a bug, since the whitespace elision will at best produce confusing results.

yuja added a comment.Feb 1 2019, 11:04 PM

The ultimate reason I abandoned that and went with the "only respect commandline options, not config, and don't do this for revert --interactive" patch series was because I had to include a note that said something like "if a user *does* set commands.commit.interactive.{ignorews,ignoreblanklines,...}, this will still trigger issue5839; it is unlikely that any user will want to set these options, though."

It's up to you, though. I worry I'm too ingrained in the internal-software development culture where we control what versions people have and can provide quick assistance when users have questions or encounter problems. I might not be properly considering the issues people might run into outside of an enterprise situation.

I agree with that we would never set the commands.commit.interactive.{...}
in hgrc, but the feature itself is useful if you have to work on unclean
codebase unlike in Google. For example, I sometimes need to commit changes
ignoring unrelated whitespace cleanups made by editor or code formatter,
because I can't control the development workflow.

That's why I thought there would be users relying on the current behavior.

spectral added a comment.Feb 4 2019, 3:40 PM
In D5744#85051, @yuja wrote:

I agree with that we would never set the commands.commit.interactive.{...}
in hgrc, but the feature itself is useful if you have to work on unclean
codebase unlike in Google. For example, I sometimes need to commit changes
ignoring unrelated whitespace cleanups made by editor or code formatter,
because I can't control the development workflow.
That's why I thought there would be users relying on the current behavior.

I think I understand what you're saying. I was under the impression that what we cared about was that hg record -b should continue working. Since there are no diffopts available on hg commit -i, you're thinking that this could be written as hg commit -i --config commands.commit.interactive.ignorews=1.

While I'm sympathetic to that argument, it is so long and unwieldy that I think I'd recommend that users just do hg --config extensions.record= record -b (or, more likely, set [extensions] record= in their hgrc).

I'll send the other patch series, we can discuss this on the relevant patches instead of keeping most of the discussion in a (somewhat unrelated) patch. I've sent D5832-D5834 with the config option approach, and still have D5877-D5878 as the "only respect commandline args" approach.

durin42 added a comment.Feb 4 2019, 4:54 PM
In D5744#85436, @spectral wrote:
In D5744#85051, @yuja wrote:

I agree with that we would never set the commands.commit.interactive.{...}
in hgrc, but the feature itself is useful if you have to work on unclean
codebase unlike in Google. For example, I sometimes need to commit changes
ignoring unrelated whitespace cleanups made by editor or code formatter,
because I can't control the development workflow.
That's why I thought there would be users relying on the current behavior.

I think I understand what you're saying. I was under the impression that what we cared about was that hg record -b should continue working. Since there are no diffopts available on hg commit -i, you're thinking that this could be written as hg commit -i --config commands.commit.interactive.ignorews=1.
While I'm sympathetic to that argument, it is so long and unwieldy that I think I'd recommend that users just do hg --config extensions.record= record -b (or, more likely, set [extensions] record= in their hgrc).
I'll send the other patch series, we can discuss this on the relevant patches instead of keeping most of the discussion in a (somewhat unrelated) patch. I've sent D5832-D5834 with the config option approach, and still have D5877-D5878 as the "only respect commandline args" approach.

In the name of getting things landed, I rearranged -committed so this change is now tip. I'll try and look at the new series soon.

Sorry for breaking a clever workflow. :/

yuja added a comment.Feb 5 2019, 7:03 AM
> I agree with that we would never set the `commands.commit.interactive.{...}`
>  in hgrc, but the feature itself is useful if you have to work on unclean
>  codebase unlike in Google. For example, I sometimes need to commit changes
>  ignoring unrelated whitespace cleanups made by editor or code formatter,
>  because I can't control the development workflow.
>
> That's why I thought there would be users relying on the current behavior.
I think I understand what you're saying.  I was under the impression that what we cared about was that `hg record -b` should continue working. Since there are no diffopts available on `hg commit -i`, you're thinking that this could be written as `hg commit -i --config commands.commit.interactive.ignorews=1`.
While I'm sympathetic to that argument, it is so long and unwieldy that I think I'd recommend that users just do `hg --config extensions.record= record -b` (or, more likely, set `[extensions] record=` in their hgrc).

Yeah. If we didn't have the same issue for revert, I would agree with that and
dropping --config diff.* support from hg commit -i.

I'll send the other patch series, we can discuss this on the relevant patches instead of keeping most of the discussion in a (somewhat unrelated) patch.  I've sent https://phab.mercurial-scm.org/D5832-D5834 with the config option approach, and still have D5877-D5878 as the "only respect commandline args" approach.

I've queued D5832-D5834 as it can provide escape hatch for a potential BC.
Many thanks!

Revision Contents
Changeset List

PathPackages
Mmercurial/cmdutil.py (4 lines)
Mtests/test-commit-interactive.t (44 lines)
Mtests/test-split.t (108 lines)

Diff 13572

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, whitespace=True) diffopts = patch.difffeatureopts(ui, opts=opts)
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, whitespace=True) diffopts = patch.difffeatureopts(repo.ui)
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:

tests/test-commit-interactive.t

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

tests/test-split.t

1df0d5c5a3ab draft 1df0d5c5a3ab draft
a61bcde8c529 draft a61bcde8c529 draft
$ runsplit tip >/dev/null $ runsplit tip >/dev/null
$ hg log -T '{short(node)} {phase}\n' $ hg log -T '{short(node)} {phase}\n'
00eebaf8d2e2 draft 00eebaf8d2e2 draft
a09ad58faae3 draft a09ad58faae3 draft
e704349bd21b draft e704349bd21b draft
a61bcde8c529 draft a61bcde8c529 draft
`hg split` with ignoreblanklines=1 does not infinite loop
$ mkdir $TESTTMP/f
$ hg init $TESTTMP/f/a
$ cd $TESTTMP/f/a
$ printf '1\n2\n3\n4\n5\n' > foo
$ cp foo bar
$ hg ci -qAm initial
$ printf '1\n\n2\n3\ntest\n4\n5\n' > bar
$ printf '1\n2\n3\ntest\n4\n5\n' > foo
$ hg ci -qm splitme
$ cat > $TESTTMP/messages <<EOF
> split 1
> --
> split 2
> EOF
$ printf 'f\nn\nf\n' | hg --config extensions.split= --config diff.ignoreblanklines=1 split
diff --git a/bar b/bar
2 hunks, 2 lines changed
examine changes to 'bar'? [Ynesfdaq?] f
diff --git a/foo b/foo
1 hunks, 1 lines changed
examine changes to 'foo'? [Ynesfdaq?] n
EDITOR: HG: Splitting dd3c45017cbf. Write commit message for the first split changeset.
EDITOR: splitme
EDITOR:
EDITOR:
EDITOR: HG: Enter commit message. Lines beginning with 'HG:' are removed.
EDITOR: HG: Leave message empty to abort commit.
EDITOR: HG: --
EDITOR: HG: user: test
EDITOR: HG: branch 'default'
EDITOR: HG: changed bar
created new head
diff --git a/foo b/foo
1 hunks, 1 lines changed
examine changes to 'foo'? [Ynesfdaq?] f
EDITOR: HG: Splitting dd3c45017cbf. So far it has been split into:
EDITOR: HG: - f205aea1c624: split 1
EDITOR: HG: Write commit message for the next split changeset.
EDITOR: splitme
EDITOR:
EDITOR:
EDITOR: HG: Enter commit message. Lines beginning with 'HG:' are removed.
EDITOR: HG: Leave message empty to abort commit.
EDITOR: HG: --
EDITOR: HG: user: test
EDITOR: HG: branch 'default'
EDITOR: HG: changed foo
saved backup bundle to $TESTTMP/f/a/.hg/strip-backup/dd3c45017cbf-463441b5-split.hg (obsstore-off !)
Let's try that again, with a slightly different set of patches, to ensure that
the ignoreblanklines thing isn't somehow position dependent.
$ hg init $TESTTMP/f/b
$ cd $TESTTMP/f/b
$ printf '1\n2\n3\n4\n5\n' > foo
$ cp foo bar
$ hg ci -qAm initial
$ printf '1\n2\n3\ntest\n4\n5\n' > bar
$ printf '1\n2\n3\ntest\n4\n\n5\n' > foo
$ hg ci -qm splitme
$ cat > $TESTTMP/messages <<EOF
> split 1
> --
> split 2
> EOF
$ printf 'f\nn\nf\n' | hg --config extensions.split= --config diff.ignoreblanklines=1 split
diff --git a/bar b/bar
1 hunks, 1 lines changed
examine changes to 'bar'? [Ynesfdaq?] f
diff --git a/foo b/foo
2 hunks, 2 lines changed
examine changes to 'foo'? [Ynesfdaq?] n
EDITOR: HG: Splitting 904c80b40a4a. Write commit message for the first split changeset.
EDITOR: splitme
EDITOR:
EDITOR:
EDITOR: HG: Enter commit message. Lines beginning with 'HG:' are removed.
EDITOR: HG: Leave message empty to abort commit.
EDITOR: HG: --
EDITOR: HG: user: test
EDITOR: HG: branch 'default'
EDITOR: HG: changed bar
created new head
diff --git a/foo b/foo
2 hunks, 2 lines changed
examine changes to 'foo'? [Ynesfdaq?] f
EDITOR: HG: Splitting 904c80b40a4a. So far it has been split into:
EDITOR: HG: - ffecf40fa954: split 1
EDITOR: HG: Write commit message for the next split changeset.
EDITOR: splitme
EDITOR:
EDITOR:
EDITOR: HG: Enter commit message. Lines beginning with 'HG:' are removed.
EDITOR: HG: Leave message empty to abort commit.
EDITOR: HG: --
EDITOR: HG: user: test
EDITOR: HG: branch 'default'
EDITOR: HG: changed foo
saved backup bundle to $TESTTMP/f/b/.hg/strip-backup/904c80b40a4a-47fb907f-split.hg (obsstore-off !)