D1351: changegroup: use any node, not min(), in treemanifest's generatemanifests

[martinvonz (Martin von Zweigbergk)]

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.

INLINE COMMENTS

> 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).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1351

To: spectral, #hg-reviewers
Cc: indygreg, martinvonz, mercurial-devel