| 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
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",
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
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,
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
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",