| 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
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
> 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()