[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: |
Tue, 29 Jun 2010 00:16:57 +0200 |
User-agent: |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9.1.10) Gecko/20100512 Thunderbird/3.0.5 |
Am 28.06.10 22:45, schrieb Tero Koskinen:
>>> At the moment the server carries three branches:
>>> net.venge.monotone.tkoskine.disapprove-multirev
>
> I updated nvm.tkoskine.disapprove-multirev branch at stronglytyped.org
> server. The branch now contains code, some tests, and a small documentation
> change.
The docs now say
> address@hidden mtn disapprove @var{id}
> address@hidden mtn disapprove address@hidden @var{child}
while the command says
> Syntax specific to 'mtn disapprove':
>
> disapprove REVISION [REVISION]
Could you please adapt one of the two (the latter might be the better
choice) and / or alternativly mention in the command's description which
of the two arguments is the parent and which is the child?
> +check(get("goodfile", "testfile"))
> +commit()
> [...]
> +check(get("badfile", "testfile"))
A small tip: Instead of pulling predefined files with dummy contents
from the test directory, it might be easier to just use
addfile("goodfile", "dummy content")
which creates a new file "goodfile" with the contents "dummy content"
and mtn add's it.
>> Very cool, code looks good and clean. Some minor obvervations:
>> [...]
>> 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.
>
> I am not totally sure did I got this right. I use erase_ancestors()
> to determine early if there are no common ancestors. However, I still
> do pretty much checking in the walk_revisions function also.
Maybe I'm just a little picky, but my original "miuse case" was:
$ mtn disapprove h:BRANCH1 h:BRANCH2
which - after resolving the heads for both branches - errors out with
mtn: misuse: revision REV1 is not a child of REV2, cannot invert
whereas I imagined a message like
mtn: misuse: the revision REV1 and REV2 do not share common history
could be more helpful. But again, maybe I'm just picky here. It should
only be a string change though...
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