Patchwork [stable] dirstate: don't rename branch file if writing it failed

login
register
mail settings
Submitter Idan Kamara
Date Dec. 14, 2012, 2:15 p.m.
Message ID <ee7a3e1aff9ca0436f10.1355494500@idan>
Download mbox | patch
Permalink /patch/105/
State Superseded
Commit 3bc21f6daac472b7f4795fa7951f2e4e6458cacb
Headers show

Comments

Idan Kamara - Dec. 14, 2012, 2:15 p.m.
# HG changeset patch
# User Idan Kamara <idankk86 at gmail.com>
# Date 1355493250 -7200
# Branch stable
# Node ID ee7a3e1aff9ca0436f10447b594b6e307e2afad6
# Parent  ebc0fa067c07808b77f060e285d0c9d8d25c6750
dirstate: don't rename branch file if writing it failed
Pierre-Yves David - Dec. 15, 2012, 12:27 p.m.
On Fri, Dec 14, 2012 at 04:15:00PM +0200, Idan Kamara wrote:
> # HG changeset patch
> # User Idan Kamara <idankk86 at gmail.com>
> # Date 1355493250 -7200
> # Branch stable
> # Node ID ee7a3e1aff9ca0436f10447b594b6e307e2afad6
> # Parent  ebc0fa067c07808b77f060e285d0c9d8d25c6750
> dirstate: don't rename branch file if writing it failed
> 
> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> --- a/mercurial/dirstate.py
> +++ b/mercurial/dirstate.py
> @@ -265,8 +265,9 @@
>          f = self._opener('branch', 'w', atomictemp=True)
>          try:
>              f.write(self._branch + '\n')
> -        finally:
>              f.close()
> +        except:
> +            f.discard()

It looks like you forget to reraise the exception. Otherwise we have a silent
bare except. Those are one of the most annoying things in the
universes.

How did you spotted this? This patch is clearly a welcome fix, but what did
broke without it?
Idan Kamara - Dec. 15, 2012, 1:03 p.m.
On Sat, Dec 15, 2012 at 2:27 PM, Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:
>
> On Fri, Dec 14, 2012 at 04:15:00PM +0200, Idan Kamara wrote:
> > # HG changeset patch
> > # User Idan Kamara <idankk86 at gmail.com>
> > # Date 1355493250 -7200
> > # Branch stable
> > # Node ID ee7a3e1aff9ca0436f10447b594b6e307e2afad6
> > # Parent  ebc0fa067c07808b77f060e285d0c9d8d25c6750
> > dirstate: don't rename branch file if writing it failed
> >
> > diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> > --- a/mercurial/dirstate.py
> > +++ b/mercurial/dirstate.py
> > @@ -265,8 +265,9 @@
> >          f = self._opener('branch', 'w', atomictemp=True)
> >          try:
> >              f.write(self._branch + '\n')
> > -        finally:
> >              f.close()
> > +        except:
> > +            f.discard()
>
> It looks like you forget to reraise the exception. Otherwise we have a
> silent
> bare except. Those are one of the most annoying things in the
> universes.

Oops, will resend.

>
> How did you spotted this? This patch is clearly a welcome fix, but what
> did
> broke without it?

I was changing something close by and saw this.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20121215/dc44a4c0/attachment.html>

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -265,8 +265,9 @@ 
         f = self._opener('branch', 'w', atomictemp=True)
         try:
             f.write(self._branch + '\n')
-        finally:
             f.close()
+        except:
+            f.discard()
 
     def _read(self):
         self._map = {}