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

[Tero Koskinen]

Hi,

On Wed, 21 Apr 2010 01:20:15 +0200 Thomas Keller wrote:

> 
> Hi Tero!
> 
> > I have been working on two quickie tasks, two-argument disapprove
> > 
> > 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.

I don't have write access to monotone.ca server, so I cannot push my branch
there, but pulling stronglytyped.org shouldn't be a problem.

> Very cool, code looks good and clean. Some minor obvervations:
> 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".

I changed the code to use "parents" in the messages.

>    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.

>    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.

r and r2 are now renamed to "parent" and "child". Parent is assumed
to be ancestor of child.

> Thanks for your work,
> Thomas.

-- 
Tero Koskinen <address@hidden>