[viewvc-dev] Re: [patch] supporting cvsnt kopt

Bryce Schober wrote:
> On Thu, Jan 29, 2009 at 3:13 PM, Bryce Schober
> <[email protected]
> <mailto:[email protected]>> wrote:
> 
>     I'm attaching a patch that hopefully will at least start a
>     conversation about implementing the simplest possible handling for
>     basic CVSNT kopts. It will apply the first revision kopt that it
>     encounters if the file's mode is still None.
> 
> 
> Ok, I just realized that my translation of "kv" should not have been
> "k". In fact, I think no translation should be necessary, because "kv"
> will already result in keyword substitution, as implemented in
> InternalRevisionReader.get_content_stream().
> 
> Another point of explanation is that this patch uses the mode of the
> first revision encountered in the rcs file. This doesn't reconstruct
> history accurately, but it does end up using the most recently-commited
> revision, which is most likely to be correct for binary files, which is
> my primary target.
> 
> This patch isn't intended as implying support for CVSNT conversions, but
> at least in my conversion, it's the only thing I badly needed. This is a
> pretty simple fix for problems with binary files in conversions of
> CVSNT, so I'd really like to push for this patch's inclusion.

Thanks for the patch.  I'm sorry I haven't responded earlier.

Part of the problem is that the code that your patch touches actually
belongs to the ViewVC project.  (The script cvs2svn_rcsparse/update.sh
can be used to upgrade its version.)  So any changes there would also
have to be accepted by the ViewVC project.

So I've CCed the ViewVC mailing list, and I am appending a copy of your
patch for their convenience.

I happen to also be a (very inactive) member of the ViewVC project :-)
And as such I am not so happy about the special case nature of your
patch.  For example, only a single "kopt" value is passed through (even
though this option can appear for each revision), and also it is not
obvious to the Sink class that a particular call to the set_expansion()
callback came from a "kopt" option rather than an "expand" option.

On the other hand, it would certainly be nice to help CVSNT users to
escape their quasi-proprietary world.

IMHO it would be much better to find a generic way for the parser to
inform the Sink about RCS "newphrases", no matter where they appear.
This would also allow us to use the "commitid" value if it is present.

Perhaps there should be a new callback set_admin_newphrase(self,
keyword, value), and a new "newphrases" map argument to the
define_revision() and set_revision_info() callbacks.  (This, of course,
assumes that newphrase keywords will never be repeated within one delta
section or one deltatext section, and also that their order is irrelevant.)

Even nicer would be a "**newphrases" argument to the latter two
callbacks; that way callers that are interested in a particular
newphrase can declare their callback like so:

    def define_revision(
        self, revision, timestamp, author, state, branches, next,
        kopt=None, **kw
        ):
        # ...

and also the usage would be backwards-compatible if new keywords are
ever decided to be standard.  Although the rcsfile(5) specification
allows newphrase ids that are not valid Python identifiers, in practice
they probably all are.

I thought several times about making changes like this to the rcsparser
code, but I'm not sure how serious it would be to break backwards
compatibility in the Sink interface.

Michael

------------------------------------------------------
http://viewvc.tigris.org/ds/viewMessage.do?dsForumId=4251&dsMessageId=1106739

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

Index: cvs2svn_rcsparse/common.py
===================================================================
--- cvs2svn_rcsparse/common.py	(revision 4774)
+++ cvs2svn_rcsparse/common.py	(working copy)
@@ -261,6 +261,9 @@
       if token == 'desc' or token[0] in string.digits:
         self.ts.unget(token)
         break
+      if token == 'kopt' and self.sink.cvs_file.mode is None:
+        mode = self.ts.get()
+        self.sink.set_expansion(mode)
       # consume everything up to the semicolon
       self._read_until_semicolon()