| 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
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)
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,
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()