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

[Thomas Keller]

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