[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Monotone-devel] WIP: multirevision disapprove and clean workspace f
From: |
Thomas Keller |
Subject: |
Re: [Monotone-devel] WIP: multirevision disapprove and clean workspace features |
Date: |
Wed, 21 Apr 2010 01:20:15 +0200 |
User-agent: |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b2pre Thunderbird/3.0.4 |
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