D1351: changegroup: use any node, not min(), in treemanifest's generatemanifests
martinvonz (Martin von Zweigbergk)
phabricator at mercurial-scm.org
Fri Nov 10 00:41:33 EST 2017
martinvonz added a comment.
In https://phab.mercurial-scm.org/D1351#22473, @indygreg wrote:
> Should I be concerned about the lack of test fallout? This new behavior is non-deterministic. Do we not have testing for this code or is existing testing not low-level enough to uncover behavior changes resulting from this change?
I'm not worried about it. It only makes a difference for treemanifests, and the change is just about the order in changegroup and (therefore) the order in which we write revlogs. We don't have much testing of treemanifests and we rarely check the exact format in a changegroup. We do print out a debug message on line 482 that could be used to see a difference before and after this patch, but I just checked that test-treemanifest.t doesn't pass --verbose. Still, I wouldn't mind if some Facebooker tried to run their treemanifest tests (in hgexperimental) against a version with this patch applied.
> indygreg wrote in changegroup.py:738
> Can we use ``dict.popitem()`` instead? That will pop a random key-value pair. I just don't know if the key needs to remain in the dict until later in the function...
Good idea. I'm pretty sure that should be safe (but perhaps tests will prove me wrong).
To: spectral, #hg-reviewers
Cc: indygreg, martinvonz, mercurial-devel
More information about the Mercurial-devel