Patchwork [2,of,6] transaction: use unfiltered changelog length for transaction start

login
register
mail settings
Submitter Durham Goode
Date May 18, 2017, 6:23 p.m.
Message ID <35235b1cefb101dad067.1495131836@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/20679/
State Superseded
Headers show

Comments

Durham Goode - May 18, 2017, 6:23 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1495129620 25200
#      Thu May 18 10:47:00 2017 -0700
# Node ID 35235b1cefb101dad0672a4c4ee9486fba8b935b
# Parent  19be454ee16925d01da8f9d329cb48556083da1b
transaction: use unfiltered changelog length for transaction start

Previously we used the normal changelog length for the transaction start. This
triggered the computehidden logic which in a future patch triggers logic in the
new hidden store which may start a transaction. This is circular and results in
broken transaction mechanics, so let's use the unfiltered repo instead to avoid
triggering computehidden during transaction start.

This seems like how it should've been in the first place anyway since recording
the length of the filtered changelog seems odd for a transaction.

This was causing test failures in a future patch, so test coverage will come
from the future patch.
Pierre-Yves David - May 19, 2017, 8:05 p.m.
On 05/18/2017 08:23 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1495129620 25200
> #      Thu May 18 10:47:00 2017 -0700
> # Node ID 35235b1cefb101dad0672a4c4ee9486fba8b935b
> # Parent  19be454ee16925d01da8f9d329cb48556083da1b
> transaction: use unfiltered changelog length for transaction start
>
> Previously we used the normal changelog length for the transaction start. This
> triggered the computehidden logic which in a future patch triggers logic in the
> new hidden store which may start a transaction. This is circular and results in
> broken transaction mechanics, so let's use the unfiltered repo instead to avoid
> triggering computehidden during transaction start.
>
> This seems like how it should've been in the first place anyway since recording
> the length of the filtered changelog seems odd for a transaction.
>
> This was causing test failures in a future patch, so test coverage will come
> from the future patch.

yes, writing filtered journal seems a bit odd. However I would rather 
see the whole _writejournal run unfiltered.

There are a '@unfilteredmethod' decorator for this purpose.

To push things further. I wonder why we are not running the transaction 
logic on an unfiltered repository, but I could see potential issue with 
hooks running on an unfiltered repository.

I've run the testsuit with a unfiltered transaction and everything pass. 
That might be from the lack of testing of the problematic cases.

> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1123,7 +1123,7 @@ class localrepository(object):
>          self.vfs.write("journal.branch",
>                            encoding.fromlocal(self.dirstate.branch()))
>          self.vfs.write("journal.desc",
> -                          "%d\n%s\n" % (len(self), desc))
> +                          "%d\n%s\n" % (len(self.unfiltered()), desc))
>          self.vfs.write("journal.bookmarks",
>                            self.vfs.tryread("bookmarks"))
>          self.svfs.write("journal.phaseroots",
via Mercurial-devel - May 24, 2017, 9:17 p.m.
On Fri, May 19, 2017 at 1:05 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 05/18/2017 08:23 PM, Durham Goode wrote:
>>
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1495129620 25200
>> #      Thu May 18 10:47:00 2017 -0700
>> # Node ID 35235b1cefb101dad0672a4c4ee9486fba8b935b
>> # Parent  19be454ee16925d01da8f9d329cb48556083da1b
>> transaction: use unfiltered changelog length for transaction start
>>
>> Previously we used the normal changelog length for the transaction start.
>> This
>> triggered the computehidden logic which in a future patch triggers logic
>> in the
>> new hidden store which may start a transaction. This is circular and
>> results in
>> broken transaction mechanics, so let's use the unfiltered repo instead to
>> avoid
>> triggering computehidden during transaction start.
>>
>> This seems like how it should've been in the first place anyway since
>> recording
>> the length of the filtered changelog seems odd for a transaction.
>>
>> This was causing test failures in a future patch, so test coverage will
>> come
>> from the future patch.
>
>
> yes, writing filtered journal seems a bit odd. However I would rather see
> the whole _writejournal run unfiltered.
>
> There are a '@unfilteredmethod' decorator for this purpose.

That seems like a trivial little improvement for V2. There are no
other calls in the method that seem like they would be affected, so it
should be functionally equivalent as far as I can tell.

>
> To push things further. I wonder why we are not running the transaction
> logic on an unfiltered repository, but I could see potential issue with
> hooks running on an unfiltered repository.
>
> I've run the testsuit with a unfiltered transaction and everything pass.
> That might be from the lack of testing of the problematic cases.
>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -1123,7 +1123,7 @@ class localrepository(object):
>>          self.vfs.write("journal.branch",
>>                            encoding.fromlocal(self.dirstate.branch()))
>>          self.vfs.write("journal.desc",
>> -                          "%d\n%s\n" % (len(self), desc))
>> +                          "%d\n%s\n" % (len(self.unfiltered()), desc))
>>          self.vfs.write("journal.bookmarks",
>>                            self.vfs.tryread("bookmarks"))
>>          self.svfs.write("journal.phaseroots",
>
>
>
> --
> Pierre-Yves David
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - May 24, 2017, 11:38 p.m.
On 05/24/2017 11:17 PM, Martin von Zweigbergk wrote:
> On Fri, May 19, 2017 at 1:05 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 05/18/2017 08:23 PM, Durham Goode wrote:
>>>
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1495129620 25200
>>> #      Thu May 18 10:47:00 2017 -0700
>>> # Node ID 35235b1cefb101dad0672a4c4ee9486fba8b935b
>>> # Parent  19be454ee16925d01da8f9d329cb48556083da1b
>>> transaction: use unfiltered changelog length for transaction start
>>>
>>> Previously we used the normal changelog length for the transaction start.
>>> This
>>> triggered the computehidden logic which in a future patch triggers logic
>>> in the
>>> new hidden store which may start a transaction. This is circular and
>>> results in
>>> broken transaction mechanics, so let's use the unfiltered repo instead to
>>> avoid
>>> triggering computehidden during transaction start.
>>>
>>> This seems like how it should've been in the first place anyway since
>>> recording
>>> the length of the filtered changelog seems odd for a transaction.
>>>
>>> This was causing test failures in a future patch, so test coverage will
>>> come
>>> from the future patch.
>>
>>
>> yes, writing filtered journal seems a bit odd. However I would rather see
>> the whole _writejournal run unfiltered.
>>
>> There are a '@unfilteredmethod' decorator for this purpose.
>
> That seems like a trivial little improvement for V2. There are no
> other calls in the method that seem like they would be affected, so it
> should be functionally equivalent as far as I can tell.

Yep, seems fine as a follow up.

Cheers,
via Mercurial-devel - May 24, 2017, 11:40 p.m.
On Wed, May 24, 2017 at 4:38 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 05/24/2017 11:17 PM, Martin von Zweigbergk wrote:
>>
>> On Fri, May 19, 2017 at 1:05 PM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>>
>>>
>>> On 05/18/2017 08:23 PM, Durham Goode wrote:
>>>>
>>>>
>>>> # HG changeset patch
>>>> # User Durham Goode <durham@fb.com>
>>>> # Date 1495129620 25200
>>>> #      Thu May 18 10:47:00 2017 -0700
>>>> # Node ID 35235b1cefb101dad0672a4c4ee9486fba8b935b
>>>> # Parent  19be454ee16925d01da8f9d329cb48556083da1b
>>>> transaction: use unfiltered changelog length for transaction start
>>>>
>>>> Previously we used the normal changelog length for the transaction
>>>> start.
>>>> This
>>>> triggered the computehidden logic which in a future patch triggers logic
>>>> in the
>>>> new hidden store which may start a transaction. This is circular and
>>>> results in
>>>> broken transaction mechanics, so let's use the unfiltered repo instead
>>>> to
>>>> avoid
>>>> triggering computehidden during transaction start.
>>>>
>>>> This seems like how it should've been in the first place anyway since
>>>> recording
>>>> the length of the filtered changelog seems odd for a transaction.
>>>>
>>>> This was causing test failures in a future patch, so test coverage will
>>>> come
>>>> from the future patch.
>>>
>>>
>>>
>>> yes, writing filtered journal seems a bit odd. However I would rather see
>>> the whole _writejournal run unfiltered.
>>>
>>> There are a '@unfilteredmethod' decorator for this purpose.
>>
>>
>> That seems like a trivial little improvement for V2. There are no
>> other calls in the method that seem like they would be affected, so it
>> should be functionally equivalent as far as I can tell.
>
>
> Yep, seems fine as a follow up.

I have assumed there will be a v2 of this series anyway, so might as
well do it right there (because it sounds like the decorator is
preferred).

>
> Cheers,
>
> --
> Pierre-Yves David
Pierre-Yves David - May 25, 2017, 12:07 a.m.
On 05/25/2017 01:40 AM, Martin von Zweigbergk wrote:
> On Wed, May 24, 2017 at 4:38 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 05/24/2017 11:17 PM, Martin von Zweigbergk wrote:
>>>
>>> On Fri, May 19, 2017 at 1:05 PM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>>
>>>>
>>>> On 05/18/2017 08:23 PM, Durham Goode wrote:
>>>>>
>>>>>
>>>>> # HG changeset patch
>>>>> # User Durham Goode <durham@fb.com>
>>>>> # Date 1495129620 25200
>>>>> #      Thu May 18 10:47:00 2017 -0700
>>>>> # Node ID 35235b1cefb101dad0672a4c4ee9486fba8b935b
>>>>> # Parent  19be454ee16925d01da8f9d329cb48556083da1b
>>>>> transaction: use unfiltered changelog length for transaction start
>>>>>
>>>>> Previously we used the normal changelog length for the transaction
>>>>> start.
>>>>> This
>>>>> triggered the computehidden logic which in a future patch triggers logic
>>>>> in the
>>>>> new hidden store which may start a transaction. This is circular and
>>>>> results in
>>>>> broken transaction mechanics, so let's use the unfiltered repo instead
>>>>> to
>>>>> avoid
>>>>> triggering computehidden during transaction start.
>>>>>
>>>>> This seems like how it should've been in the first place anyway since
>>>>> recording
>>>>> the length of the filtered changelog seems odd for a transaction.
>>>>>
>>>>> This was causing test failures in a future patch, so test coverage will
>>>>> come
>>>>> from the future patch.
>>>>
>>>>
>>>>
>>>> yes, writing filtered journal seems a bit odd. However I would rather see
>>>> the whole _writejournal run unfiltered.
>>>>
>>>> There are a '@unfilteredmethod' decorator for this purpose.
>>>
>>>
>>> That seems like a trivial little improvement for V2. There are no
>>> other calls in the method that seem like they would be affected, so it
>>> should be functionally equivalent as far as I can tell.
>>
>>
>> Yep, seems fine as a follow up.
>
> I have assumed there will be a v2 of this series anyway, so might as
> well do it right there (because it sounds like the decorator is
> preferred).

Ha yes, I forgot is was part of that series. That one patch is fairly 
independent and does that must probably be done in all case. Landing it 
early would be good.

Actually, I've just wrote such patch and emailed it.

Cheers,

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1123,7 +1123,7 @@  class localrepository(object):
         self.vfs.write("journal.branch",
                           encoding.fromlocal(self.dirstate.branch()))
         self.vfs.write("journal.desc",
-                          "%d\n%s\n" % (len(self), desc))
+                          "%d\n%s\n" % (len(self.unfiltered()), desc))
         self.vfs.write("journal.bookmarks",
                           self.vfs.tryread("bookmarks"))
         self.svfs.write("journal.phaseroots",