Patchwork [02,of,10] transaction: only generate file when we actually close the transaction

login
register
mail settings
Submitter Pierre-Yves David
Date Oct. 28, 2014, 3:41 p.m.
Message ID <74f8131d57fbe539ed5f.1414510876@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/6480/
State Accepted
Headers show

Comments

Pierre-Yves David - Oct. 28, 2014, 3:41 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1413606348 25200
#      Fri Oct 17 21:25:48 2014 -0700
# Branch stable
# Node ID 74f8131d57fbe539ed5fe16d84230b47ef328ddb
# Parent  5ccb5eb0a6dedf6910ed95434b6dde95b5e473c5
transaction: only generate file when we actually close the transaction

Before this change, the file were written for every call to `tr.close()`
exposing data to reader far too early.
Pierre-Yves David - Oct. 28, 2014, 3:57 p.m.
On 10/28/2014 04:41 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1413606348 25200
> #      Fri Oct 17 21:25:48 2014 -0700
> # Branch stable
> # Node ID 74f8131d57fbe539ed5fe16d84230b47ef328ddb
> # Parent  5ccb5eb0a6dedf6910ed95434b6dde95b5e473c5
> transaction: only generate file when we actually close the transaction

These two first patch are simple and safe.

The 8 others are more impactful but solve some of the current 
inconsistency with changeglog and nested transaction. Nested transaction 
are more common nowaday so even if we never had report for such bug they 
may impact some of our user with 3.2

More transaction work is ready and scheduled for 3.3


>
> Before this change, the file were written for every call to `tr.close()`
> exposing data to reader far too early.
>
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -264,12 +264,12 @@ class transaction(object):
>           return self.count > 0
>
>       @active
>       def close(self):
>           '''commit the transaction'''
> -        self._generatefiles()
>           if self.count == 1 and self.onclose is not None:
> +            self._generatefiles()
>               self.onclose()
>
>           self.count -= 1
>           if self.count != 0:
>               return
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Matt Mackall - Oct. 29, 2014, 9:40 p.m.
On Tue, 2014-10-28 at 16:57 +0100, Pierre-Yves David wrote:
> 
> On 10/28/2014 04:41 PM, Pierre-Yves David wrote:
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david@fb.com>
> > # Date 1413606348 25200
> > #      Fri Oct 17 21:25:48 2014 -0700
> > # Branch stable
> > # Node ID 74f8131d57fbe539ed5fe16d84230b47ef328ddb
> > # Parent  5ccb5eb0a6dedf6910ed95434b6dde95b5e473c5
> > transaction: only generate file when we actually close the transaction
> 
> These two first patch are simple and safe.

Queued these. Though see below about refactoring.

> The 8 others are more impactful but solve some of the current 
> inconsistency with changeglog and nested transaction. Nested transaction 
> are more common nowaday so even if we never had report for such bug they 
> may impact some of our user with 3.2

Pretty terrified to do this much rewriting to fairly subtle code in
stable. 'Refactor' is not a word I like to see in this context: if it's
complex enough that it needs refactoring first to fix, it's probably too
complex to fix in stable.

So now that I've seen these, I'm afraid this is going to have to wait
until after 3.2 (in smaller batches than 10, please).

> More transaction work is ready and scheduled for 3.3

I think we'll need a TransactionPlan page on the wiki that gives an
overview of the mechanics and semantics.
Pierre-Yves David - Nov. 2, 2014, 1:06 p.m.
On 10/29/2014 09:40 PM, Matt Mackall wrote:
> On Tue, 2014-10-28 at 16:57 +0100, Pierre-Yves David wrote:
>> More transaction work is ready and scheduled for 3.3
>
> I think we'll need a TransactionPlan page on the wiki that gives an
> overview of the mechanics and semantics.

Basis plan create here: http://mercurial.selenic.com/wiki/TransactionPlan

I'll repatchbomb shortly.

Patch

diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -264,12 +264,12 @@  class transaction(object):
         return self.count > 0
 
     @active
     def close(self):
         '''commit the transaction'''
-        self._generatefiles()
         if self.count == 1 and self.onclose is not None:
+            self._generatefiles()
             self.onclose()
 
         self.count -= 1
         if self.count != 0:
             return