[PATCH 4 of 6] bmstore: close file in a finally block in _writerepo

[Simon King]

On Wed, Dec 2, 2015 at 4:19 PM, Augie Fackler <raf at durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie at google.com>
> # Date 1447293828 18000
> #      Wed Nov 11 21:03:48 2015 -0500
> # Node ID 293c2a5543cbe1243e89592c80d24b634f6c99c7
> # Parent  6a78f21222c1cee841367433bd354ec0402b6346
> bmstore: close file in a finally block in _writerepo
>
> Also rename the variable to file_ to avoid shadowing a builtin.
>
> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -132,9 +132,11 @@ class bmstore(dict):
>          wlock = repo.wlock()
>          try:
>
> -            file = repo.vfs('bookmarks', 'w', atomictemp=True)
> -            self._write(file)
> -            file.close()
> +            file_ = repo.vfs('bookmarks', 'w', atomictemp=True)
> +            try:
> +                self._write(file_)
> +            finally:
> +                file_.close()
>

If an exception occurs in self._write, won't this overwrite the bookmarks
file with a potentially half-written one? Should you use something like
this instead:

try:
    self._write(file_)
except:
    file_.discard()
    raise
else:
    file_.close()


Simon
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20151202/f4d02304/attachment.html>