[PATCH] largefiles: improve comments, internal docstrings

Greg Ward greg at gerg.ca
Wed Oct 12 20:55:49 CDT 2011


# HG changeset patch
# User Greg Ward <greg at gerg.ca>
# Date 1318467567 14400
# Node ID af15a79d808dffe2d4e5804fe74dc87612021995
# Parent  dac2edce4e4a7eb63ff2c1bb6ab2289c0bcec9e5
largefiles: improve comments, internal docstrings

- fix some ungrammatical/unclear/incorrect comments/docstrings
- rewrite some really unclear comments/docstrings
- make formatting/style more consistent with the rest of Mercurial
  (lowercase without period unless it's really multiple sentences)
- wrap to 75 columns
- always say "largefile(s)", not "lfile(s)" (or "big files")
- one space between sentences, not two

diff --git a/hgext/largefiles/basestore.py b/hgext/largefiles/basestore.py
--- a/hgext/largefiles/basestore.py
+++ b/hgext/largefiles/basestore.py
@@ -6,7 +6,7 @@
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
-'''Base class for store implementations and store-related utility code.'''
+'''base class for store implementations and store-related utility code'''
 
 import os
 import tempfile
@@ -168,9 +168,10 @@
     if not remote:
         path = getattr(repo, 'lfpullsource', None) or \
             ui.expandpath('default-push', 'default')
-        # If 'default-push' and 'default' can't be expanded
-        # they are just returned. In that case use the empty string which
-        # use the filescheme.
+
+        # ui.expandpath() leaves 'default-push' and 'default' alone if
+        # they cannot be expanded: fallback to the empty string,
+        # meaning the current directory.
         if path == 'default-push' or path == 'default':
             path = ''
             remote = repo
diff --git a/hgext/largefiles/lfcommands.py b/hgext/largefiles/lfcommands.py
--- a/hgext/largefiles/lfcommands.py
+++ b/hgext/largefiles/lfcommands.py
@@ -6,7 +6,7 @@
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
-'''High-level command functions: lfadd() et. al, plus the cmdtable.'''
+'''High-level command function for lfconvert, plus the cmdtable.'''
 
 import os
 import shutil
@@ -316,11 +316,9 @@
     revmap[ctx.node()] = rdst.changelog.tip()
 
 def _islfile(file, ctx, matcher, size):
-    '''
-    A file is a lfile if it matches a pattern or is over
-    the given size.
-    '''
-    # Never store hgtags or hgignore as lfiles
+    '''Return true if file should be considered a largefile, i.e.
+    matcher matches it or it is larger than size.'''
+    # never store special .hg* files as largefiles
     if file == '.hgtags' or file == '.hgignore' or file == '.hgsigs':
         return False
     if matcher and matcher(file):
diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
--- a/hgext/largefiles/lfutil.py
+++ b/hgext/largefiles/lfutil.py
@@ -76,7 +76,7 @@
     try:
         util.oslink(src, dest)
     except OSError:
-        # If hardlinks fail fall back on copy
+        # if hardlinks fail, fallback on copy
         shutil.copyfile(src, dest)
         os.chmod(dest, os.stat(src).st_mode)
 
@@ -122,8 +122,8 @@
 
 def openlfdirstate(ui, repo):
     '''
-    Return a dirstate object that tracks big files: i.e. its root is the
-    repo root, but it is saved in .hg/largefiles/dirstate.
+    Return a dirstate object that tracks largefiles: i.e. its root is
+    the repo root, but it is saved in .hg/largefiles/dirstate.
     '''
     admin = repo.join(longname)
     opener = scmutil.opener(admin)
@@ -133,10 +133,10 @@
     else:
         lfdirstate = largefiles_dirstate(opener, ui, repo.root)
 
-    # If the largefiles dirstate does not exist, populate and create it.  This
-    # ensures that we create it on the first meaningful largefiles operation in
-    # a new clone.  It also gives us an easy way to forcibly rebuild largefiles
-    # state:
+    # If the largefiles dirstate does not exist, populate and create
+    # it. This ensures that we create it on the first meaningful
+    # largefiles operation in a new clone. It also gives us an easy
+    # way to forcibly rebuild largefiles state:
     #   rm .hg/largefiles/dirstate && hg status
     # Or even, if things are really messed up:
     #   rm -rf .hg/largefiles && hg status
@@ -177,7 +177,8 @@
     return (modified, added, removed, missing, unknown, ignored, clean)
 
 def listlfiles(repo, rev=None, matcher=None):
-    '''list largefiles in the working copy or specified changeset'''
+    '''return a list of largefiles in the working copy or the
+    specified changeset'''
 
     if matcher is None:
         matcher = getstandinmatcher(repo)
@@ -197,10 +198,11 @@
     return repo.join(os.path.join(longname, hash))
 
 def copyfromcache(repo, hash, filename):
-    '''copyfromcache copies the specified largefile from the repo or system
-    cache to the specified location in the repository.  It will not throw an
-    exception on failure, as it is meant to be called only after ensuring that
-    the needed largefile exists in the cache.'''
+    '''Copy the specified largefile from the repo or system cache to
+    filename in the repository. Return true on success or false if the
+    file was not found in either cache (which should not happened:
+    this is meant to be called only after ensuring that the needed
+    largefile exists in the cache).'''
     path = findfile(repo, hash)
     if path is None:
         return False
@@ -249,9 +251,9 @@
     return getmatcher(repo, pats, opts, showbad=False)
 
 def getmatcher(repo, pats=[], opts={}, showbad=True):
-    '''Wrapper around scmutil.match() that adds showbad: if false, neuter
-    the match object\'s bad() method so it does not print any warnings
-    about missing files or directories.'''
+    '''Wrapper around scmutil.match() that adds showbad: if false,
+    neuter the match object's bad() method so it does not print any
+    warnings about missing files or directories.'''
     match = scmutil.match(repo[None], pats, opts)
 
     if not showbad:
@@ -259,9 +261,9 @@
     return match
 
 def composestandinmatcher(repo, rmatcher):
-    '''Return a matcher that accepts standins corresponding to the files
-    accepted by rmatcher. Pass the list of files in the matcher as the
-    paths specified by the user.'''
+    '''Return a matcher that accepts standins corresponding to the
+    files accepted by rmatcher. Pass the list of files in the matcher
+    as the paths specified by the user.'''
     smatcher = getstandinmatcher(repo, rmatcher.files())
     isstandin = smatcher.matchfn
     def composed_matchfn(f):
@@ -283,8 +285,8 @@
     return shortname + '/' + filename.replace(os.sep, '/')
 
 def isstandin(filename):
-    '''Return true if filename is a big file standin.  filename must
-    be in Mercurial\'s internal form (slash-separated).'''
+    '''Return true if filename is a big file standin. filename must be
+    in Mercurial's internal form (slash-separated).'''
     return filename.startswith(shortname + '/')
 
 def splitstandin(filename):
@@ -310,7 +312,7 @@
     return repo[node][standin(filename)].data().strip()
 
 def writestandin(repo, standin, hash, executable):
-    '''write hhash to <repo.root>/<standin>'''
+    '''write hash to <repo.root>/<standin>'''
     writehash(hash, repo.wjoin(standin), executable)
 
 def copyandhash(instream, outfile):
@@ -323,7 +325,7 @@
         outfile.write(data)
 
     # Blecch: closing a file that somebody else opened is rude and
-    # wrong.  But it's so darn convenient and practical!  After all,
+    # wrong. But it's so darn convenient and practical! After all,
     # outfile was opened just to copy and hash.
     outfile.close()
 
@@ -364,7 +366,7 @@
         if not data:
             break
         yield data
-    # Same blecch as above.
+    # same blecch as copyandhash() above
     infile.close()
 
 def readhash(filename):
@@ -425,9 +427,8 @@
 def httpsendfile(ui, filename):
     return httpconnection.httpsendfile(ui, filename, 'rb')
 
-# Convert a path to a unix style path. This is used to give a
-# canonical path to the lfdirstate.
 def unixpath(path):
+    '''Return a version of path normalized for use with the lfdirstate.'''
     return os.path.normpath(path).replace(os.sep, '/')
 
 def islfilesrepo(repo):
diff --git a/hgext/largefiles/localstore.py b/hgext/largefiles/localstore.py
--- a/hgext/largefiles/localstore.py
+++ b/hgext/largefiles/localstore.py
@@ -6,7 +6,7 @@
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
-'''Store class for local filesystem.'''
+'''store class for local filesystem'''
 
 import os
 
@@ -17,17 +17,17 @@
 import basestore
 
 class localstore(basestore.basestore):
-    '''Because there is a system wide cache, the local store always uses that
-    cache.  Since the cache is updated elsewhere, we can just read from it here
-    as if it were the store.'''
+    '''Because there is a system-wide cache, the local store always
+    uses that cache. Since the cache is updated elsewhere, we can
+    just read from it here as if it were the store.'''
 
     def __init__(self, ui, repo, remote):
         url = os.path.join(remote.path, '.hg', lfutil.longname)
         super(localstore, self).__init__(ui, repo, util.expandpath(url))
 
     def put(self, source, filename, hash):
-        '''Any file that is put must already be in the system wide cache so do
-        nothing.'''
+        '''Any file that is put must already be in the system-wide
+        cache so do nothing.'''
         return
 
     def exists(self, hash):
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -59,9 +59,9 @@
 
 # -- Wrappers: modify existing commands --------------------------------
 
-# Add works by going through the files that the user wanted to add
-# and checking if they should be added as lfiles. Then making a new
-# matcher which matches only the normal files and running the original
+# Add works by going through the files that the user wanted to add and
+# checking if they should be added as largefiles. Then it makes a new
+# matcher which matches only the normal files and runs the original
 # version of add.
 def override_add(orig, ui, repo, *pats, **opts):
     large = opts.pop('large', None)
@@ -101,8 +101,8 @@
     bad = []
     standins = []
 
-    # Need to lock otherwise there could be a race condition inbetween when
-    # standins are created and added to the repo
+    # Need to lock, otherwise there could be a race condition between
+    # when standins are created and added to the repo.
     wlock = repo.wlock()
     try:
         if not opts.get('dry_run'):
@@ -221,8 +221,8 @@
         False, False)
     (unsure, modified, added, removed, missing, unknown, ignored, clean) = s
 
-    # Need to lock between the standins getting updated and their lfiles
-    # getting updated
+    # Need to lock between the standins getting updated and their
+    # largefiles getting updated
     wlock = repo.wlock()
     try:
         if opts['check']:
@@ -245,9 +245,9 @@
         wlock.release()
     return orig(ui, repo, *pats, **opts)
 
-# Override filemerge to prompt the user about how they wish to merge lfiles.
-# This will handle identical edits, and copy/rename + edit without prompting
-# the user.
+# Override filemerge to prompt the user about how they wish to merge
+# largefiles. This will handle identical edits, and copy/rename +
+# edit without prompting the user.
 def override_filemerge(origfn, repo, mynode, orig, fcd, fco, fca):
     # Use better variable names here. Because this is a wrapper we cannot
     # change the variable names in the function declaration.
@@ -288,12 +288,13 @@
             repo.wwrite(fcdest.path(), fcother.data(), fcother.flags())
             return 0
 
-# Copy first changes the matchers to match standins instead of lfiles.
-# Then it overrides util.copyfile in that function it checks if the destination
-# lfile already exists. It also keeps a list of copied files so that the lfiles
-# can be copied and the dirstate updated.
+# Copy first changes the matchers to match standins instead of
+# largefiles.  Then it overrides util.copyfile in that function it
+# checks if the destination largefile already exists. It also keeps a
+# list of copied files so that the largefiles can be copied and the
+# dirstate updated.
 def override_copy(orig, ui, repo, pats, opts, rename=False):
-    # doesn't remove lfile on rename
+    # doesn't remove largefile on rename
     if len(pats) < 2:
         # this isn't legal, let the original function deal with it
         return orig(ui, repo, pats, opts, rename)
@@ -309,9 +310,10 @@
     if os.path.isdir(dest):
         if not os.path.isdir(makestandin(dest)):
             os.makedirs(makestandin(dest))
-    # This could copy both lfiles and normal files in one command, but we don't
-    # want to do that first replace their matcher to only match normal files
-    # and run it then replace it to just match lfiles and run it again
+    # This could copy both largefiles and normal files in one command,
+    # but we don't want to do that first replace their matcher to only
+    # match normal files and run it then replace it to just match
+    # lfiles and run it again
     nonormalfiles = False
     nolfiles = False
     try:
diff --git a/hgext/largefiles/proto.py b/hgext/largefiles/proto.py
--- a/hgext/largefiles/proto.py
+++ b/hgext/largefiles/proto.py
@@ -17,8 +17,8 @@
                           'file.\n'
 
 def putlfile(repo, proto, sha):
-    """putlfile puts a largefile into a repository's local cache and into the
-    system cache."""
+    '''Put a largefile into a repository's local cache and into the
+    system cache.'''
     f = None
     proto.redirect()
     try:
@@ -40,17 +40,19 @@
     return wireproto.pushres(0)
 
 def getlfile(repo, proto, sha):
-    """getlfile retrieves a largefile from the repository-local cache or system
-    cache."""
+    '''Retrieve a largefile from the repository-local cache or system
+    cache.'''
     filename = lfutil.findfile(repo, sha)
     if not filename:
         raise util.Abort(_('requested largefile %s not present in cache') % sha)
     f = open(filename, 'rb')
     length = os.fstat(f.fileno())[6]
-    # since we can't set an HTTP content-length header here, and mercurial core
-    # provides no way to give the length of a streamres (and reading the entire
-    # file into RAM would be ill-advised), we just send the length on the first
-    # line of the response, like the ssh proto does for string responses.
+
+    # Since we can't set an HTTP content-length header here, and
+    # Mercurial core provides no way to give the length of a streamres
+    # (and reading the entire file into RAM would be ill-advised), we
+    # just send the length on the first line of the response, like the
+    # ssh proto does for string responses.
     def generator():
         yield '%d\n' % length
         for chunk in f:
@@ -58,8 +60,8 @@
     return wireproto.streamres(generator())
 
 def statlfile(repo, proto, sha):
-    """statlfile sends '2\n' if the largefile is missing, '1\n' if it has a
-    mismatched checksum, or '0\n' if it is in good condition"""
+    '''Return '2\n' if the largefile is missing, '1\n' if it has a
+    mismatched checksum, or '0\n' if it is in good condition'''
     filename = lfutil.findfile(repo, sha)
     if not filename:
         return '2\n'
@@ -113,10 +115,10 @@
             try:
                 return int(self._call("statlfile", sha=sha))
             except (ValueError, urllib2.HTTPError):
-                # if the server returns anything but an integer followed by a
+                # If the server returns anything but an integer followed by a
                 # newline, newline, it's not speaking our language; if we get
                 # an HTTP error, we can't be sure the largefile is present;
-                # either way, consider it missing
+                # either way, consider it missing.
                 return 2
 
     repo.__class__ = lfileswirerepository
diff --git a/hgext/largefiles/remotestore.py b/hgext/largefiles/remotestore.py
--- a/hgext/largefiles/remotestore.py
+++ b/hgext/largefiles/remotestore.py
@@ -4,7 +4,7 @@
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
-'''Remote largefile store; the base class for servestore'''
+'''remote largefile store; the base class for servestore'''
 
 import urllib2
 
@@ -15,7 +15,7 @@
 import basestore
 
 class remotestore(basestore.basestore):
-    """A largefile store accessed over a network"""
+    '''a largefile store accessed over a network'''
     def __init__(self, ui, repo, url):
         super(remotestore, self).__init__(ui, repo, url)
 
diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py
+++ b/hgext/largefiles/reposetup.py
@@ -42,9 +42,10 @@
         def status_nolfiles(self, *args, **kwargs):
             return super(lfiles_repo, self).status(*args, **kwargs)
 
-        # When lfstatus is set, return a context that gives the names of lfiles
-        # instead of their corresponding standins and identifies the lfiles as
-        # always binary, regardless of their actual contents.
+        # When lfstatus is set, return a context that gives the names
+        # of largefiles instead of their corresponding standins and
+        # identifies the largefiles as always binary, regardless of
+        # their actual contents.
         def __getitem__(self, changeid):
             ctx = super(lfiles_repo, self).__getitem__(changeid)
             if self.lfstatus:
@@ -81,9 +82,9 @@
             return ctx
 
         # Figure out the status of big files and insert them into the
-        # appropriate list in the result. Also removes standin files from
-        # the listing. This function reverts to the original status if
-        # self.lfstatus is False
+        # appropriate list in the result. Also removes standin files
+        # from the listing. Revert to the original status if
+        # self.lfstatus is False.
         def status(self, node1='.', node2=None, match=None, ignored=False,
                 clean=False, unknown=False, listsubrepos=False):
             listignored, listclean, listunknown = ignored, clean, unknown
@@ -131,8 +132,8 @@
                 m = copy.copy(match)
                 m._files = [tostandin(f) for f in m._files]
 
-                # get ignored clean and unknown but remove them later if they
-                # were not asked for
+                # get ignored, clean, and unknown but remove them
+                # later if they were not asked for
                 try:
                     result = super(lfiles_repo, self).status(node1, node2, m,
                         True, True, True, listsubrepos)
@@ -140,11 +141,11 @@
                     result = super(lfiles_repo, self).status(node1, node2, m,
                         True, True, True)
                 if working:
-                    # Hold the wlock while we read lfiles and update the
-                    # lfdirstate
+                    # hold the wlock while we read largefiles and
+                    # update the lfdirstate
                     wlock = repo.wlock()
                     try:
-                        # Any non lfiles that were explicitly listed must be
+                        # Any non-largefiles that were explicitly listed must be
                         # taken out or lfdirstate.status will report an error.
                         # The status of these files was already computed using
                         # super's status.
@@ -321,15 +322,15 @@
                 for f in match._files:
                     fstandin = lfutil.standin(f)
 
-                    # Ignore known lfiles and standins
+                    # ignore known largefiles and standins
                     if f in lfiles or fstandin in standins:
                         continue
 
-                    # Append directory separator to avoid collisions
+                    # append directory separator to avoid collisions
                     if not fstandin.endswith(os.sep):
                         fstandin += os.sep
 
-                    # Prevalidate matching standin directories
+                    # prevalidate matching standin directories
                     if lfutil.any_(st for st in match._files if \
                             st.startswith(fstandin)):
                         continue
@@ -384,8 +385,8 @@
     def checkrequireslfiles(ui, repo, **kwargs):
         if 'largefiles' not in repo.requirements and lfutil.any_(
                 lfutil.shortname+'/' in f[0] for f in repo.store.datafiles()):
-            # work around bug in mercurial 1.9 whereby requirements is a list
-            # on newly-cloned repos
+            # workaround bug in Mercurial 1.9 whereby requirements is
+            # a list on newly-cloned repos
             repo.requirements = set(repo.requirements)
 
             repo.requirements |= set(['largefiles'])
diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
--- a/hgext/largefiles/uisetup.py
+++ b/hgext/largefiles/uisetup.py
@@ -110,8 +110,8 @@
     proto.capabilities_orig = wireproto.capabilities
     wireproto.capabilities = proto.capabilities
 
-    # these let us reject non-lfiles clients and make them display our error
-    # messages
+    # these let us reject non-largefiles clients and make them display
+    # our error messages
     protocol.webproto.refuseclient = proto.webproto_refuseclient
     sshserver.sshserver.refuseclient = proto.sshproto_refuseclient
 
diff --git a/hgext/largefiles/wirestore.py b/hgext/largefiles/wirestore.py
--- a/hgext/largefiles/wirestore.py
+++ b/hgext/largefiles/wirestore.py
@@ -3,7 +3,7 @@
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
-'''largefile store working over mercurial's wire protocol'''
+'''largefile store working over Mercurial's wire protocol'''
 
 import lfutil
 import remotestore


More information about the Mercurial-devel mailing list