Page MenuHomePhabricator

uncommit: don't allow dirty working copy with PATH (issue5977)
ClosedPublic

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

Details

Summary

On a dirty PATH, uncommit was working without even setting the config
experimental.uncommitondirtydir to True. Ideally, it should abort
as it does for a dirty dir. This patch makes uncommit to require the
config option experimental.uncommitondirtydir on a dirty PATH.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit added a subscriber: pulkit.Feb 14 2019, 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)Feb 14 2019, 11:56 AM
navaneeth.suresh updated this revision to Diff 14096.
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.Feb 15 2019, 12:35 PM
hgext/uncommit.py
143

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)Feb 15 2019, 2:08 PM
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 updated this revision to Diff 14100.
navaneeth.suresh marked 4 inline comments as done.Feb 15 2019, 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
172

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

174

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.Feb 15 2019, 2:12 PM
hgext/uncommit.py
143

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

martinvonz added inline comments.Feb 15 2019, 2:15 PM
hgext/uncommit.py
143

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
143

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?

174

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.Feb 15 2019, 2:39 PM
hgext/uncommit.py
143

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)Feb 16 2019, 12:07 AM
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 updated this revision to Diff 14130.
navaneeth.suresh marked 4 inline comments as done.Feb 16 2019, 12:13 AM
navaneeth.suresh added inline comments.
hgext/uncommit.py
143

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.Feb 16 2019, 12:26 AM
navaneeth.suresh updated this revision to Diff 14131.

This patch changes uncommit behavior to always require experimental.uncommitondirtywdir option if wdir is dirty. However what we want when wdir is dirty is as follows:

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

Do we really want to tell the user to set an experimental option? I prefer exposing it as --allow-dirty-working-copy.

Do we really want to tell the user to set an experimental option? I prefer exposing it as --allow-dirty-working-copy.

Well I am happy with that. We should delete the config option then.

In D5940#87924, @pulkit wrote:

Do we really want to tell the user to set an experimental option? I prefer exposing it as --allow-dirty-working-copy.

Well I am happy with that. We should delete the config option then.

I (and Google in general) have that config option set, so I'd prefer if it wasn't removed. I'm not sure what I think we should do here :(

In D5940#87924, @pulkit wrote:

Do we really want to tell the user to set an experimental option? I prefer exposing it as --allow-dirty-working-copy.

Well I am happy with that. We should delete the config option then.

I (and Google in general) have that config option set, so I'd prefer if it wasn't removed. I'm not sure what I think we should do here :(

Ah, okay. Let's respect both config options and flag.

pulkit added inline comments.Mar 1 2019, 1:42 PM
hgext/uncommit.py
165

how about if not pats or isdirtypath?

navaneeth.suresh marked an inline comment as done.Mar 1 2019, 2:39 PM
navaneeth.suresh added inline comments.
hgext/uncommit.py
165

That's sweet. I've tried that earlier and failed to work because I thought bailifchanged couldn't handle dirty PATH. My bad! Current revision has your suggestion implemented.

I don't understand the commit message. experimental.uncommitondirtydir already works with PATH, doesn't it? I think you mean something like "uncommit: allow dirty working copy with PATH". Did I understand that right?

tests/test-uncommit.t
445

I still don't think it's a good idea to recommend an experimental config option

navaneeth.suresh marked 5 inline comments as done.Mar 2 2019, 12:35 AM

I don't understand the commit message. experimental.uncommitondirtydir already works with PATH, doesn't it? I think you mean something like "uncommit: allow dirty working copy with PATH". Did I understand that right?

If PATH is given, experimental.uncommitondirtydir is ignored. This is the current scenario. I thought the current commit message would go well. But, yours sound sweet to me. Do you want me to replace that?

tests/test-uncommit.t
445

I was going with Yuya's and Pulkit's suggestions. Fixing an existing bug without modifying the current UI sounds reasonable to me. I too agree that recommending an experimental config option quite often is not a good idea. But, we don't want to remove that config option either as you (and Google) are already using that. I'll wait for @pulkit and you. If you are having a strong opinion on this, then I won't do that.

pulkit added a comment.Mar 3 2019, 4:04 PM

I don't understand the commit message. experimental.uncommitondirtydir already works with PATH, doesn't it? I think you mean something like "uncommit: allow dirty working copy with PATH". Did I understand that right?

Right now, if PATH is given, we always allow to uncommit. This patch now looks good to me. I will wait about what you think before pushing it.

tests/test-uncommit.t
445

Once this patch gets in, can you send a follow-up which adds the new flag and suggest that?

In D5940#88276, @pulkit wrote:

I don't understand the commit message. experimental.uncommitondirtydir already works with PATH, doesn't it? I think you mean something like "uncommit: allow dirty working copy with PATH". Did I understand that right?

Right now, if PATH is given, we always allow to uncommit. This patch now looks good to me. I will wait about what you think before pushing it.

Sure, but the commit message makes it sound like we're changing behavior when experimental.uncommitondirtydir is set, which is not correct.

tests/test-uncommit.t
445

Ideally that patch would come first (so we don't have to remember to roll back this patch if that other patch doesn't come before we cut the next release).

tests/test-uncommit.t
445

I'm up for anything. I can send a follow-up suggesting the new flag. Pushing this will fix an existing bug and the follow-up will be on replacing the experimental config option uncommitondirtydir with the flag --allow-dirty-working-copy on working with PATH. But, I once tried that and failed to work. Do flags support hyphen in names? This may sound trivial. But, I need to know this. I'll wait for a confirmation message from you before working on this further.

pulkit added a comment.Mar 4 2019, 6:55 AM

Can you also fix the commit message of the patch?

Once you get the patch for the new flag, and fix commit message for this one, we are good to go.

tests/test-uncommit.t
445

@navaneeth.suresh can you send the patch adding the flag also. You can also send that patch based on this patch. Yes flags do support hypens in names. The hypens are converted to underscores. So you would need to do opts['allow_dirty_working_copy'].

tests/test-uncommit.t
445

Sure. Doing that right away!

navaneeth.suresh retitled this revision from uncommit: make experimental.uncommitondirtydir to work on PATH (issue5977) to uncommit: allow dirty working copy with PATH (issue5977).Mar 4 2019, 9:11 AM
navaneeth.suresh updated this revision to Diff 14342.

The patch fails to apply for me. Can you rebase this on tip of default branch of https://www.mercurial-scm.org/repo/hg-committed?

Also, can you put little more effort in improving the commit message and describing what was the old behavior, what is the new behavior.

navaneeth.suresh edited the summary of this revision. (Show Details)Mar 6 2019, 12:34 AM
navaneeth.suresh updated this revision to Diff 14371.

Can you fix the first line of the commit message to say "don't allow dirty working copy with PATH" (the "don't" was missing). It was allowed before this patch, it is not allowed with this patch (unless the config is set, of course).

navaneeth.suresh retitled this revision from uncommit: allow dirty working copy with PATH (issue5977) to uncommit: don't allow dirty working copy with PATH (issue5977).Mar 6 2019, 12:49 AM

Can you fix the first line of the commit message to say "don't allow dirty working copy with PATH" (the "don't" was missing). It was allowed before this patch, it is not allowed with this patch (unless the config is set, of course).

Fixed. I just copy-pasted the one you suggested before. Hope the current one will work.

pulkit accepted this revision.Mar 6 2019, 8:03 AM
pulkit added a comment.Mar 6 2019, 8:27 AM

Amended the following in flight and resolved conflicts while rebasing the next one.

diff --git a/hgext/uncommit.py b/hgext/uncommit.py
--- a/hgext/uncommit.py
+++ b/hgext/uncommit.py
@@ -160,8 +160,8 @@ def uncommit(ui, repo, *pats, **opts):
 
         m, a, r, d = repo.status()[:4]
         isdirtypath = any(set(m + a + r + d) & set(pats))
-        if not repo.ui.configbool('experimental', 'uncommitondirtywdir') and \
-            (not pats or isdirtypath):
+        if (not repo.ui.configbool('experimental', 'uncommitondirtywdir') and
+            (not pats or isdirtypath)):
             cmdutil.bailifchanged(repo, hint=_('requires '
                                 'experimental.uncommitondirtywdir to uncommit'))
         old = repo['.']
diff --git a/tests/test-uncommit.t b/tests/test-uncommit.t
--- a/tests/test-uncommit.t
+++ b/tests/test-uncommit.t
@@ -480,6 +480,7 @@ experimental.uncommitondirtywdir should 
   $ hg status
   A b
   $ hg unc a
+  note: keeping empty commit
   $ hg unc b
   abort: uncommitted changes
   (requires experimental.uncommitondirtywdir to uncommit)

Next time please run all the tests.

This revision was automatically updated to reflect the committed changes.