This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7320

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

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

Download Raw Diff

Details

Reviewers

indygreg

Group Reviewers

hg-reviewers

Commits

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

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptNov 8 2019, 4:29 AM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

marmoute added a child revision: D7321: revlog: deal with nodemap deletion within the index.Nov 8 2019, 4:29 AM

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

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

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

marmoute added a commit: rHG67e15214213b: revlog: clean up the node of all revision stripped in the C code.Nov 8 2019, 8:56 PM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHG67e15214213b: revlog: clean up the node of all revision stripped in the C code (authored by marmoute). · Explain Why

This revision was automatically updated to reflect the committed changes.

martinvonz mentioned this in D7321: revlog: deal with nodemap deletion within the index.Nov 9 2019, 1:58 AM

marmoute added a commit: rHG53581e220ba3: revlog: clean up the node of all revision stripped in the C code.Nov 10 2019, 10:37 AM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/cext/revlog.c (2 lines)

Commit	Parents	Author	Summary	Date
c8aded473183	2dc431994b4a	Pierre-Yves David		Nov 8 2019, 4:01 AM

Status	Author	Revision
Closed	marmoute	D7362 revlog: deprecate direct `nodemap` access
Closed	marmoute	D7361 revlog: drop special hack in the `nodemap` property
Closed	marmoute	D7360 perf: make `perfrevlogindex` use the new `index.rev` api if available
Closed	marmoute	D7359 perf: make `perfphasesremote` use the new `index.has_node` api
Closed	marmoute	D7358 perf: make `perfnodemap` use the new `index.get_rev` api if available
Closed	marmoute	D7357 index: use `index.get_rev` in `remotefilelog`
Closed	marmoute	D7356 index: use `index.has_node` in `infinitypush`
Closed	marmoute	D7355 index: use `index.has_node` in `phabricator.getoldnodedrevmap`
Closed	marmoute	D7354 index: use `index.get_rev` in `histedit.processreplacement`
Closed	marmoute	D7353 index: use `index.get_rev` in `histedit.adjustreplacementsfrommarkers`
Closed	marmoute	D7352 index: use `index.has_node` in `histedit.cleanupnode`
Closed	marmoute	D7351 index: use `index.has_node` in `histedit._finishhistedit
Closed	marmoute	D7350 index: use `index.get_rev` in `rebase._computeobsoletenotrebased`
Closed	marmoute	D7349 index: use `index.get_rev` in `rebase.successorrevs`
Closed	marmoute	D7339 index: use `index.has_node` in `bundlerepo.bundlerevlog`
Closed	marmoute	D7338 index: use `index.get_rev` in `unionrepo.unionrevlog`
Closed	marmoute	D7340 index: use `index.rev` in `bundlerepo.bundlerevlog`
Closed	marmoute	D7348 index: use `index.has_node` in `exchangev2._pullchangesetdiscovery`
Closed	marmoute	D7347 index: use `index.get_rev` in `exchange._pullapplyphases`
Closed	marmoute	D7346 index: use `index.has_node` in `exchange._pulldiscoverychangegroup`
Closed	marmoute	D7345 index: use `index.rev` in `exchange.fallbackheads`
Closed	marmoute	D7344 index: use `index.has_node` in `bookmarks.bmstore`
Closed	marmoute	D7343 index: use `index.get_rev` in `revset._mapbynodefunc`
Closed	marmoute	D7342 index: use `index.has_node` in `tags.findglobaltags`
Closed	marmoute	D7341 index: use `index.get_rev` in `discovery._postprocessobsolete`
Closed	marmoute	D7337 index: use `index.get_rev` in `repoview.pinnedrevs`
Closed	marmoute	D7336 index: use `index.get_rev` in `obsolete._computephasedivergentset`
Closed	marmoute	D7335 index: use `index.get_rev` in `obsutil.getobsoleted`
Closed	marmoute	D7331 index: use `index.has_node` in `phases.filterunknown`
Closed	marmoute	D7330 index: use `index.has_node` in `analyzeremotephases`
Closed	marmoute	D7329 index: use `index.has_node` in `repo._rollback`
Closed	marmoute	D7328 index: use `index.has_node` in `revlog.addgroup`
Closed	marmoute	D7334 index: use `index.has_node` in `obsutil.foreground`
Closed	marmoute	D7333 index: use `index.has_node` in `obsutil.exclusivemarkers`
Closed	marmoute	D7332 index: use `index.get_rev` in `phases.newheads`
Closed	marmoute	D7327 index: use `index.get_rev` in `localrepo.known`
Closed	marmoute	D7326 index: add a `get_rev` method (API)
Closed	marmoute	D7325 index: use `index.rev` in `revlog.rev`
Closed	marmoute	D7324 index: add a `rev` method (API)
Closed	marmoute	D7323 index: use `index.has_node` in `revlog.addrevision`
Closed	marmoute	D7322 index: add a `has_node` method (API)
Closed	marmoute	D7321 revlog: deal with nodemap deletion within the index
Closed	marmoute	D7320 revlog: clean up the node of all revision stripped in the C code
Closed	marmoute	D7319 revlog: move nodemap update within the index code
Closed	marmoute	D7318 revlog: deprecate the _nodecache attribute (API)
Closed	marmoute	D7317 revlog: access the nodemap through the index
Closed	marmoute	D7316 revlog: return the nodemap as the nodecache
Closed	marmoute	D7315 revlog: clarify which version use the older API in perf
Closed	marmoute	D7314 revlog: no longer return the nodemap after parsing
Closed	marmoute	D7313 revlog: move the nodemap into the index object (for pure)
Closed	marmoute	D7312 revlog: introduce an explicit NodeMap class for pure code
Closed	marmoute	D7311 utils: move the `dirs` definition in pathutil (API)

Diff 17736

mercurial/cext/revlog.c

	"revlog index deletion indices are invalid");			"revlog index deletion indices are invalid");
	return -1;			return -1;
	}			}

	if (start < self->length) {			if (start < self->length) {
	if (self->ntinitialized) {			if (self->ntinitialized) {
	Py_ssize_t i;			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);			const char *node = index_node_existing(self, i);
	if (node == NULL)			if (node == NULL)
	return -1;			return -1;

	nt_delete_node(&self->nt, node);			nt_delete_node(&self->nt, node);
	}			}
	if (self->added)			if (self->added)
	index_invalidate_added(self, 0);			index_invalidate_added(self, 0);

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	17736		Nov 8 2019, 4:29 AM	★	★
Diff 2	17833	rHG67e15214213b46f483bb2827a522d46b3142e61f	Nov 8 2019, 4:01 AM	★	★