Re: Improvement on newphrase handling in cvs2svn_rcsparse/common.py ?

On 04/07/2011 01:36 PM, Giovanni Pellicciotta wrote:
> I'm not sure whether you consider this a bug or not, but I encountered a
> number of 'ERROR: 'xxx.c,v' is not a valid ,v file' error messages when
> converting a CVS repository with cvs2svn 2.3.0.

Thanks a lot for the excellent bug report, analysis, and patch!  I
indeed consider this a bug.

> [...] This code implicitly assumes that a newphrase is a single token that can
> be discarded (the lines starting with # Chew up "newphrase"):

...or multiple tokens, provided that none of them are in admin_token_map
or start with a digit...

>   def parse_rcs_admin(self):
>     while 1:
>       # Read initial token at beginning of line
>       token = self.ts.get()
> 
>       try:
>         f = self.admin_token_map[token]
>       except KeyError:
>         # We're done once we reach the description of the RCS tree
>         if token[0] in string.digits:
>           self.ts.unget(token)
>           return
>         else:
>           # Chew up "newphrase"
>           # warn("Unexpected RCS token: $token\n")
>           pass
>       else:
>         if f is None:
>           self.ts.unget(token)
>           return
>         else:
>           f(self, token)
> 
> However, a newphrase can consist of multiple tokens and ends with a
> semicolon, so IMHO the code could be improved to handle this better, as
> follows:
> 
>   def _parse_admin_newphrase(self):
>     while 1:
>       token = self.ts.get()
>       if token == ';':
>         break
> 
>   def parse_rcs_admin(self):
>     while 1:
>       # Read initial token at beginning of line
>       token = self.ts.get()
>       print "FOUND TOKEN: %s" % (token)
>       try:
>         f = self.admin_token_map[token]
>       except KeyError:
>         # We're done once we reach the description of the RCS tree
>         if token[0] in string.digits:
>           self.ts.unget(token)
>           return
>         else:
>           # Chew up "newphrase"
>           self._parse_admin_newphrase()         
>       else:
>         if f is None:
>           self.ts.unget(token)
>           return
>         else:
>           f(self, token)

Yes, this looks very reasonable.  It would make sense to remove the
"print" statement, and the _parse_admin_newphrase() method could be
shortened to

    while self.ts.get() != ';':
      pass

and probably inlined.

It would also be helpful if you would include your RCS file (or a
suitably simplified/anonymized version), for testing purposes.

But...the rcsparse code is actually taken from the ViewVC project [1,2].
 It would be preferable that you submit your bug fix to that project.
Once it is in the upstream project, the cvs2svn project would upgrade to
the fixed version.

Michael

[1] http://www.viewvc.org/
[2] http://www.viewvc.org/contributing.html

-- 
Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/

------------------------------------------------------
http://cvs2svn.tigris.org/ds/viewMessage.do?dsForumId=1667&dsMessageId=2717668

To unsubscribe from this discussion, e-mail: [[email protected]].