This is an archive of the discontinued Mercurial Phabricator instance.

uncommit: move fb-extension to core which uncommits a changeset
ClosedPublic

Authored by pulkit on Aug 27 2017, 4:45 PM.
Tags
None
Subscribers

Details

Summary

uncommit extension in fb-hgext adds a uncommit command which by default
uncommits a changeset and move all the changes to the working directory. If
file names are passed, uncommit moves the changes from those files to the
working directory and left the changeset with remaining committed files.

The uncommit extension in fb-hgext does not creates an empty commit like the one
in evolve extension unless user has specified ui.alllowemptycommit to True.

The test file added is a combination of tests from test-uncommit.t,
test-uncommit-merge.t and test-uncommit-bookmark.t from fb-hgext.

.. feature::

A new uncommit extension which provides `hg uncommit` using which one can
uncommit part or all of the changeset. This command undoes the effect of a
local commit, returning the affected files to their uncommitted state.

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 created this revision.Aug 27 2017, 4:45 PM
pulkit added a comment.Sep 4 2017, 8:32 AM

This has been in from a week, any updates on this?

yuja added a subscriber: yuja.Sep 5 2017, 10:16 AM

Overall, the patch looks good, but I don't have expertise in this area.

The uncommit extension in fb-hgext does not creates an empty commit like the one
in evolve extension unless user has specified ui.alllowemptycommit to True.

Maybe we'll need a command option to keep the empty commit? ui.allowemptycommit
is an internal flag,

hgext/uncommit.py
50

Can't we make this share some bits with cmdutil.amend?

154

Nit: wctx.parents() should return at least one ctx even if it is null
revision.

160

Nit: amend uses old.mutable().

169

Perhaps this should exit with 1 as "hg commit" would do.

174

I'm not sure if this retractboundary() is valid. cmdutil.amend()
appears to use phases.new-commit config instead.

177

Can't we use scmutil.cleanupnodes()?

martinvonz added inline comments.
hgext/uncommit.py
17

How about added files? Also, I think we use "removed" for tracked changes ("deleted" for untracked removal of file).

100

I prefer the form that doesn't depend on the order in the status tuple:

s = repo.status(...)
for f in s.added:
    ...
149

This is inconsistent with the amend extension that Jun added. We can always add support for that later.

tests/test-uncommit.t
23

s/some/part/ (I think)

quark requested changes to this revision.Sep 8 2017, 3:04 PM
quark added a subscriber: quark.

Please address other reviewers' comments.

hgext/uncommit.py
13

I'd add "(EXPERIMENTAL)". The command does not write unamend_source so it's in theory problematic with the current obsstore design.

177

Yep. That will also make _updatebookmarks unnecessary.

This revision now requires changes to proceed.Sep 8 2017, 3:04 PM
pulkit added a comment.Sep 9 2017, 1:23 PM
In D529#10887, @quark wrote:

Please address other reviewers' comments.

Yeah will do that soon, was busy for past few days.

pulkit edited edge metadata.Sep 11 2017, 10:24 AM
pulkit updated this revision to Diff 1712.
durham accepted this revision.Sep 11 2017, 12:23 PM
durham added a subscriber: durham.

Looks good, except for my ui.allowemptycommit comment.

hgext/uncommit.py
60

If I read this right, it means a user having ui.allowemptycommit set to True will cause hg uncommit to always leave a commit behind? I don't think that's what we want. ui.allowemptycommit is to let a user make an empt commit if they want. It's not really meant to say 'leave empty commits behind for me'. I don't think it should affect the uncommit command.

I see this came from our original extension. I'd still drop it.

62

Might be worth a comment that returning p1 here is special cased to not create an obsmarker later.

155

Should "amend" be "uncommit" here?

160

Same here, "amend" -> "uncommit"

pulkit edited the summary of this revision. (Show Details)Sep 11 2017, 4:51 PM
pulkit updated this revision to Diff 1729.
pulkit marked 15 inline comments as done.Sep 11 2017, 4:53 PM
pulkit added inline comments.
hgext/uncommit.py
174

The phases.new-commit config update the phase to old.phase() if --secret flag is not passed. Since we don't have that I think we can use retractboundary to directly update to old.phase()

quark accepted this revision.Sep 11 2017, 6:21 PM

Generally look good to me. I think core hg does not have very convenient commit editing APIs yet (memctx feels too low-level). I'm counting on in-memory-memory work to provide convenient APIs eventually. So I'd hold on refactoring until we have better APIs.

hgext/uncommit.py
174

I think it's still safer to follow common practice and avoid using low-level retractboundary. It seems to be a few lines change around line 85. I guess retractboundary may cause surprises cases when the uncommitted node happens to exist.

pulkit updated this revision to Diff 1823.Sep 14 2017, 2:28 PM
pulkit marked an inline comment as done.Sep 14 2017, 2:29 PM
durin42 accepted this revision.Sep 15 2017, 11:40 AM
This revision is now accepted and ready to land.Sep 15 2017, 11:40 AM
This revision was automatically updated to reflect the committed changes.
durin42 added a subscriber: pulkit.Sep 15 2017, 5:10 PM

We've had some discussion about this on irc. The evolve behavior of preserving empty commits is important in one circumstance: when the commit will briefly be empty, but should retain its identity for the purposes of evolution. Personally, I've never used uncommit and ended up with an empty commit (via the evolve version, which I currently use), so I've got a pretty strong sense that this is an edge case. Here's where we're landing for the time being:

We'll add --keep to uncommit, preserve the current default, but refuse to do no-arguments uncommit if the working directory is dirty. This prevents users from accidentally blowing their foot off in the cases that are the most likely to cause pain. This still leaves some difficulty at the margins (how do you uncommit everything if you've got a dirty working directory?), but we can probably overcome that in time. One thing we hope will help there is a generalized hg undo command, which it sounds like Facebook has at least partially drafted.

Note that this is not necessarily a final state for uncommit: it's possible that after further experimentation we'll flip the default, or otherwise adjust behavior. It's a complicated balancing act between what's intuitive (which we don't all agree on) and preserving the richest metadata possible.

Thanks,
Augie