darcs

Patch 1182 Implementation of the command 'darcs undo'.

Title Implementation of the command 'darcs undo'.
Superseder Nosy List ganesh, mdiaz
Related Issues
Status in-discussion Assigned To ganesh
Milestone

Created on 2014-07-23.19:23:31 by mdiaz, last changed 2015-02-05.19:46:00 by gh.

Files
File name Status Uploaded Type Edit Remove
implementation-of-the-command-_darcs-undo__.dpatch mdiaz, 2014-07-23.19:23:31 application/x-darcs-patch
implementation-of-the-command-_darcs-undo__.dpatch mdiaz, 2014-07-29.13:33:18 application/x-darcs-patch
implementation-of-the-command-_darcs-undo__.dpatch mdiaz, 2014-07-30.23:50:20 application/x-darcs-patch
implementation-of-the-commands-_undo_-and-_redo__.dpatch mdiaz, 2014-08-18.12:07:39 application/x-darcs-patch
patch-preview.txt mdiaz, 2014-07-23.19:23:31 text/x-darcs-patch
patch-preview.txt mdiaz, 2014-07-29.13:33:18 text/x-darcs-patch
patch-preview.txt mdiaz, 2014-07-30.23:50:20 text/x-darcs-patch
patch-preview.txt mdiaz, 2014-08-18.12:07:39 text/x-darcs-patch
unnamed mdiaz, 2014-07-23.19:23:31
unnamed mdiaz, 2014-07-29.13:33:18
unnamed mdiaz, 2014-07-30.23:50:20
unnamed mdiaz, 2014-08-18.12:07:39
See mailing list archives for discussion on individual patches.
Messages
msg17627 (view) Author: mdiaz Date: 2014-07-23.19:23:31
First implementation of the command 'darcs undo'.

This implementation only makes a copy (named 'previous_hashed_inventory') 
of the 'hashed_inventory' file using the function finalizeRepositoryChanges 
in the module Repository.Internal.

The command 'darcs undo' swap the content of the files 'hashed_inventory' 
and 'previous_hashed_inventory'.

Using this command is possible for example, to undo the last run of 
the command amend-record.

1 patch for repository http://darcs.net/screened:

Wed Jul 23 16:09:16 ART 2014  Marcio Diaz <marcio.diaz@gmail.com>
  * Implementation of the command 'darcs undo'.
Attachments
msg17632 (view) Author: mdiaz Date: 2014-07-29.13:33:18
Changes:
- Now we can undo more than one command (for now only 
supports record and amend).

1 patch for repository http://darcs.net/screened:

patch 6a88f96435197174ae4154cf796fb41db0a5ba8c
Author: Marcio Diaz <marcio.diaz@gmail.com>
Date:   Tue Jul 29 10:29:19 ART 2014
  * Implementation of the command 'darcs undo'.
Attachments
msg17633 (view) Author: ganesh Date: 2014-07-29.20:36:49
Some brief initial comments:

- watch out for spurious whitespace changes

- at some point please merge undoFun into finalizeRepositoryChanges as 
discussed before - it'll mean a tedious initial pass through the 
codebase marking everything as not undoable by default, but it should 
help to shake out any tricky cases and also make sure that nothing is 
missed. It'll also be good for future maintainability as people won't be 
able to forget undo.
msg17634 (view) Author: ganesh Date: 2014-07-29.20:45:31
Another general point: this new feature will require substantial testing. 
You should start on that now so that you build up the tests as you go, as 
it'll be much easier than doing them in one big block at the end. The 
tests will also help reviewers understand what the feature can and can't 
do.
msg17638 (view) Author: mdiaz Date: 2014-07-30.23:50:20
Changes:
- Removed spurious whitespace.
- undoFun renamed to 'handleUndoFiles' and called inside finalizeRepositoryChanges.
- Record, unrecord, amend, tag and rebase suspend are marked as YesUndoable.
  Looks like rebase unsuspend can't be YesUndoable due to a conditional execution of
  finalizeRepositoryChanges in the function checkSuspendedStatus. 

1 patch for repository http://darcs.net/screened:

patch e64d722616d62de9d1cd6b645c02c0d74805744f
Author: Marcio Diaz <marcio.diaz@gmail.com>
Date:   Wed Jul 30 20:39:21 ART 2014
  * Implementation of the command 'darcs undo'.
Attachments
msg17645 (view) Author: ganesh Date: 2014-08-05.18:08:43
Getting there :-)

Did you document the on-disk format yet? Also don't forget about tests.

I think the most important cleanup left is to sort out the description 
of the existing use sites where that's unclear.

One simple change would be to merge the description into the Undoable 
type, i.e.

    data Undoable = YesUndoable String | NoUndoable

there's no need for a description for a non-undoable command (is there?) 
That would eliminate a lot of the existing sites with unclear 
descriptions.

It's fine for "rebase453" ("rebase inject") to be NoUndoable - that's a 
hacky private command.

I think "rebase obliterate" is undoable, did you have a specific reason 
that it isn't?
msg17647 (view) Author: ganesh Date: 2014-08-06.07:40:46
I had a look at the UI. It's a bit confusing, particularly with the 
"Last state" thing. On the other hand, the UI does mean it supports 
"redo" implicitly.

I wonder if it would be better to have a separate "redo" command 
instead, and present the things to undo (and redo) as a more explicit 
stack where the last command is listed first. Perhaps if a single "darcs 
undo" could only undo one operation, the UI would be simpler - it would 
just say "undo record [y/n]?"
msg17655 (view) Author: mdiaz Date: 2014-08-18.12:07:39
Taking into account your suggestions I implemented two 
separate commands: 'undo' and 'redo'.
Both undo or redo just one operation.
I think you were right, it seems much simpler now.

For now, as indicated by the test, its only enabled for
record, unrecord, amend and obliterate.

For the next version I'll probably move handleUndoFiles
out of the function finalizeRepositoryChanges, 
since the revert command doesn't call this function.

Now we are going to support much more commands, 
so I wonder what will be the protocol (if any) 
to clear the saved states.

2 patches for repository http://darcs.net/screened:

patch c9b040dd1a6d7d0de140beb23f59b9440f00cab5
Author: Marcio Diaz <marcio.diaz@gmail.com>
Date:   Mon Aug 18 00:10:31 ART 2014
  * Implementation of the commands 'undo' and 'redo'.

patch b6f633d4c9806861a4e5a3f12a064ae10d652a4f
Author: Marcio Diaz <marcio.diaz@gmail.com>
Date:   Mon Aug 18 00:11:35 ART 2014
  * Test for undo and redo commands.
Attachments
msg17657 (view) Author: ganesh Date: 2014-08-19.22:00:48
I don't think we need to clear out the saved states explicitly, only when 
something invalidates them.

The user doesn't need to use 'darcs undo' if she doesn't want to, so I 
don't think they get in the way.
msg18019 (view) Author: bf Date: 2015-02-05.15:42:43
Hm, last activity 6 months ago. Looks like you have sorted out most of
the initial objections, so shouldn't this go into screened? If you don't
have the time I can try to rebase it (guessing that this will be
necessary). There is no milestone yet, but (again guessing) you haven't
planned for this to go into 2.10, or have you?
msg18035 (view) Author: gh Date: 2015-02-05.19:45:59
I don't think we should take the risk to have this into 2.10 since it is
a new feature, its specification is still not clear, and its author is
no longer around.
History
Date User Action Args
2014-07-23 19:23:31mdiazcreate
2014-07-29 13:33:18mdiazsetfiles: + patch-preview.txt, implementation-of-the-command-_darcs-undo__.dpatch, unnamed
messages: + msg17632
2014-07-29 20:36:50ganeshsetstatus: needs-screening -> in-discussion
messages: + msg17633
2014-07-29 20:45:31ganeshsetmessages: + msg17634
2014-07-29 20:45:34ganeshsetassignedto: ganesh
nosy: + ganesh
2014-07-30 23:50:20mdiazsetfiles: + patch-preview.txt, implementation-of-the-command-_darcs-undo__.dpatch, unnamed
messages: + msg17638
2014-08-05 18:08:44ganeshsetmessages: + msg17645
2014-08-06 07:40:46ganeshsetmessages: + msg17647
2014-08-18 12:07:40mdiazsetfiles: + patch-preview.txt, implementation-of-the-commands-_undo_-and-_redo__.dpatch, unnamed
messages: + msg17655
2014-08-19 22:00:48ganeshsetmessages: + msg17657
2015-02-05 15:42:44bfsetmessages: + msg18019
2015-02-05 19:46:00ghsetmessages: + msg18035