Re: [Monotone-devel] WIP: multirevision disapprove and clean workspace features

[Thomas Keller]

Hi Tero!

> I have been working on two quickie tasks, two-argument disapprove
> and "clean"/"purge" command which removes "extra" files from the
> workspace. They are not completely ready yet (tests+docs missing),
> but just in case someone is interested, I have made them available
> on my Monotone server at
>   stronglytyped.org
> 
> At the moment the server carries three branches:
> net.venge.monotone
> net.venge.monotone.tkoskine.disapprove-multirev
> net.venge.monotone.tkoskine.purge

Very cool, code looks good and clean. Some minor obvervations:

1) nvm.tkoskine.purge:

   We're usually very careful before nuking files in the user's
   workspace and this command does all of its beauty without any
   "do you really want to do that?!" checking. Maybe it would be
   a good idea to implement that and only act on the common global
   --non-interactive option automatically?

   As a bonus point this could lead to some generic input prompting
   code which could be used in other areas as well and replace
   hackish implementations there (I particularily remember our
   key password prompt here...), i.e. something like this (in
   pseudo code):

   enum { yes, no, undefined } bool_answer;
   user_prompt::ask_yes_no(const string & question, bool_answer & yes)
   {
       string answer;
       bool answer_given;
       string q = F("%s (y|n) ") % question;
       ask(q, "/^(y|Y|n|N)$/$", 3, answer, answer_given);
       bool_answer = answer_given
                ? (answer == "y" || answer == "Y" ? yes : no )
                : undefined;
   }

   user_prompt::ask(const string & question,
                    const string & pattern,
                    int tries,
                    string & answer,
                    bool & answer_given)
   {
       while (...)
       {
           // output the question
           // read input and check against pattern
           // break if the output matches the pattern,
           // set answer_given to true, otherwise to false
       }
   }

2) nvm.tkoskine.disapprove-multirev:

   I know the term "changeset" has been used here before, but as
   far as I know this is the only place in the complete source tree
   and I'd love to see some unification towards "parents", i.e.
   "revision %s has %d parents, cannot invert".

   The code which checks if the two given revisions have no merges
   and share a common ancestor at all could be improved a bit. If
   I trigger it with two distinct revisions from two trees, it
   bails out early if it finds the first merge revision from the
   start revision's tree, however a much more useful error could
   be that both revisions don't share a common ancestor and the
   easiest way to determine that is to run erase_ancestors() on
   them. If the result is 2, they're two different trees, if its
   one, one of them is an ancestor of the other and 0 should fire
   an invariant, I guess. Then run walk_revisions and only check
   for merge revisions which you explicitely want to exclude.

   Finally I'd give r and r2 more sounding names, to make it really
   clear what the start and what the end revision in the range of
   revisions is.

Thanks for your work,
Thomas.

-- 
GPG-Key 0x160D1092 | address@hidden | http://thomaskeller.biz
Please note that according to the EU law on data retention, information
on every electronic information exchange might be retained for a period
of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en

signature.asc
Description: OpenPGP digital signature