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.

Download Raw Diff

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

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

spectral created this revision.Jan 29 2019, 9:31 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJan 29 2019, 9:31 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

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.

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.

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.

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

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 mentioned this in D5788: commit: make `commit -i` respect whitespace CLI args, ignore config.Jan 31 2019, 6:49 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).

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.

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.

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.

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.

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 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. :/

> 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

			Path	Packages
M			mercurial/cmdutil.py (4 lines)
M			tests/test-commit-interactive.t (44 lines)
M			tests/test-split.t (108 lines)

Commit	Parents	Author	Summary	Date
		Kyle Lippincott		Jan 28 2019, 9:05 PM

	Status	Author	Revision
	Closed	spectral	D5744 commit: ignore diff whitespace settings when doing `commit -i` (issue5839)
	Closed	spectral	D5743 patch: handle 0 context lines (diff.unified=0) when parsing patches

Diff 13567

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

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	13567		Jan 29 2019, 9:31 PM	★	★
Diff 2	13572	rHG19486fc49020444d81cdb39911000307eaa20dd5	Jan 28 2019, 9:05 PM	★	★