darcs

Patch 536 Maybefy arguments to rollback (and 5 more)

Title Maybefy arguments to rollback (and 5 more)
Superseder Nosy List exlevan
Related Issues
Status accepted Assigned To
Milestone

Created on 2011-01-27.00:00:26 by exlevan, last changed 2011-02-05.19:09:35 by gh.

Files
File name Status Uploaded Type Edit Remove
maybefy-arguments-to-rollback.dpatch exlevan, 2011-01-27.00:00:25 application/x-darcs-patch
unnamed exlevan, 2011-01-27.00:00:25 text/x-darcs-patch
unnamed exlevan, 2011-01-27.00:00:25
See mailing list archives for discussion on individual patches.
Messages
msg13592 (view) Author: exlevan Date: 2011-01-27.00:00:25
6 patches for repository http://darcs.net/screened:

Wed Jan 26 18:52:29 EET 2011  Alexey Levan <exlevan@gmail.com>
  * Maybefy arguments to rollback

Wed Jan 26 21:27:59 EET 2011  Alexey Levan <exlevan@gmail.com>
  * Maybefy arguments to record

Wed Jan 26 22:56:24 EET 2011  Alexey Levan <exlevan@gmail.com>
  * Maybefy arguments to whatsnew

Wed Jan 26 23:22:27 EET 2011  Alexey Levan <exlevan@gmail.com>
  * Split Darcs.Commands.WhatsNew.announceFiles

Thu Jan 27 01:22:25 EET 2011  Alexey Levan <exlevan@gmail.com>
  * Don't treat empty lists as special; use Maybe instead

Thu Jan 27 01:26:54 EET 2011  Alexey Levan <exlevan@gmail.com>
  * Remove unused areFileArgs
Attachments
msg13593 (view) Author: gh Date: 2011-01-27.11:47:51
Could you give a little explanation on why this bundle is interesting?
msg13595 (view) Author: exlevan Date: 2011-01-27.12:31:25
2011/1/27 gh <bugs@darcs.net>:
>
> gh <guillaumh@gmail.com> added the comment:
>
> Could you give a little explanation on why this bundle is interesting?

This is a refactoring which continues work started with patch178 on
lifting to the type level the differences between commands invoked
with no file arguments and with some arguments which may be invalid.
Typically, in absence of file args means that command applies to all
files.  This behaviour was implemented by pattern-matching on
aguments, and empty list was given a special meaning. In some cases,
however, this led to buggy behaviour, because after arg list was
checked for invalid entries and filtered accordingly, matching on it
could change the meaning of command in border cases.

patch178 fixes this problem by wrapping the arg list in the Maybe
datatype, so now the meaning of command is not affected by operations
on arguments.  Nothing means that command applies to all files, Just
fs means that "apply only to fs files", and Just [] means that
something is wrong.  (For the lack of better word, I called this
change "maybefy".)  In this patches, this change was applied to more
commands, not only those that caused bugs, and a couple of functions
from Darcs.Repository.State, which, too, gave special meaning to the
empty lists, were changed accordingly.

The only places that left keeping the old behaviour are the functions
in Darcs.SelectChanges.  Some of them have no meaning when applied the
empty list arguments, while others can be changed to support the
maybefied lists. For now I left their arguments wrapped in (fromMaybe
[]), and, if thiese pathes are applied, I'll change them, too.

In general, these changes better separate the meaning of commands wrt
the arguments they were called with, and will help to avoid the bugs
mixing up those meanings in future.
msg13608 (view) Author: gh Date: 2011-02-01.16:13:15
Thanks for the explanation Alexey. Also looking at the previous bundle
review at http://bugs.darcs.net/patch178 was helpful to me.

The weakness in my review is that I don't know the code enough to tell
where the conversion to this new semantics has to be done, but since
you mention you're going to write a followup bundle, I'm trusting you
on that.


Also Alexey, could you could move the functions announceFiles and
filterExistingFiles outside of Darcs.Command.Whatsnew in your next
patch bundle? It does not look great to have modules like
Darcs.Command.Revert and Darcs.Command.AmendRecord that import
Darcs.Command.Whatnew just for that.

review below:

2011/1/27 Alexey Levan <bugs@darcs.net>:
>
> New submission from Alexey Levan <exlevan@gmail.com>:
>
> 6 patches for repository http://darcs.net/screened:
>
> Wed Jan 26 18:52:29 EET 2011  Alexey Levan <exlevan@gmail.com>
>  * Maybefy arguments to rollback
>
> Wed Jan 26 21:27:59 EET 2011  Alexey Levan <exlevan@gmail.com>
>  * Maybefy arguments to record
>
> Wed Jan 26 22:56:24 EET 2011  Alexey Levan <exlevan@gmail.com>
>  * Maybefy arguments to whatsnew

The 3 patches above look correct and are very similar one another:
they "convert" an empty list of paths into Nothing, which means "apply
this command to no specific file".

> Wed Jan 26 23:22:27 EET 2011  Alexey Levan <exlevan@gmail.com>
>  * Split Darcs.Commands.WhatsNew.announceFiles

Before the above patch, announceFiles was doing 2 things: printing a
list of files on the terminal and returning the list of correct paths
from the given list. Now the behaviour is separated in two functions,
it's better.

>
> Thu Jan 27 01:22:25 EET 2011  Alexey Levan <exlevan@gmail.com>
>  * Don't treat empty lists as special; use Maybe instead

OK essentially the above pathc is about making the functions
unrecordedChanges and readUnrecorded (from Darcs.Repository.State) use
that new "maybe semantics". So the patch name is not very clear, but
well.

>
> Thu Jan 27 01:26:54 EET 2011  Alexey Levan <exlevan@gmail.com>
>  * Remove unused areFileArgs

OK.


I have pushed the bundle to screened but not to darcs.net yet due to
dependencies on other patches not yet in darcs.net .

Guillaume
msg13637 (view) Author: gh Date: 2011-02-05.19:09:35
Pushed to darcs.net since dependencies are now in.
History
Date User Action Args
2011-01-27 00:00:26exlevancreate
2011-01-27 00:00:36exlevansetstatus: needs-review -> needs-screening
2011-01-27 11:47:51ghsetmessages: + msg13593
2011-01-27 12:31:26exlevansetmessages: + msg13595
2011-02-01 16:13:16ghsetmessages: + msg13608
2011-02-01 16:13:44ghsetstatus: needs-screening -> accepted
2011-02-05 19:09:35ghsetmessages: + msg13637