[PATCH] largefiles: replace usage of tempfile.NamedTemporaryFile with tempfile.mkstemp

[Greg Ward]

On Thu, Oct 20, 2011 at 5:31 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 569feddf9101067ced5a3375a9a9d844ee5dc64a
> # Parent  3eb1a90ea40941ad7a920daad665e27ffdb6f291
> largefiles: replace usage of 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.

Please wrap commit messages to 80 columns
(http://mercurial.selenic.com/wiki/ContributingChanges).

> diff --git a/hgext/largefiles/proto.py b/hgext/largefiles/proto.py
> --- a/hgext/largefiles/proto.py
> +++ b/hgext/largefiles/proto.py
> @@ -19,23 +19,26 @@
>  def putlfile(repo, proto, sha):
>     '''Put a largefile into a repository's local cache and into the
>     system cache.'''
> -    f = None
> +

Unnecessary blank line.

>     proto.redirect()
> +
> +    fd, tmpname = tempfile.mkstemp(prefix='hg-putlfile-')
> +    tmpfp = os.fdopen(fd, 'wb+')

At least on Unix, this almost certainly puts the temp file in /tmp.
That is very unlikely to be the same filesystem as my repository,
which means we're going to have to copy the file at the end -- cannot
hardlink.

I'm confused about whether the file is copied to
<repo.root>/.hg/largefiles or ~/.largefiles (or whatever the user
cache is called now). If the former, you should pass
dir=<repo.root>/.hg/largefiles to mkstemp(). If the latter, pass
dir=<usercache>. If both, it doesn't really matter. You might still be
able to hardlink one of them, but you might not. But if you put the
temp file in /tmp, it's more likely that you'll have to copy at least
once.

>     try:
> -        try:
> -            f = tempfile.NamedTemporaryFile(mode='wb+', prefix='hg-putlfile-')
> -            proto.getfile(f)
> -            f.seek(0)
> -            if sha != lfutil.hexsha1(f):
> -                return wireproto.pushres(1)
> -            lfutil.copytocacheabsolute(repo, f.name, sha)
> -        except IOError:
> -            repo.ui.warn(
> -                _('error: could not put received data into largefile store'))
> +        proto.getfile(tmpfp)
> +        tmpfp.seek(0)
> +        if sha != lfutil.hexsha1(tmpfp):
>             return wireproto.pushres(1)
> +        tmpfp.close()
> +
> +        lfutil.copytocacheabsolute(repo, tmpname, sha)
> +    except IOError:
> +        repo.ui.warn(
> +            _('error: could not put received data into largefile store'))

We generally don't start error messages with "error:" (unfortunately).
Also, there's no indication of where the error happened (what file,
what dir) or what it was (disk full? permission denied?). Please use
the exception object!

> +        return wireproto.pushres(1)
>     finally:
> -        if f:
> -            f.close()
> +        tmpfp.close()
> +        os.unlink(tmpname)
>
>     return wireproto.pushres(0)