Patchwork revlog: address review feedback for deltachain C implementation

login
register
mail settings
Submitter Gregory Szorc
Date July 2, 2017, 2:42 a.m.
Message ID <6f7c332d8956aea59498.1498963324@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/21877/
State Accepted
Headers show

Comments

Gregory Szorc - July 2, 2017, 2:42 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1498962917 25200
#      Sat Jul 01 19:35:17 2017 -0700
# Node ID 6f7c332d8956aea59498e94c2dd3536530772040
# Parent  6d678ab1b10d0fddc73003d21aa3c7ec43194e2e
revlog: address review feedback for deltachain C implementation

* Scope of "value" is reduced
* index_baserev() is documented
* Error is no longer redundantly set for -2 return values
* Error values are compared <= -2 instead of == -2 to protect
  against odd failure scenarios
Yuya Nishihara - July 2, 2017, 1:08 p.m.
On Sat, 01 Jul 2017 19:42:04 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1498962917 25200
> #      Sat Jul 01 19:35:17 2017 -0700
> # Node ID 6f7c332d8956aea59498e94c2dd3536530772040
> # Parent  6d678ab1b10d0fddc73003d21aa3c7ec43194e2e
> revlog: address review feedback for deltachain C implementation

Queued, thanks.

>  	/* This should never happen. */
> -	if (baserev == -2) {
> -		PyErr_SetString(PyExc_IndexError, "unable to resolve data");
> +	if (baserev <= -2) {
> +		/* Error should be set by index_deref() */
> +		assert(PyErr_Occurred());

added #include <assert.h> in flight.

Patch

diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -816,6 +816,11 @@  bail:
 	return NULL;
 }
 
+/**
+ * Obtain the base revision index entry.
+ *
+ * Callers must ensure that rev >= 0 or illegal memory access may occur.
+ */
 static inline int index_baserev(indexObject *self, int rev)
 {
 	const char *data;
@@ -841,7 +846,7 @@  static PyObject *index_deltachain(indexO
 	PyObject *stoparg;
 	int stoprev, iterrev, baserev = -1;
 	int stopped;
-	PyObject *chain = NULL, *value = NULL, *result = NULL;
+	PyObject *chain = NULL, *result = NULL;
 	const Py_ssize_t length = index_length(self);
 
 	if (!PyArg_ParseTuple(args, "iOi", &rev, &stoparg, &generaldelta)) {
@@ -876,15 +881,16 @@  static PyObject *index_deltachain(indexO
 	baserev = index_baserev(self, rev);
 
 	/* This should never happen. */
-	if (baserev == -2) {
-		PyErr_SetString(PyExc_IndexError, "unable to resolve data");
+	if (baserev <= -2) {
+		/* Error should be set by index_deref() */
+		assert(PyErr_Occurred());
 		goto bail;
 	}
 
 	iterrev = rev;
 
 	while (iterrev != baserev && iterrev != stoprev) {
-		value = PyInt_FromLong(iterrev);
+		PyObject *value = PyInt_FromLong(iterrev);
 		if (value == NULL) {
 			goto bail;
 		}
@@ -913,8 +919,9 @@  static PyObject *index_deltachain(indexO
 		baserev = index_baserev(self, iterrev);
 
 		/* This should never happen. */
-		if (baserev == -2) {
-			PyErr_SetString(PyExc_IndexError, "unable to resolve data");
+		if (baserev <= -2) {
+			/* Error should be set by index_deref() */
+			assert(PyErr_Occurred());
 			goto bail;
 		}
 	}
@@ -923,7 +930,7 @@  static PyObject *index_deltachain(indexO
 		stopped = 1;
 	}
 	else {
-		value = PyInt_FromLong(iterrev);
+		PyObject *value = PyInt_FromLong(iterrev);
 		if (value == NULL) {
 			goto bail;
 		}