[PATCH V2] largefiles: replace tempfile.NamedTemporaryFile with tempfile.mkstemp

[Greg Ward]

On Mon, Oct 24, 2011 at 6:01 PM, Hao Lian <hao at fogcreek.com> wrote:
> # HG changeset patch
> # User Hao Lian <hao at fogcreek.com>
> # Date 1319145899 14400
> # Branch stable
> # Node ID c53a79531d3c5688cc31d13e0ba4c288c57e79ef
> # Parent  8b8dd13295dbd733cc03ebb3c2af2f33d24d5428
> largefiles: replace tempfile.NamedTemporaryFile with tempfile.mkstemp
>
> This is consistent with the rest of Mercurial's code, mirroring the
> try-finally-unlink structure elsewhere. Furthermore, it fixes the case where
> largefiles throws an IOError on Windows when the temporary file is opened a
> second time by copytocacheabsolute.
>
> This patch creates the temporary file in the repo's largefiles store rather than
> /tmp, which might be a different filesystem.
>
> diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
> --- a/hgext/largefiles/lfutil.py
> +++ b/hgext/largefiles/lfutil.py
> @@ -13,6 +13,7 @@
>  import platform
>  import shutil
>  import stat
> +import tempfile
>
>  from mercurial import dirstate, httpconnection, match as match_, util, scmutil
>  from mercurial.i18n import _
> @@ -442,6 +443,13 @@
>     return ('largefiles' in repo.requirements and
>             util.any(shortname + '/' in f[0] for f in repo.store.datafiles()))
>
> +def mkstemp(repo, prefix):
> +    '''Returns a file object and a filename corresponding to a temporary file in
> +    the repo's largefiles store.'''

That's not a file object, it's a file descriptor. I would just say

  '''Return (fd, filename) for a temporary file in the repo's
largefile store.'''

> +    path = repo.join(longname)
> +    createdir(repo.join(path))

Should use mercurial.util.makedirs(). Can lfutil.createdir() disappear
entirely? don't see what value it adds.

>  class storeprotonotcapable(Exception):
>     def __init__(self, storetypes):
>         self.storetypes = storetypes
> diff --git a/hgext/largefiles/proto.py b/hgext/largefiles/proto.py
> --- a/hgext/largefiles/proto.py
> +++ b/hgext/largefiles/proto.py
> @@ -4,7 +4,6 @@
>  # GNU General Public License version 2 or any later version.
>
>  import os
> -import tempfile
>  import urllib2
>
>  from mercurial import error, httprepo, util, wireproto
> @@ -19,23 +18,25 @@
>  def putlfile(repo, proto, sha):
>     '''Put a largefile into a repository's local store and into the
>     user cache.'''
> -    f = None
>     proto.redirect()
> +
> +    fd, tmpname = lfutil.mkstemp(repo, prefix='hg-putlfile')
> +    tmpfp = os.fdopen(fd, 'wb+')
>     try:
>         try:
> -            f = tempfile.NamedTemporaryFile(mode='wb+', prefix='hg-putlfile-')
> -            proto.getfile(f)
> -            f.seek(0)
> -            if sha != lfutil.hexsha1(f):
> +            proto.getfile(tmpfp)
> +            tmpfp.seek(0)
> +            if sha != lfutil.hexsha1(tmpfp):
>                 return wireproto.pushres(1)
> +            tmpfp.close()
>             lfutil.copytostoreabsolute(repo, f.name, sha)
>         except IOError:
> -            repo.ui.warn(
> -                _('error: could not put received data into largefile store'))
> +            repo.ui.warn(_('largefiles: failed to put %s (%s) into store') %
> +                         (sha, tmpname))

You're still not showing the actual error! Disk full? Permission
denied? Something else? I need to *know* -- don't make me guess!

Greg