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

login
register
mail settings
Submitter Augie Fackler
Date Dec. 2, 2015, 4:19 p.m.
Message ID <293c2a5543cbe1243e89.1449073150@imladris.local>
Download mbox | patch
Permalink /patch/11751/
State Superseded
Headers show

Comments

Augie Fackler - Dec. 2, 2015, 4:19 p.m.
# HG changeset patch
# User Augie Fackler <augie@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.
Simon King - Dec. 2, 2015, 4:42 p.m.
On Wed, Dec 2, 2015 at 4:19 PM, Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@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
Augie Fackler - Dec. 2, 2015, 4:48 p.m.
> On Dec 2, 2015, at 11:42 AM, Simon King <simon@simonking.org.uk> wrote:
> 
> On Wed, Dec 2, 2015 at 4:19 PM, Augie Fackler <raf@durin42.com> wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@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()

Good catch. I’ll mail a v2 presently.

> 
> 
> Simon
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

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()
 
         finally:
             wlock.release()