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 | tommyd3...@jabber.ccc.de | 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
_______________________________________________ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel