Patchwork D7320: revlog: clean up the node of all revision stripped in the C code

login
register
mail settings
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

phabricator - Nov. 8, 2019, 9:31 a.m.
marmoute created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  For some obscure reason, the loop cleaning up node was skipping the first
  element… I cannot see a reason for it. The overall code is running fine
  nevertheless because the node are also explicitly deleted from python.
  
  We want to delete this explicit deletion, so we need to fix that code first.
  
  This work is part of a refactoring to unify the revlog index and the nodemap.
  This unification prepare the use of a persistent nodemap.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/cext/revlog.c

CHANGE DETAILS




To: marmoute, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 8, 2019, 7:40 p.m.
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
phabricator - Nov. 8, 2019, 10:31 p.m.
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
phabricator - Nov. 9, 2019, 12:28 a.m.
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;