Submitter | Katsunori FUJIWARA |
---|---|
Date | May 28, 2014, 3 p.m. |
Message ID | <59f48202458c5236a656.1401289214@juju> |
Download | mbox | patch |
Permalink | /patch/4883/ |
State | Changes Requested |
Headers | show |
Comments
On 05/28/2014 08:00 AM, FUJIWARA Katsunori wrote: > # HG changeset patch > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> > # Date 1401288802 -32400 > # Wed May 28 23:53:22 2014 +0900 > # Node ID 59f48202458c5236a65631c9a43425e8176245de > # Parent 25cb27f9fa274069475fc8d1a129a272d9d7e147 > subrepo: use "vfs.tryread()" instead of explicit invocations of file APIs > > This patch removes "_calcfilehash", because there is no code path > referring it. > > This patch also avoids building absolute path "absname" up by > "self._repo.join" for files in "filelist", because "vfs.tryread" > doesn't need it. use of "and" uin a commit description. Can this be two changesets? > diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py > --- a/mercurial/subrepo.py > +++ b/mercurial/subrepo.py > @@ -31,14 +31,6 @@ def _getstorehashcachename(remotepath): > '''get a unique filename for the store hash cache of a remote repository''' > return util.sha1(_expandedabspath(remotepath)).hexdigest()[0:12] > > -def _calcfilehash(filename): > - data = '' > - if os.path.exists(filename): > - fd = open(filename, 'rb') > - data = fd.read() > - fd.close() > - return util.sha1(data).hexdigest() > - > class SubrepoAbort(error.Abort): > """Exception class used to avoid handling a subrepo error more than once""" > def __init__(self, *args, **kw): > @@ -554,9 +546,10 @@ class hgsubrepo(abstractsubrepo): > # sort the files that will be hashed in increasing (likely) file size > filelist = ('bookmarks', 'store/phaseroots', 'store/00changelog.i') > yield '# %s\n' % _expandedabspath(remotepath) > + vfs = self._repo.vfs > for relname in filelist: > - absname = os.path.normpath(self._repo.join(relname)) > - yield '%s = %s\n' % (relname, _calcfilehash(absname)) > + yield '%s = %s\n' % (relname, > + util.sha1(vfs.tryread(relname)).hexdigest()) You can maybe use an intermediate variable to avoid the need to spawn this on two lines.
At Thu, 05 Jun 2014 17:20:03 -0700, Pierre-Yves David wrote: > > On 05/28/2014 08:00 AM, FUJIWARA Katsunori wrote: > > # HG changeset patch > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> > > # Date 1401288802 -32400 > > # Wed May 28 23:53:22 2014 +0900 > > # Node ID 59f48202458c5236a65631c9a43425e8176245de > > # Parent 25cb27f9fa274069475fc8d1a129a272d9d7e147 > > subrepo: use "vfs.tryread()" instead of explicit invocations of file APIs > > > > This patch removes "_calcfilehash", because there is no code path > > referring it. > > > > This patch also avoids building absolute path "absname" up by > > "self._repo.join" for files in "filelist", because "vfs.tryread" > > doesn't need it. > > use of "and" uin a commit description. Can this be two changesets? 'building absolute path "absname" up' becomes fully meaningless by using 'vfs.tryread()' instread of '_calcfilehash()'. So, I think that avoiding 'building absolute path "absname" up' and using 'vfs.tryread()' should be applied at the same time. But separating removal of "_calcfilehash" into another patch may make this one easier to be read. I'll try to do so (and write more detailed description) in V2 series > > diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py > > --- a/mercurial/subrepo.py > > +++ b/mercurial/subrepo.py > > @@ -31,14 +31,6 @@ def _getstorehashcachename(remotepath): > > '''get a unique filename for the store hash cache of a remote repository''' > > return util.sha1(_expandedabspath(remotepath)).hexdigest()[0:12] > > > > -def _calcfilehash(filename): > > - data = '' > > - if os.path.exists(filename): > > - fd = open(filename, 'rb') > > - data = fd.read() > > - fd.close() > > - return util.sha1(data).hexdigest() > > - > > class SubrepoAbort(error.Abort): > > """Exception class used to avoid handling a subrepo error more than once""" > > def __init__(self, *args, **kw): > > @@ -554,9 +546,10 @@ class hgsubrepo(abstractsubrepo): > > # sort the files that will be hashed in increasing (likely) file size > > filelist = ('bookmarks', 'store/phaseroots', 'store/00changelog.i') > > yield '# %s\n' % _expandedabspath(remotepath) > > + vfs = self._repo.vfs > > for relname in filelist: > > - absname = os.path.normpath(self._repo.join(relname)) > > - yield '%s = %s\n' % (relname, _calcfilehash(absname)) > > + yield '%s = %s\n' % (relname, > > + util.sha1(vfs.tryread(relname)).hexdigest()) > > You can maybe use an intermediate variable to avoid the need to spawn > this on two lines. I thought that an intermediate variable in this case might be redundant at working for this series. I'll introduce an intermediate variable for readability. > > -- > Pierre-Yves David > ---------------------------------------------------------------------- [FUJIWARA Katsunori] foozy@lares.dti.ne.jp
Patch
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py --- a/mercurial/subrepo.py +++ b/mercurial/subrepo.py @@ -31,14 +31,6 @@ def _getstorehashcachename(remotepath): '''get a unique filename for the store hash cache of a remote repository''' return util.sha1(_expandedabspath(remotepath)).hexdigest()[0:12] -def _calcfilehash(filename): - data = '' - if os.path.exists(filename): - fd = open(filename, 'rb') - data = fd.read() - fd.close() - return util.sha1(data).hexdigest() - class SubrepoAbort(error.Abort): """Exception class used to avoid handling a subrepo error more than once""" def __init__(self, *args, **kw): @@ -554,9 +546,10 @@ class hgsubrepo(abstractsubrepo): # sort the files that will be hashed in increasing (likely) file size filelist = ('bookmarks', 'store/phaseroots', 'store/00changelog.i') yield '# %s\n' % _expandedabspath(remotepath) + vfs = self._repo.vfs for relname in filelist: - absname = os.path.normpath(self._repo.join(relname)) - yield '%s = %s\n' % (relname, _calcfilehash(absname)) + yield '%s = %s\n' % (relname, + util.sha1(vfs.tryread(relname)).hexdigest()) def _getstorehashcachepath(self, remotepath): '''get a unique path for the store hash cache'''