Patchwork [3,of,4] test-dirstate-race: ensure that a isn't in the lookup set at the end

login
register
mail settings
Submitter Siddharth Agarwal
Date June 10, 2017, 9:10 p.m.
Message ID <212b4b3a41a4edf86cde.1497129021@devvm31800.prn1.facebook.com>
Download mbox | patch
Permalink /patch/21318/
State Accepted
Headers show

Comments

Siddharth Agarwal - June 10, 2017, 9:10 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1497128851 25200
#      Sat Jun 10 14:07:31 2017 -0700
# Node ID 212b4b3a41a4edf86cdea2057bfa1c4d3f8a923f
# Parent  a69ae05fa66239c2aecb3673115de82105c8e9c6
test-dirstate-race: ensure that a isn't in the lookup set at the end

We're going to rely on this in upcoming patches.
via Mercurial-devel - June 12, 2017, 5:21 a.m.
On Sat, Jun 10, 2017 at 2:10 PM, Siddharth Agarwal <sid0@fb.com> wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1497128851 25200
> #      Sat Jun 10 14:07:31 2017 -0700
> # Node ID 212b4b3a41a4edf86cdea2057bfa1c4d3f8a923f
> # Parent  a69ae05fa66239c2aecb3673115de82105c8e9c6
> test-dirstate-race: ensure that a isn't in the lookup set at the end
>
> We're going to rely on this in upcoming patches.
>
> diff --git a/tests/test-dirstate-race.t b/tests/test-dirstate-race.t
> --- a/tests/test-dirstate-race.t
> +++ b/tests/test-dirstate-race.t
> @@ -156,4 +156,4 @@ treated differently in _checklookup() ac
>    $ hg files
>    a
>    $ hg debugdirstate
> -  n * * * a (glob)
> +  n 644          2 unset               a

Doesn't look like it's guaranteed to be unset. Running "run-tests.py
-j50 --runs-per-test=50 test-dirstate-race.t" seems to confirm that (2
of 5 failed for me). How do you rely on it in an upcoming patch?


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Siddharth Agarwal - June 12, 2017, 5:24 a.m.
On 6/11/17 10:21 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> Doesn't look like it's guaranteed to be unset. Running "run-tests.py
> -j50 --runs-per-test=50 test-dirstate-race.t" seems to confirm that (2
> of 5 failed for me). How do you rely on it in an upcoming patch?


Interesting, I'm not seeing that on my Linux box. Could you drop this 
for now? I think I can work around this problem.
via Mercurial-devel - June 12, 2017, 5:26 a.m.
On Sun, Jun 11, 2017 at 10:24 PM, Siddharth Agarwal <sid@less-broken.com> wrote:
> On 6/11/17 10:21 PM, Martin von Zweigbergk via Mercurial-devel wrote:
>>
>> Doesn't look like it's guaranteed to be unset. Running "run-tests.py
>> -j50 --runs-per-test=50 test-dirstate-race.t" seems to confirm that (2
>> of 5 failed for me). How do you rely on it in an upcoming patch?

Oops, that was meant to be "2 of *50* failed for me".

>
>
>
> Interesting, I'm not seeing that on my Linux box. Could you drop this for
> now? I think I can work around this problem.
>

Sure, will drop it.
Katsunori FUJIWARA - June 12, 2017, 10:04 a.m.
At Sun, 11 Jun 2017 22:24:20 -0700,
Siddharth Agarwal wrote:
> 
> On 6/11/17 10:21 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> > Doesn't look like it's guaranteed to be unset. Running "run-tests.py
> > -j50 --runs-per-test=50 test-dirstate-race.t" seems to confirm that (2
> > of 5 failed for me). How do you rely on it in an upcoming patch?
> 
> 
> Interesting, I'm not seeing that on my Linux box. Could you drop this 
> for now? I think I can work around this problem.

Unfortunately, this can occur, if mtime of "a" is N, and _getfsnow()
returns N+1 or more (= "a" is written out at almost N+1 but N in
"int").

IMHO, "hg update" with --config debug.dirstate.delaywrite=2 should
avoid this problem.


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Katsunori FUJIWARA - June 12, 2017, 10:31 a.m.
At Mon, 12 Jun 2017 19:04:28 +0900,
FUJIWARA Katsunori wrote:
> 
> At Sun, 11 Jun 2017 22:24:20 -0700,
> Siddharth Agarwal wrote:
> > 
> > On 6/11/17 10:21 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> > > Doesn't look like it's guaranteed to be unset. Running "run-tests.py
> > > -j50 --runs-per-test=50 test-dirstate-race.t" seems to confirm that (2
> > > of 5 failed for me). How do you rely on it in an upcoming patch?
> > 
> > 
> > Interesting, I'm not seeing that on my Linux box. Could you drop this 
> > for now? I think I can work around this problem.
> 
> Unfortunately, this can occur, if mtime of "a" is N, and _getfsnow()
> returns N+1 or more (= "a" is written out at almost N+1 but N in
> "int").
> 
> IMHO, "hg update" with --config debug.dirstate.delaywrite=2 should
> avoid this problem.

Oops, sorry, this is incorrect, because this is the way to ENSURE
mtime in dirstate :-<

IMHO, additional "touch -t YYYYmmddHHMM 'a'" and "hg status" with
tests/fakedirstatewritetime.py in previous $TESTTMP/dirstaterace.sh
can ensure "unset"-ness of mtime in dirstate in this
test-dirstate-race.t case.


> 
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> 
> -- 
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - June 12, 2017, 5:33 p.m.
On Sun, Jun 11, 2017 at 10:26 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Sun, Jun 11, 2017 at 10:24 PM, Siddharth Agarwal <sid@less-broken.com> wrote:
>> On 6/11/17 10:21 PM, Martin von Zweigbergk via Mercurial-devel wrote:
>>>
>>> Doesn't look like it's guaranteed to be unset. Running "run-tests.py
>>> -j50 --runs-per-test=50 test-dirstate-race.t" seems to confirm that (2
>>> of 5 failed for me). How do you rely on it in an upcoming patch?
>
> Oops, that was meant to be "2 of *50* failed for me".
>
>>
>>
>>
>> Interesting, I'm not seeing that on my Linux box. Could you drop this for
>> now? I think I can work around this problem.
>>
>
> Sure, will drop it.

I was too slow to rebase, review and accept the stack of changes, so
this is now public. Could you send a patch to back this out or change
it as Foozy suggests?

Patch

diff --git a/tests/test-dirstate-race.t b/tests/test-dirstate-race.t
--- a/tests/test-dirstate-race.t
+++ b/tests/test-dirstate-race.t
@@ -156,4 +156,4 @@  treated differently in _checklookup() ac
   $ hg files
   a
   $ hg debugdirstate
-  n * * * a (glob)
+  n 644          2 unset               a