Page MenuHomePhabricator

index: don't include nullid in len()
ClosedPublic

Authored by martinvonz on Aug 1 2018, 2:47 PM.

Details

Summary

I suspect the reason the nullid is in the index in the last position
is that it lets index[i] for regular revision number, even when index
was just a regular Python list. An alternative solution would have
been to reserve revision number 0 for the null revision. I don't know
why that wasn't done. Now that we have classes backing the index, we
can easily make index[-1] get the nullid without having to put it last
in the list and including it in the len().

This patch just hides the nullid -- it will still be accessible at
index[len(index)].

I realize that this will be annoying when checking out across this
commit for debugging (including bisection).

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Aug 1 2018, 2:47 PM
martinvonz updated this revision to Diff 9781.Aug 2 2018, 12:40 AM
martinvonz updated this revision to Diff 9885.Aug 4 2018, 12:20 AM
yuja added a subscriber: yuja.Aug 4 2018, 2:42 AM
	if (PyInt_Check(value)) {
		long rev = PyInt_AS_LONG(value);
  • return rev >= -1 && rev < index_length(self);

+ return rev >= -1 && rev < index_length(self) - 1;

Fixed this to index_length(self) + 1. Maybe you'll want to drop + 1 by
a later patch, but excluding the tip rev is clearly wrong.

This revision was automatically updated to reflect the committed changes.
In D4022#63500, @yuja wrote:
	if (PyInt_Check(value)) {
		long rev = PyInt_AS_LONG(value);
  • return rev >= -1 && rev < index_length(self);

+ return rev >= -1 && rev < index_length(self) - 1;

Fixed this to index_length(self) + 1. Maybe you'll want to drop + 1 by
a later patch, but excluding the tip rev is clearly wrong.

Thanks for careful review (as always). Yes, I had meant to clean up the "+1" and "-1" this patch sprinkled across the code, but thanks for the reminder because I had forgotten about that.