uncommit: make experimental.uncommitondirtydir to work on PATH (issue5977)
Needs ReviewPublic

Authored by navaneeth.suresh on Mon, Feb 11, 2:01 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

The behaviour of uncommit may confuse a new user. Although it never
destroys the data, it can hide instead.
experimental.uncommitondirtydir should be set true to uncommit
on dirty PATH as for dirty working directory.

Original patch to evolve extension authored by Dan Villiom Podlaski Christiansen:
https://bitbucket.org/octobus/evolve-devel/pull-requests/8/bug-5977-uncommit-dirtiness/diff

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped
pulkit added a subscriber: pulkit.Thu, Feb 14, 8:38 AM

Hi, it will be good if you specify that the patch is authored by someone else, mention their name and also provide the link to the original PR of this patch.

In D5940#87026, @pulkit wrote:

Hi, it will be good if you specify that the patch is authored by someone else, mention their name and also provide the link to the original PR of this patch.

@pulkit the link of WIP patch to evolve extension's uncommit is given in the bugzilla page itself. Link to the patch is https://bitbucket.org/octobus/evolve-devel/pull-requests/8/bug-5977-uncommit-dirtiness/diff which is authored by Dan Villiom Podlaski Christiansen.

In D5940#87026, @pulkit wrote:

Hi, it will be good if you specify that the patch is authored by someone else, mention their name and also provide the link to the original PR of this patch.

@pulkit the link of WIP patch to evolve extension's uncommit is given in the bugzilla page itself. Link to the patch is https://bitbucket.org/octobus/evolve-devel/pull-requests/8/bug-5977-uncommit-dirtiness/diff which is authored by Dan Villiom Podlaski Christiansen.

Yep, that's how I figured it out. ;)

It's always good to include that information in commit message. Commit messages should try to explain all the things related to patch and should contain all the related links.

navaneeth.suresh edited the summary of this revision. (Show Details)
martinvonz added inline comments.
tests/test-uncommit.t
177–178

Why did this change?

I don't understand what the new flag is for since we already how the config option. They seem to me to do the same thing.

tests/test-uncommit.t
177–178

I'm also surprised at the change in that output. Do you recommend removing that config option if that doesn't do anything new?

martinvonz added inline comments.Fri, Feb 15, 12:35 PM
hgext/uncommit.py
140

Could you rename --force to e.g. --allow-dirty-working-copy so it's clearer what's being allowed?

tests/test-uncommit.t
177–178

I like that config option and I don't want to lose it. I was even hoping it would be on by default some day.

IIUC, the new behavior is to require --force if there are uncommitted changes, whether you're uncommitting one file or all. I was actually surprised that it used to be allowed to uncommit specific files on a dirty working copy. I like the new consistency more.

I think the --force option should be equivalent to the existing config. How about we just add and not opts.get('force') to the existing condition on line 159? I'd also prefer to drop the not pats from that condition, but that can (and should) be in a separate patch.

navaneeth.suresh edited the summary of this revision. (Show Details)
navaneeth.suresh retitled this revision from uncommit: add -f/--force when possibly hiding data (issue5977) to uncommit: add --allowdirtywcopy when possibly hiding data (issue5977).
navaneeth.suresh marked 4 inline comments as done.Fri, Feb 15, 2:11 PM

Your current patch is doing more than one thing. Let's split this up and make one patch do one thing.

hgext/uncommit.py
166

this merits a different patch and tests of it's own.

168

this should also consider checking the config experimental.uncommitondirtywdir.

mercurial/rewriteutil.py
43 ↗(On Diff #14096)

I am not sure why this is a part of patch. Can you explain?

pulkit added inline comments.Fri, Feb 15, 2:12 PM
hgext/uncommit.py
140

we have a config for that already, let's prevent to introduce flags for config options and vice versa here.

martinvonz added inline comments.Fri, Feb 15, 2:15 PM
hgext/uncommit.py
140

Doesn't that mean the whole patch is unnecessary? It pretty much just provides an easier way of saying --config experimental.uncommitondirtywdir=yes, doesn't it?

hgext/uncommit.py
140

I tried with --allow-dirty-working-copy but, it failed to work. I'm also not convinced by the patch now. How do you want me to proceed?

168

will correct that.

mercurial/rewriteutil.py
43 ↗(On Diff #14096)

I imported those from the original PR to the evolve extension version of uncommit. IIUC, this will remove the check in uncommit.py and let it do here.

pulkit added inline comments.Fri, Feb 15, 2:39 PM
hgext/uncommit.py
140

Nice point. I had to revisit the bug to understand more now. IMO the bug says as following:

uncommit should require a flag or config when it is trying to do the following:

  1. uncommit some files which are dirty in wdir, and
  2. the files are passed as hg uncommit PATH

(refer Yuya's comment: https://bz.mercurial-scm.org/show_bug.cgi?id=5977#c1)

However the bug was filed for evolve uncommit, it's applicable to core one also.

The core uncommit allows you to do hg uncommit PATH on a dirty wdir even if experimental.uncommitondirtywdir is set. We should change that and let hg uncommit PATH work only if PATH are not dirty in wdir. If they are dirty, use experimental.uncommitondirtywdir.

The end result on a dirty wdir should look like:

hg uncommit : abort and require experimental.uncommitondirtywdir
hg uncommit PATH: works if PATH is clean in wdir, aborts if PATH is dirty and require experimental.uncommitondirtywdir

How does that sound?

(Ignore my earlier comments as I didn't figured this out completely then)

(P.S. I hate to say that we have two uncommits which are kind of diverging)

navaneeth.suresh edited the summary of this revision. (Show Details)
navaneeth.suresh retitled this revision from uncommit: add --allowdirtywcopy when possibly hiding data (issue5977) to uncommit: make experimental.uncommitondirtydir to work on PATH (issue5977).
navaneeth.suresh marked 4 inline comments as done.Sat, Feb 16, 12:13 AM
navaneeth.suresh added inline comments.
hgext/uncommit.py
140

I quite like the idea. This should be a quick fix. I've updated the revision and edited the summary. Please see if it works as expected. In this revision, I'm doing only one thing. For the case of an outstanding uncommitted merge, I will be sending another patch as a follow-up.

navaneeth.suresh marked an inline comment as done.