Issue 2394 optimize and convert options are actually sub-commands

Title optimize and convert options are actually sub-commands
Priority Status resolved
Milestone Resolved in 2.10.0
Superseder Nosy List bfrk
Assigned To

Created on 2014-05-29.12:38:08 by bfrk, last changed 2014-06-25.00:06:20 by noreply.

msg17482 (view) Author: bfrk Date: 2014-05-29.12:38:06
1. Summarise the issue (what were doing, what went wrong?)

Both convert and optimize commands make no clear distinction between
their sub-commands and options. Here, I understand an option as some
switch or combination of switches that modify the standard behavior,
while a sub-command is a switch (or combination of switches) that
selects an action from some finite choice.

Looking at the code reveals that e.g. --pristine, --patch-index, and
--disable-patch-index switches for the optimize command are actually
treated like sub-commands, not as options. That is, giving one of these
options makes the code branch off to a specific subroutine that does
exactly this action, which is different from what is done when none of
these options are given.

Similarly, --import, --export, --darcs-1, and --darcs-2 act as
sub-commands of convert (the last being the default).

In both cases, conflicting sub-commands are resolved "by accident", i.e.
the implementing code arbitrarily chooses one of them of the other.

2. What behaviour were you expecting instead?

Ideally, these two command would become super commands. A more
compatible solution would be to at least make their status clear in the
docs, provide a separate data type OptimizeAction resp. ConvertAction,
and a getter function [DarcsFlag] -> XyzAction.

It is important that conflicting commands or options result in an error
message. Otherwise the user is left guessing what happened in such a case.
msg17484 (view) Author: gh Date: 2014-05-29.17:05:46
Yes, that's an interesting observation.  In the case of optimize,
careful that there is a default optimize that runs other optimize
"subcommands", and I'd rather have darcs keeping of providing that
"default optimize". In the case of convert it's clearer to me that we
should separate the subcommands.
msg17485 (view) Author: bfrk Date: 2014-05-29.20:43:31
Yes, that's right, there is a default action. There are also real
options: --upgrade does something in addition to the default or
sub-commands, likewise --http. It is only --pristine, --patch-index, and
--disable-patch-index, that are mutually exclusive, as well as
--compress and --uncompress. Furthermore, the first three sub-commands
do respect --upgrade and --http, but do *not* respect --reorder,
--compress, --uncompress, or --relink. I have no idea why this is so. 

The current behavior can be mapped to a command tree like this:

  options: --upgrade, --http, --reorder, --relink
      options: --upgrade, --http
      options: --upgrade, --http, --reorder, --relink

This strikes me as much more complicated than necessary. A much simpler
solution would be to allow mixing everything as much as possible.
Particularly, each option will do something in addition to the default
and the other options. There would be only two mutually exclusive cases:
--patch-index vs. --no-patch-index, and --compress vs. --uncompress.
Both are easily recognized as opposite option pairs.

This will make both the UI and the implementation much clearer. I think
I'll send a patch for the details rather than paste the code here.
msg17496 (view) Author: bfrk Date: 2014-05-30.11:53:26
I changed the title of this issue to reflect my conclusion on this issue
and to make clear that it no longer covers the convert command.
msg17508 (view) Author: gh Date: 2014-06-02.18:57:32
Now that I'm looking again at thiks, I had a change of heart, sorry  :-)

I've been doing time profiling of `optimize --reorder` and I find it a
shame that it spends most of the time cleaning again pristine and
inventories, while there's no need for it.
Also (for Ale's GSoC project) I'm interested in the running time of
reorder without having to take repo garbage collection into account.

So I think it should all be subcommands. I'd suggest the following:

* optimize clean  (our current default one)
* optimize upgrade (OF to hashed)
* optimize http (does clean at the beginning but nevermind)
* optimize enable-patch-index
* optimize disable-patch-index
* optimize compress
* optimize uncompress
* optimize reorder

And we may see in the near future:

* optimize cache (garbage collection of global cache)

At the code level I'd suggest keeping all of this in the same module.
msg17518 (view) Author: bfrk Date: 2014-06-03.22:23:01
Please re-consider this, see my message to the mailing list.
msg17576 (view) Author: noreply Date: 2014-06-25.00:06:19
The following patch sent by Guillaume Hoffmann <guillaumh@gmail.com> updated issue issue2394 with
status=resolved;resolvedin=2.10.0 HEAD

* resolve issue2394: make optimize a supercommand 
Ignore-this: 841fbf0c5e0017eaff8b87b75ad80a37
Date User Action Args
2014-05-29 12:38:08bfrkcreate
2014-05-29 17:05:47ghsetmessages: + msg17484
2014-05-29 20:43:32bfrksetmessages: + msg17485
2014-05-30 11:53:28bfrksetmessages: + msg17496
title: optimize and convert options are actually sub-commands -> allow maximal mixing of optimize actions
2014-06-02 18:57:33ghsetmessages: + msg17508
title: allow maximal mixing of optimize actions -> optimize and convert options are actually sub-commands
2014-06-03 22:23:02bfrksetmessages: + msg17518
2014-06-25 00:06:20noreplysetstatus: unknown -> resolved
messages: + msg17576
resolvedin: 2.10.0