| pulkit |
| hg-reviewers |
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
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
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.
| 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? | |
| 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. | |
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? |
| hgext/uncommit.py | ||
|---|---|---|
| 143 | we have a config for that already, let's prevent to introduce flags for config options and vice versa here. | |
| 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. |
| 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:
(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 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) | |
| 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. | |
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.
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 :(
| hgext/uncommit.py | ||
|---|---|---|
| 165 | how about if not pats or isdirtypath? | |
| 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 | |
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. | |
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? | |
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. | |
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! | |
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.
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.
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.
Could you rename --force to e.g. --allow-dirty-working-copy so it's clearer what's being allowed?