Page MenuHomePhabricator

unamend: move fb extension unamend to core

Authored by pulkit on Sep 26 2017, 4:46 PM.



unamend extension adds an unamend command which undoes the effect of the amend
command. This patch moves the unamend command from that extension to uncommit
extension and this one does not completely undoes the effect of amend command as
it creates a new commit, rather than reviving the old one back.

This also adds tests for the same.

.. feature::

A new unamend command in uncommit extension which undoes the effect of the
amend command by creating a new changeset which was there before amend and
moving the changes that were amended to the working directory.

Diff Detail

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Sep 26 2017, 4:46 PM
durham added a subscriber: durham.Sep 27 2017, 5:23 AM
durham added inline comments.

This should be in a with statement probably? Can we just have it be as part of the top level lock with statement?

durham requested changes to this revision.Sep 27 2017, 10:27 AM

Generally looks good. Just need to fix the transaction thing.

This revision now requires changes to proceed.Sep 27 2017, 10:27 AM
pulkit updated this revision to Diff 2128.Sep 27 2017, 1:32 PM
singhsrb added a subscriber: singhsrb.EditedSep 28 2017, 2:28 PM

I just wanted to mention that unamend command does not track copytrace information properly. For example, if we do:

  1. touch dummy
  2. hg commit -Aqm 'dummy'
  3. hg cp dummy dummy2
  4. hg commit -Aqm 'dummy2'
  5. hg cp dummy2 dummy3
  6. hg amend
  7. hg log -f dummy3
  8. hg unamend
  9. hg amend
  10. hg log -f dummy3

Notice the difference in output on step 10 and 7. While 7 correctly records the copy information indicating that dummy3 was copied from dummy, 10 considers dummy3 a new file.

We may want to fix this before taking unamend to core because the corresponding amend command does handle copytracing properly.

pulkit planned changes to this revision.Sep 29 2017, 9:20 AM

I just wanted to mention that unamend command does not track copytrace information properly.

That's a pretty good catch. Thanks a lot.

ryanmce added inline comments.

Test test

ryanmce added inline comments.Oct 11 2017, 10:46 AM

Tested nested comments, sorry for the spam.

pulkit added a comment.Nov 2 2017, 2:15 PM

This is lying here from a long time. It will be good some feedback is given. :)

durham accepted this revision.Nov 10 2017, 6:45 PM

Overall looks good to me. My one comment is probably not enough to block this going in.


Might be worth doing the predecessor check in the lock as well, since the result of this verification could technically change between now and when the lock is taken.

pulkit updated this revision to Diff 3421.Nov 11 2017, 9:11 AM
durin42 added inline comments.

undo the _most recent_ amend? or can I run this iteratively and undo many amends in sequence?


Should we also look for unamend_source in the extra, and potentially refuse to unamend an unamend? Or not?


cannot unamend a changeset with children

pulkit added a subscriber: quark.Nov 17 2017, 9:14 AM
pulkit added inline comments.

Nice catch, it should be undo the _most recent_ amend. Thanks!


I don't think users will run unamend(1) and then will try to run unamend(2) again to unamend the first unamend. It's not intuitive in my opinion. @durham @quark what do you guys think here?

pulkit added inline comments.Nov 17 2017, 3:06 PM

Okay while trying to add this condition, I found we cannot refuse to unamend a changeset on the basis of unamend_source, for e.g
a -amend-> b -unamend-> a' -amend-> c -unamend-> a''

But if we refuse on basis on unamend_source, unamend c will refuse. We need to be more smart here but when I think the other way around, I think it's okay to unamend an unamend. I am open to suggestion and will go with what you guys prefer is good.

durham added inline comments.Nov 20 2017, 1:16 PM

I'd just let unamend undo an unamend. Letting unamend toggle back and forth between the two states seems like it might grant the user more confidence in the command, even.

durin42 added inline comments.Nov 20 2017, 1:42 PM

Works for me. Add some documentation/testing of this so we know it's intentional in the future?

pulkit updated this revision to Diff 3671.Nov 20 2017, 3:04 PM

Any updates on this?

durham accepted this revision.Nov 27 2017, 3:59 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Dec 2 2017, 8:39 AM

Can you send a follow-up to fix these issues?


Probably better to query curctx after locks are taken.


This should be curctx.hex(). It's highly discouraged to store blob in extras.


Mostly unused variables?