RE: [PATCH 1/N] Document that CVSFile.get_filename() is normalised

Jon Foster <[email protected]> Thu, 8 Apr 2010 15:41:02 +0100

Newsgroups	gmane.comp.version-control.subversion.cvs2svn.devel
Message-ID	<[email protected]>

Hi,

Michael Haggerty wrote:
> Jon Foster wrote:
> > The paths returned by CVSFile.get_filename() are already normalised,
> > there's no need to pass them through os.path.normpath() again.
> > I'm not happy relying on undocumented behaviour, so document this.
> 
> Is this always the case?  I think you are relying on
> 
>     os.path.join(os.path.normpath(a), os.path.normpath(b)) \
>         == os.path.normpath(os.path.join(a, b))

Almost.  I'm relying on a similar but related rule:

     os.path.join(os.path.normpath(a), b) \
         == os.path.normpath(os.path.join(a, b))
...where b was returned by os.listdir().

> but this is not necessarily the case, for example if a or b contain
'.'
> or '..'.  For example, I think that there is a problem if
> project.project_cvs_repos_path=='.'.

You're right, this won't work if project.project_cvs_repos_path=='.'.
Good catch!

> I like the idea of guaranteeing that the output of get_filename() is
> normalized, though.  What do you think about putting a call to
> os.path.normpath() in your _calculate_filename() method and doing the
> same for CVSDirectory?

That sounds good.  We don't create that many CVSPath objects so there's
no need to totally optimize out os.path.normpath().  Calling it once
per CVSPath is fine.

> You might also think about pulling up some common code (e.g., the
> get_filename() method, the "filename" attribute, and the
initialization
> thereof) to CVSPath to avoid gratuitous differences between
CVSDirectory
> and CVSFile.  Of course the _calculate_filename() methods will have to
> be defined in the derived classes.

Patch is attached.  I've renamed get_filename() to
_calculate_filename().
I made CVSPath call os.path.normpath(_calculate_filename()) at
construction time, and cache the result in a new "filename" member
variable.  There's a get_filename() function in CVSPath for backwards
compatibility.

This replaces patches 1 and 3 from my original optimization patch
series.  You'll still want to look at patches 2 and 4 (the symbol
transform changes).

Kind regards,

Jon

**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.

If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.

Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232

Co. Registered in England number 02817269

Please contact the sender if you believe you have received this email in error.

**********************************************************************

______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email 
______________________________________________________________________

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

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

cvs2svn_opt3_cvsfile_filename_patch_v2.txt (text/plain, 3.5 KB)

Index: cvs2svn_lib/cvs_path.py
===================================================================
--- cvs2svn_lib/cvs_path.py	(revision 5107)
+++ cvs2svn_lib/cvs_path.py	(working copy)
@@ -55,6 +55,7 @@
       'parent_directory',
       'basename',
       'ordinal',
+      'filename',
       ]
 
   def __init__(self, id, project, parent_directory, basename):
@@ -63,6 +64,8 @@
     self.parent_directory = parent_directory
     self.basename = basename
 
+    self.filename = os.path.normpath(self._calculate_filename())
+
   def __getstate__(self):
     """This method must only be called after ordinal has been set."""
 
@@ -79,7 +82,30 @@
         self.ordinal,
         ) = state
     self.project = Ctx()._projects[project_id]
+    self.filename = os.path.normpath(self._calculate_filename())
 
+  def get_filename(self):
+    """Return the filesystem path to this CVSPath in the CVS repository.
+
+    This is in native format, and already normalised the way
+    os.path.normpath() normalises paths.
+
+    It starts with the repository path passed to run_options.add_project()
+    in the options.py file."""
+
+    # This turns out to be a hot path through the code.
+    # It's used by SubtreeSymbolTransform and similar transforms, so it's
+    # called at least:
+    #   (num_files * num_symbols_per_file * num_subtree_symbol_transforms)
+    # times.  On a large repository with several subtree symbol transforms,
+    # that can exceed 100,000,000 calls.  And _calculate_filename() is quite
+    # complex, so doing that every time could add about 10 minutes to the
+    # cvs2svn runtime.
+    #
+    # So now we precalculate this and just return it.
+
+    return self.filename
+
   def get_ancestry(self):
     """Return a list of the CVSPaths leading from the root path to SELF.
 
@@ -175,22 +201,21 @@
   def __init__(self, id, project, parent_directory, basename):
     """Initialize a new CVSDirectory object."""
 
-    CVSPath.__init__(self, id, project, parent_directory, basename)
     # This member is filled in by CollectData.close():
     self.empty_subdirectory_ids = []
 
-  def get_filename(self):
+    CVSPath.__init__(self, id, project, parent_directory, basename)
+
+  def _calculate_filename(self):
     """Return the filesystem path to this CVSPath in the CVS repository."""
 
     if self.parent_directory is None:
       return self.project.project_cvs_repos_path
     else:
       return os.path.join(
-          self.parent_directory.get_filename(), self.basename
+          self.parent_directory.filename, self.basename
           )
 
-  filename = property(get_filename)
-
   def __getstate__(self):
     return (
         CVSPath.__getstate__(self),
@@ -262,16 +287,16 @@
         ):
     """Initialize a new CVSFile object."""
 
-    CVSPath.__init__(self, id, project, parent_directory, basename)
+    assert parent_directory is not None
+
     self._in_attic = in_attic
     self.executable = executable
     self.file_size = file_size
     self.mode = mode
     self.description = description
+    CVSPath.__init__(self, id, project, parent_directory, basename)
 
-    assert self.parent_directory is not None
-
-  def get_filename(self):
+  def _calculate_filename(self):
     """Return the filesystem path to this CVSPath in the CVS repository."""
 
     if self._in_attic:
@@ -283,8 +308,6 @@
           self.parent_directory.filename, self.basename + ',v'
           )
 
-  filename = property(get_filename)
-
   def __getstate__(self):
     return (
         CVSPath.__getstate__(self),