Patchwork [02,of,10] subrepo: use "vfs.tryread()" instead of explicit invocations of file APIs

# 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.

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