This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

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

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

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.

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.

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

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