Patchwork [evolve-ext] touch: make sure the new noise number is different from the old one

login
register
mail settings
Submitter Jun Wu
Date April 2, 2016, 1:09 a.m.
Message ID <ed7b2174e2f41a432b50.1459559366@x1c>
Download mbox | patch
Permalink /patch/14247/
State Changes Requested
Headers show

Comments

Jun Wu - April 2, 2016, 1:09 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1459558346 -3600
#      Sat Apr 02 01:52:26 2016 +0100
# Node ID ed7b2174e2f41a432b5000d54f48d11387f41eba
# Parent  9ae4e79a28f3c09e76567f68be3d6f82e650e55b
touch: make sure the new noise number is different from the old one

Before this patch, there is no check to ensure the new touch noise is different
from the old one. It may cause issues in some situations where the generated
random numbers are the same. For example, when running with chg, the forked
request handler process will inherit the state of random from its parent, which
is always same if the parent process hasn't changed. Another example is when
debugging, people may want to unrandomize everything to get reproducible
results.

This patch addresses the issue by changing the way the new touch-noise is
calculated to make sure it will be different from the old one.

Another patch will make chg change random state per request.
Jun Wu - April 2, 2016, 1:15 a.m.
On 04/02/2016 02:09 AM, Jun Wu wrote:
> +            noise += random.randint(1, 0xffffffff) & 0xffffffff

Sorry but I was sleepy writing this. This line is intended to be:

             noise = (noise + random.randint(1, 0xffffffff)) & 0xffffffff

Or if we don't care about the length of "extra":

             noise += random.randint(1, 0xffffffff)
Pierre-Yves David - April 3, 2016, 8:46 p.m.
On 04/01/2016 06:09 PM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459558346 -3600
> #      Sat Apr 02 01:52:26 2016 +0100
> # Node ID ed7b2174e2f41a432b5000d54f48d11387f41eba
> # Parent  9ae4e79a28f3c09e76567f68be3d6f82e650e55b
> touch: make sure the new noise number is different from the old one
>
> Before this patch, there is no check to ensure the new touch noise is different
> from the old one. It may cause issues in some situations where the generated
> random numbers are the same. For example, when running with chg, the forked
> request handler process will inherit the state of random from its parent, which
> is always same if the parent process hasn't changed. Another example is when
> debugging, people may want to unrandomize everything to get reproducible
> results.
>
> This patch addresses the issue by changing the way the new touch-noise is
> calculated to make sure it will be different from the old one.
>
> Another patch will make chg change random state per request.

This lack of randomness issue is an issue. `hg touch` currently rely on 
this randomness to ensure it will succesfully revive changesets in all 
cases.

Let's look at the following scenario:

1) "abc00" is created (a changeset)
2) hg prune "abcde" makes it obsolete
3) hg --hidden touch "abcde" create "abc42" to revive it
4) hg amend try "abc42" into "abc43"
4) hg prune "abc42" makes the new version obsolete
5) hg --hidden touch "abcde" is called to bring the original changeset 
from the dead once again.

This last step need to use a different seed than the first time 
otherwise the result will be "abc42", and already obsolete changesets, 
defeating the purpose of the call.

(This apply accros computer too, so checking if "abc42" exists is not 
really on the table)


This changeset fix a 1 in 2³² bugs (so it is still useful) but hide this 
failure because the test are not checking this specific scenario.

Can you send a V2 with a test added for the still problematic case? I 
would like to make sure we can check it is fixed in chg.

Cheers,
Jun Wu - April 3, 2016, 11:01 p.m.
On 04/03/2016 09:46 PM, Pierre-Yves David wrote:
> This changeset fix a 1 in 2³² bugs (so it is still useful) but hide this
> failure because the test are not checking this specific scenario.

Yes. As discussed in IRC. This patch does not solve all the issues.
But fixing all of them seems to be impossible to do efficiently.

> Can you send a V2 with a test added for the still problematic case? I would
> like to make sure we can check it is fixed in chg.

Since it's impossible to solve the issue without adding notable overhead, I'm
going to drop this one. Then the existing test will catch chg randomness issue.

Patch

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -2990,7 +2990,9 @@ 
         for r in revs:
             ctx = repo[r]
             extra = ctx.extra().copy()
-            extra['__touch-noise__'] = random.randint(0, 0xffffffff)
+            noise = int(extra.get('__touch-noise__', 0))
+            noise += random.randint(1, 0xffffffff) & 0xffffffff
+            extra['__touch-noise__'] = str(noise)
             # search for touched parent
             p1 = ctx.p1().node()
             p2 = ctx.p2().node()