( )⚙ D7320 revlog: clean up the node of all revision stripped in the C code

This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by marmoute on Nov 8 2019, 4:29 AM.

Details

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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

marmoute created this revision.Nov 8 2019, 4:29 AM
indygreg requested changes to this revision.Nov 8 2019, 2:40 PM
indygreg added a subscriber: indygreg.

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.

This revision now requires changes to proceed.Nov 8 2019, 2:40 PM

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 and 52e9bf215f96. 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.

marmoute requested review of this revision.Nov 8 2019, 7:28 PM

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 crash horribly, and debugging show that the value left out is clearly a non-sentinel value that should be deleted.

With this change, D7321 pass all test flawlessly.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.