Submitter | phabricator |
---|---|
Date | Nov. 8, 2019, 9:31 a.m. |
Message ID | <differential-rev-PHID-DREV-74qbug45qcfc46t4y3ue-req@mercurial-scm.org> |
Download | mbox | patch |
Permalink | /patch/42916/ |
State | Superseded |
Headers | show |
Comments
This revision now requires changes to proceed. indygreg added a comment. indygreg requested changes to this revision. The first element in the index is a sentinel entry for `nullid`. I suspect the `+1` here is meant to ignore that sentinel. The comment for this function clearly calls out this behavior. And I I'm pretty sure this change could result in the sentinel being deleted. So either drop this patch, catch the special case of deleting the sentinel entry, or convince me I'm wrong in my claim that this patch is changing behavior. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7320/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7320 To: marmoute, #hg-reviewers, indygreg Cc: indygreg, mercurial-devel
martinvonz added a comment. In D7320#107577 <https://phab.mercurial-scm.org/D7320#107577>, @indygreg wrote: > The first element in the index is a sentinel entry for `nullid`. I suspect the `+1` here is meant to ignore that sentinel. The comment for this function clearly calls out this behavior. And I I'm pretty sure this change could result in the sentinel being deleted. > So either drop this patch, catch the special case of deleting the sentinel entry, or convince me I'm wrong in my claim that this patch is changing behavior. I think `nullid` entry used to be at the end until I changed that in 781b2720d2ac0 <https://phab.mercurial-scm.org/rHG781b2720d2ac005e2806b46d8cb91abacfe4b901> and 52e9bf215f96 <https://phab.mercurial-scm.org/rHG52e9bf215f96bf3432d9e4b5585396a1d23fa6c1>. I should have updated the comment in one of those patches. I don't see how `start + 1` would have been about `nullid`, but I also don't have any other idea what it could be about. I think it looks like it was just a bug. Btw, note that I changed `self->raw_length = start + 1;` to `self->raw_length = start;` in the second of those commits. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7320/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7320 To: marmoute, #hg-reviewers, indygreg Cc: martinvonz, indygreg, mercurial-devel
marmoute added a comment. marmoute requested review of this revision. In D7320#107577 <https://phab.mercurial-scm.org/D7320#107577>, @indygreg wrote: > The first element in the index is a sentinel entry for `nullid`. I suspect the `+1` here is meant to ignore that sentinel. The comment for this function clearly calls out this behavior. And I I'm pretty sure this change could result in the sentinel being deleted. > So either drop this patch, catch the special case of deleting the sentinel entry, or convince me I'm wrong in my claim that this patch is changing behavior. Without this change, D7321 <https://phab.mercurial-scm.org/D7321> crash horribly, and debugging show that the value left out is clearly a non-sentinel value that should be deleted. With this change, D7321 <https://phab.mercurial-scm.org/D7321> pass all test flawlessly. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7320/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7320 To: marmoute, #hg-reviewers, indygreg Cc: martinvonz, indygreg, mercurial-devel
Patch
diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c --- a/mercurial/cext/revlog.c +++ b/mercurial/cext/revlog.c @@ -2489,7 +2489,7 @@ if (self->ntinitialized) { Py_ssize_t i; - for (i = start + 1; i < self->length; i++) { + for (i = start; i < self->length; i++) { const char *node = index_node_existing(self, i); if (node == NULL) return -1;