SV: [PATCH] convert: added cvsnt mergepoint support

[Henrik Stuart]

> From: gerg.ward at gmail.com [mailto:gerg.ward at gmail.com] on behalf of
> Greg Ward
> Sent: 1. april 2009 22:21
> To: Henrik Stuart
> Cc: Mercurial Devel
> Subject: Re: [PATCH] convert: added cvsnt mergepoint support
> 
> Bookkeeping comments:

Thank you for the comments.

> 1) Please don't attach patches; put them inline.  It's harder to
> review/comment on attachments.  "hg email" is good.

I'll keep that in mind.

> 2) Tests?  (Difficult because you can't assume whatever "cvs" is found
> in the path is CVSNT.
> Is it possible to create a test CVS repo that will include
> "mergepoint" in "cvs rlog" output no matter what CVS you're running?
> Or would it be possible to include "cvs rlog" output and somehow get
> cvsps to parse that?)

It is, unfortunately, not possible to use the ,v files in a 
non-mergepoint aware CVS implementation and still get the mergepoint
information in ,v files.

Adding it as a separate unit test-like thing is a tad difficult at best,
since all the parsing is happening in cvsps.py:createlog, which also
invokes "cvs rlog". As a slightly hacked possibility, it would be
possible to place a bogus cvs command in the path and just let it
return a pre-defined rlog. I'm not sure how nice a solution this is
either.

> Now for the real review:
> 
>      tags = {}     # dictionary of revisions on current file with their
> tags
> +    branchmap = {}
>      state = 0
> 
> A comment explaining what branchmap maps would be nice.

I'll add that.

> +            if match.group(7): # cvsnt mergepoint
> +              e.mergepoint = match.group(8)
> 
> You're adding a new instance attr to logentry, but you didn't document
> it.

I'll add that.

> Also, you've suddenly jumped to 2-space indent.  Boo, hiss.  4-space
> indent is the One True Python style.  Luckily Mercurial uses it.  ;-)

My bad. I'm well aware of PEP-8, just forgot to update my vim settings
from our internal standards, sorry.

> +                e.mergepoint = myrev
> +                branches = [b for b in branchmap if branchmap[b] ==
> myrev]
> +                assert len(branches) == 1, 'unknown branch: %s' %
> e.mergepoint
> +                e.mergepoint = branches[0]
> 
> Why assign a numeric revnum to mergepoint, only to replace it with the
> branch name a millisecond later?

I'll remove that.

>              c = changeset(comment=e.comment, author=e.author,
> -                          branch=e.branch, date=e.date, entries=[])
> +                          branch=e.branch, date=e.date, entries=[],
> +                          mergepoint=e.mergepoint)
> 
> Again, new instance attrs should be documented in the class.

I'll add that.

>          if mergefrom:
> -            m = mergefrom.search(c.comment)
> -            if m:
> -                m = m.group(1)
> -                if m == 'HEAD':
> -                    m = None
> -                if m in branches and c.branch != m:
> -                    c.parents.append(changesets[branches[m]])
> +            if c.mergepoint:
> +              if c.mergepoint == 'HEAD':
> +                  c.mergepoint = None
> +              c.parents.append(changesets[branches[c.mergepoint]])
> +            else:
> +                m = mergefrom.search(c.comment)
> +                if m:
> +                    m = m.group(1)
> +                    if m == 'HEAD':
> +                        m = None
> +                    if m in branches and c.branch != m:
> +                        c.parents.append(changesets[branches[m]])
> 
> Why is your new code conditional on 'mergefrom'?  IOW, why should I
> have to ensure that 'mergefrom' is set to benefit from detecting CVSNT
> mergepoints?  If you want your new feature to be configurable (maybe a
> good idea), IMHO you should add a new config setting rather than
> overloading mergefrom.

I'll rectify that and send an updated patch later today (using hg email). 
Thanks.

-- 
Kind regards,
Henrik Stuart