[PATCH] rev: C implementation of delta chain resolution

[Yuya Nishihara]

On Sun, 25 Jun 2017 12:42:53 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at gmail.com>
> # Date 1498419694 25200
> #      Sun Jun 25 12:41:34 2017 -0700
> # Node ID 9e98095e4daca1f22c70f5374d1110c81be98cc4
> # Parent  8e3021fd1a44e48a4720bb6fa4538fba399ad213
> rev: C implementation of delta chain resolution

Generally looks good to me as this is a direct reimplementation of Python
version.

> --- a/mercurial/cext/revlog.c
> +++ b/mercurial/cext/revlog.c
> @@ -816,6 +816,139 @@ bail:
>  	return NULL;
>  }
>  
> +static inline int index_baserev(indexObject *self, int rev)
> +{
> +	const char *data;
> +
> +	if (rev >= self->length - 1) {
> +		PyObject *tuple = PyList_GET_ITEM(self->added,
> +			rev - self->length + 1);
> +		return (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 3));
> +	}
> +	else {
> +		data = index_deref(self, rev);
> +		if (data == NULL) {
> +			return -2;

Since index_deref() sets MemoryError by itself, callers of index_baserev()
should just return NULL on -2.

> +static PyObject *index_deltachain(indexObject *self, PyObject *args)
> +{
> +	int rev, generaldelta;
> +	PyObject *stoparg;
> +	int stoprev, iterrev, baserev = -1;
> +	int stopped;
> +	PyObject *chain = NULL, *value = NULL, *result = NULL;
> +	const Py_ssize_t length = index_length(self);
> +
> +	if (!PyArg_ParseTuple(args, "iOi", &rev, &stoparg, &generaldelta)) {
> +		return NULL;
> +	}
> +
> +	if (PyInt_Check(stoparg)) {
> +		stoprev = (int)PyInt_AsLong(stoparg);
> +		if (stoprev == -1 && PyErr_Occurred()) {
> +			return NULL;
> +		}
> +	}
> +	else if (stoparg == Py_None) {
> +		stoprev = -2;
> +	}
> +	else {
> +		PyErr_SetString(PyExc_ValueError,
> +			"stoprev must be integer or None");
> +		return NULL;
> +	}
> +
> +	if (rev < 0 || rev >= length - 1) {
> +		PyErr_SetString(PyExc_ValueError, "revlog index out of range");
> +		return NULL;
> +	}
> +
> +	chain = PyList_New(0);
> +	if (chain == NULL) {
> +		return NULL;
> +	}
> +
> +	baserev = index_baserev(self, rev);
> +
> +	/* This should never happen. */
> +	if (baserev == -2) {
> +		PyErr_SetString(PyExc_IndexError, "unable to resolve data");
> +		goto bail;
> +	}
> +
> +	iterrev = rev;
> +
> +	while (iterrev != baserev && iterrev != stoprev) {
> +		value = PyInt_FromLong(iterrev);

Perhaps the scope of "value" could be narrowed as it isn't processed at the
global bail routine.

> +		if (value == NULL) {
> +			goto bail;
> +		}
> +		if (PyList_Append(chain, value)) {
> +			Py_DECREF(value);
> +			goto bail;
> +		}
> +		Py_DECREF(value);
> +
> +		if (generaldelta) {
> +			iterrev = baserev;
> +		}
> +		else {
> +			iterrev--;
> +		}
> +
> +		if (iterrev < 0) {
> +			break;
> +		}
> +
> +		if (iterrev >= length) {

Maybe 'iterrev >= length - 1'? index_baserev() seems not to handle the index
of the null entry, and the real null revision should be caught by the "break"
above.

> +			PyErr_SetString(PyExc_IndexError, "revision outside index");
> +			return NULL;
> +		}