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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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.