Patchwork [1,of,3,v3] mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch

login
register
mail settings
Submitter Maciej Fijalkowski
Date July 4, 2016, 9:16 a.m.
Message ID <5c7e2b441d644a544c8f.1467623783@brick.arcode.com>
Download mbox | patch
Permalink /patch/15730/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Maciej Fijalkowski - July 4, 2016, 9:16 a.m.
# HG changeset patch
# User Maciej Fijalkowski <fijall@gmail.com>
# Date 1465303444 -7200
#      Tue Jun 07 14:44:04 2016 +0200
# Node ID 5c7e2b441d644a544c8fd26dfa3d0d50cb966307
# Parent  3c349a3d45b909dbad0506365344ee7e1471026a
mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch

This is a commit that just shuffles code around so the C code in mpatch
and the mpatch module is completely separate
Yuya Nishihara - July 6, 2016, 2:19 p.m.
On Mon, 04 Jul 2016 11:16:23 +0200, Maciej Fijalkowski wrote:
> # HG changeset patch
> # User Maciej Fijalkowski <fijall@gmail.com>
> # Date 1465303444 -7200
> #      Tue Jun 07 14:44:04 2016 +0200
> # Node ID 5c7e2b441d644a544c8fd26dfa3d0d50cb966307
> # Parent  3c349a3d45b909dbad0506365344ee7e1471026a
> mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch

> --- a/mercurial/compat.h	Mon Jun 06 13:08:13 2016 +0200
> +++ b/mercurial/compat.h	Tue Jun 07 14:44:04 2016 +0200
> @@ -35,4 +35,8 @@
>  #define inline __inline
>  #endif
>  
> +#ifndef ssize_t
> +#define ssize_t size_t
>  #endif

Wrong.

 - ssize_t isn't a macro. #ifndef can't be used
 - ssize_t should be signed, but size_t is unsigned

Can you take a look how CPython handle the definition of Py_ssize_t?

> --- a/mercurial/mpatch.c	Mon Jun 06 13:08:13 2016 +0200
> +++ b/mercurial/mpatch.c	Tue Jun 07 14:44:04 2016 +0200

> +static struct mpatch_flist *lalloc(ssize_t size)
>  {
> -	struct flist *a = NULL;
> +	struct mpatch_flist *a = NULL;
>  
>  	if (size < 1)
>  		size = 1;
>  
> -	a = (struct flist *)malloc(sizeof(struct flist));
> +	a = (struct mpatch_flist *)malloc(sizeof(struct mpatch_flist));
>  	if (a) {
> -		a->base = (struct frag *)malloc(sizeof(struct frag) * size);
> +		a->base = (struct mpatch_frag *)malloc(sizeof(struct mpatch_frag) *
> +			size);
>  		if (a->base) {
>  			a->head = a->tail = a->base;
>  			return a;
> @@ -57,12 +47,10 @@
>  		free(a);
>  		a = NULL;
>  	}
> -	if (!PyErr_Occurred())
> -		PyErr_NoMemory();

Now it's the responsibility of the caller to set PyErr_NoMemory, but

> +struct mpatch_flist *cpygetnextitem(void *bins, ssize_t pos)
> +{
> +	const char *buffer;
> +	struct mpatch_flist *res;
> +	ssize_t blen;
> +	int r;
> +
> +	PyObject *tmp = PyList_GetItem((PyObject*)bins, pos);
> +	if (!tmp)
> +		return NULL;
> +	if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t*)&blen))
> +		return NULL;
> +	if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
> +		if (!PyErr_Occurred())
> +			PyErr_SetString(mpatch_Error, mpatch_errors[-r]);
> +		return NULL;
> +	}
> +	return res;
> +}

new fold() sets mpatch_Error instead,

> +static PyObject *
> +patches(PyObject *self, PyObject *args)
> +{
> +	PyObject *text, *bins, *result;
> +	struct mpatch_flist *patch;
> +	const char *in;
> +	char *out;
> +	Py_ssize_t len, outlen, inlen;
> +
> +	if (!PyArg_ParseTuple(args, "OO:mpatch", &text, &bins))
> +		return NULL;
> +
> +	len = PyList_Size(bins);
> +	if (!len) {
> +		/* nothing to do */
> +		Py_INCREF(text);
> +		return text;
> +	}
> +
> +	if (PyObject_AsCharBuffer(text, &in, &inlen))
> +		return NULL;
> +
> +	patch = mpatch_fold(bins, cpygetnextitem, 0, len);
> +	if (!patch) {
> +		return NULL;

or return NULL without setting any exception.

I haven't read the code other than lalloc(). I'll need more time to concentrate
on reviewing this. To reviewers, feel free to beat me if you have time.

And again, I'm not a fan of mixing code moves, naming changes, design changes,
etc. into one patch just because they are things necessary for the cffi mpatch.
Sean Farley - July 6, 2016, 6:03 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Mon, 04 Jul 2016 11:16:23 +0200, Maciej Fijalkowski wrote:
>> # HG changeset patch
>> # User Maciej Fijalkowski <fijall@gmail.com>
>> # Date 1465303444 -7200
>> #      Tue Jun 07 14:44:04 2016 +0200
>> # Node ID 5c7e2b441d644a544c8fd26dfa3d0d50cb966307
>> # Parent  3c349a3d45b909dbad0506365344ee7e1471026a
>> mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch
>
>> --- a/mercurial/compat.h	Mon Jun 06 13:08:13 2016 +0200
>> +++ b/mercurial/compat.h	Tue Jun 07 14:44:04 2016 +0200
>> @@ -35,4 +35,8 @@
>>  #define inline __inline
>>  #endif
>>  
>> +#ifndef ssize_t
>> +#define ssize_t size_t
>>  #endif
>
> Wrong.
>
>  - ssize_t isn't a macro. #ifndef can't be used
>  - ssize_t should be signed, but size_t is unsigned
>
> Can you take a look how CPython handle the definition of Py_ssize_t?
>
>> --- a/mercurial/mpatch.c	Mon Jun 06 13:08:13 2016 +0200
>> +++ b/mercurial/mpatch.c	Tue Jun 07 14:44:04 2016 +0200
>
>> +static struct mpatch_flist *lalloc(ssize_t size)
>>  {
>> -	struct flist *a = NULL;
>> +	struct mpatch_flist *a = NULL;
>>  
>>  	if (size < 1)
>>  		size = 1;
>>  
>> -	a = (struct flist *)malloc(sizeof(struct flist));
>> +	a = (struct mpatch_flist *)malloc(sizeof(struct mpatch_flist));
>>  	if (a) {
>> -		a->base = (struct frag *)malloc(sizeof(struct frag) * size);
>> +		a->base = (struct mpatch_frag *)malloc(sizeof(struct mpatch_frag) *
>> +			size);
>>  		if (a->base) {
>>  			a->head = a->tail = a->base;
>>  			return a;
>> @@ -57,12 +47,10 @@
>>  		free(a);
>>  		a = NULL;
>>  	}
>> -	if (!PyErr_Occurred())
>> -		PyErr_NoMemory();
>
> Now it's the responsibility of the caller to set PyErr_NoMemory, but
>
>> +struct mpatch_flist *cpygetnextitem(void *bins, ssize_t pos)
>> +{
>> +	const char *buffer;
>> +	struct mpatch_flist *res;
>> +	ssize_t blen;
>> +	int r;
>> +
>> +	PyObject *tmp = PyList_GetItem((PyObject*)bins, pos);
>> +	if (!tmp)
>> +		return NULL;
>> +	if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t*)&blen))
>> +		return NULL;
>> +	if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
>> +		if (!PyErr_Occurred())
>> +			PyErr_SetString(mpatch_Error, mpatch_errors[-r]);
>> +		return NULL;
>> +	}
>> +	return res;
>> +}
>
> new fold() sets mpatch_Error instead,
>
>> +static PyObject *
>> +patches(PyObject *self, PyObject *args)
>> +{
>> +	PyObject *text, *bins, *result;
>> +	struct mpatch_flist *patch;
>> +	const char *in;
>> +	char *out;
>> +	Py_ssize_t len, outlen, inlen;
>> +
>> +	if (!PyArg_ParseTuple(args, "OO:mpatch", &text, &bins))
>> +		return NULL;
>> +
>> +	len = PyList_Size(bins);
>> +	if (!len) {
>> +		/* nothing to do */
>> +		Py_INCREF(text);
>> +		return text;
>> +	}
>> +
>> +	if (PyObject_AsCharBuffer(text, &in, &inlen))
>> +		return NULL;
>> +
>> +	patch = mpatch_fold(bins, cpygetnextitem, 0, len);
>> +	if (!patch) {
>> +		return NULL;
>
> or return NULL without setting any exception.
>
> I haven't read the code other than lalloc(). I'll need more time to concentrate
> on reviewing this. To reviewers, feel free to beat me if you have time.
>
> And again, I'm not a fan of mixing code moves, naming changes, design changes,
> etc. into one patch just because they are things necessary for the cffi mpatch.

Thanks to Yuya for trying to review this.

To be honest, I'm fairly put off by the patch author insisting on mixing
code and design changes. This is at least the third time I've seen an
error when trying to do both. Which is a problem because it exhausts too
much of a reviewer's time.

So, I'm asking: please, please, with a cherry on top, split up your
patches for review.
Maciej Fijalkowski - July 7, 2016, 7:02 a.m.
So how do you guys want this to be split?

On Wed, Jul 6, 2016 at 8:03 PM, Sean Farley <sean@farley.io> wrote:
> Yuya Nishihara <yuya@tcha.org> writes:
>
>> On Mon, 04 Jul 2016 11:16:23 +0200, Maciej Fijalkowski wrote:
>>> # HG changeset patch
>>> # User Maciej Fijalkowski <fijall@gmail.com>
>>> # Date 1465303444 -7200
>>> #      Tue Jun 07 14:44:04 2016 +0200
>>> # Node ID 5c7e2b441d644a544c8fd26dfa3d0d50cb966307
>>> # Parent  3c349a3d45b909dbad0506365344ee7e1471026a
>>> mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch
>>
>>> --- a/mercurial/compat.h     Mon Jun 06 13:08:13 2016 +0200
>>> +++ b/mercurial/compat.h     Tue Jun 07 14:44:04 2016 +0200
>>> @@ -35,4 +35,8 @@
>>>  #define inline __inline
>>>  #endif
>>>
>>> +#ifndef ssize_t
>>> +#define ssize_t size_t
>>>  #endif
>>
>> Wrong.
>>
>>  - ssize_t isn't a macro. #ifndef can't be used
>>  - ssize_t should be signed, but size_t is unsigned
>>
>> Can you take a look how CPython handle the definition of Py_ssize_t?
>>
>>> --- a/mercurial/mpatch.c     Mon Jun 06 13:08:13 2016 +0200
>>> +++ b/mercurial/mpatch.c     Tue Jun 07 14:44:04 2016 +0200
>>
>>> +static struct mpatch_flist *lalloc(ssize_t size)
>>>  {
>>> -    struct flist *a = NULL;
>>> +    struct mpatch_flist *a = NULL;
>>>
>>>      if (size < 1)
>>>              size = 1;
>>>
>>> -    a = (struct flist *)malloc(sizeof(struct flist));
>>> +    a = (struct mpatch_flist *)malloc(sizeof(struct mpatch_flist));
>>>      if (a) {
>>> -            a->base = (struct frag *)malloc(sizeof(struct frag) * size);
>>> +            a->base = (struct mpatch_frag *)malloc(sizeof(struct mpatch_frag) *
>>> +                    size);
>>>              if (a->base) {
>>>                      a->head = a->tail = a->base;
>>>                      return a;
>>> @@ -57,12 +47,10 @@
>>>              free(a);
>>>              a = NULL;
>>>      }
>>> -    if (!PyErr_Occurred())
>>> -            PyErr_NoMemory();
>>
>> Now it's the responsibility of the caller to set PyErr_NoMemory, but
>>
>>> +struct mpatch_flist *cpygetnextitem(void *bins, ssize_t pos)
>>> +{
>>> +    const char *buffer;
>>> +    struct mpatch_flist *res;
>>> +    ssize_t blen;
>>> +    int r;
>>> +
>>> +    PyObject *tmp = PyList_GetItem((PyObject*)bins, pos);
>>> +    if (!tmp)
>>> +            return NULL;
>>> +    if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t*)&blen))
>>> +            return NULL;
>>> +    if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
>>> +            if (!PyErr_Occurred())
>>> +                    PyErr_SetString(mpatch_Error, mpatch_errors[-r]);
>>> +            return NULL;
>>> +    }
>>> +    return res;
>>> +}
>>
>> new fold() sets mpatch_Error instead,
>>
>>> +static PyObject *
>>> +patches(PyObject *self, PyObject *args)
>>> +{
>>> +    PyObject *text, *bins, *result;
>>> +    struct mpatch_flist *patch;
>>> +    const char *in;
>>> +    char *out;
>>> +    Py_ssize_t len, outlen, inlen;
>>> +
>>> +    if (!PyArg_ParseTuple(args, "OO:mpatch", &text, &bins))
>>> +            return NULL;
>>> +
>>> +    len = PyList_Size(bins);
>>> +    if (!len) {
>>> +            /* nothing to do */
>>> +            Py_INCREF(text);
>>> +            return text;
>>> +    }
>>> +
>>> +    if (PyObject_AsCharBuffer(text, &in, &inlen))
>>> +            return NULL;
>>> +
>>> +    patch = mpatch_fold(bins, cpygetnextitem, 0, len);
>>> +    if (!patch) {
>>> +            return NULL;
>>
>> or return NULL without setting any exception.
>>
>> I haven't read the code other than lalloc(). I'll need more time to concentrate
>> on reviewing this. To reviewers, feel free to beat me if you have time.
>>
>> And again, I'm not a fan of mixing code moves, naming changes, design changes,
>> etc. into one patch just because they are things necessary for the cffi mpatch.
>
> Thanks to Yuya for trying to review this.
>
> To be honest, I'm fairly put off by the patch author insisting on mixing
> code and design changes. This is at least the third time I've seen an
> error when trying to do both. Which is a problem because it exhausts too
> much of a reviewer's time.
>
> So, I'm asking: please, please, with a cherry on top, split up your
> patches for review.
Jun Wu - July 7, 2016, 12:34 p.m.
Usually, the first patch will be moving code without changing logic
(including renaming a variable). The second patch will be changing lines
without moving code. The point is to make "diff" output more friendly to
review. See 616ea95c8f11 as an example. It's not the exactly the same with
your case but you can get the idea.

You can think this as a limitation caused by the stupid "diff" output, not
smart enough to match chunks between different files. But I wonder what a
smarter one will look like.

Excerpts from Maciej Fijalkowski's message of 2016-07-07 09:02:43 +0200:
> So how do you guys want this to be split?
> 
> On Wed, Jul 6, 2016 at 8:03 PM, Sean Farley <sean@farley.io> wrote:
> > Yuya Nishihara <yuya@tcha.org> writes:
> >
> >> On Mon, 04 Jul 2016 11:16:23 +0200, Maciej Fijalkowski wrote:
> >>> # HG changeset patch
> >>> # User Maciej Fijalkowski <fijall@gmail.com>
> >>> # Date 1465303444 -7200
> >>> #      Tue Jun 07 14:44:04 2016 +0200
> >>> # Node ID 5c7e2b441d644a544c8fd26dfa3d0d50cb966307
> >>> # Parent  3c349a3d45b909dbad0506365344ee7e1471026a
> >>> mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch
> >>
> >>> --- a/mercurial/compat.h     Mon Jun 06 13:08:13 2016 +0200
> >>> +++ b/mercurial/compat.h     Tue Jun 07 14:44:04 2016 +0200
> >>> @@ -35,4 +35,8 @@
> >>>  #define inline __inline
> >>>  #endif
> >>>
> >>> +#ifndef ssize_t
> >>> +#define ssize_t size_t
> >>>  #endif
> >>
> >> Wrong.
> >>
> >>  - ssize_t isn't a macro. #ifndef can't be used
> >>  - ssize_t should be signed, but size_t is unsigned
> >>
> >> Can you take a look how CPython handle the definition of Py_ssize_t?
> >>
> >>> --- a/mercurial/mpatch.c     Mon Jun 06 13:08:13 2016 +0200
> >>> +++ b/mercurial/mpatch.c     Tue Jun 07 14:44:04 2016 +0200
> >>
> >>> +static struct mpatch_flist *lalloc(ssize_t size)
> >>>  {
> >>> -    struct flist *a = NULL;
> >>> +    struct mpatch_flist *a = NULL;
> >>>
> >>>      if (size < 1)
> >>>              size = 1;
> >>>
> >>> -    a = (struct flist *)malloc(sizeof(struct flist));
> >>> +    a = (struct mpatch_flist *)malloc(sizeof(struct mpatch_flist));
> >>>      if (a) {
> >>> -            a->base = (struct frag *)malloc(sizeof(struct frag) * size);
> >>> +            a->base = (struct mpatch_frag *)malloc(sizeof(struct mpatch_frag) *
> >>> +                    size);
> >>>              if (a->base) {
> >>>                      a->head = a->tail = a->base;
> >>>                      return a;
> >>> @@ -57,12 +47,10 @@
> >>>              free(a);
> >>>              a = NULL;
> >>>      }
> >>> -    if (!PyErr_Occurred())
> >>> -            PyErr_NoMemory();
> >>
> >> Now it's the responsibility of the caller to set PyErr_NoMemory, but
> >>
> >>> +struct mpatch_flist *cpygetnextitem(void *bins, ssize_t pos)
> >>> +{
> >>> +    const char *buffer;
> >>> +    struct mpatch_flist *res;
> >>> +    ssize_t blen;
> >>> +    int r;
> >>> +
> >>> +    PyObject *tmp = PyList_GetItem((PyObject*)bins, pos);
> >>> +    if (!tmp)
> >>> +            return NULL;
> >>> +    if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t*)&blen))
> >>> +            return NULL;
> >>> +    if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
> >>> +            if (!PyErr_Occurred())
> >>> +                    PyErr_SetString(mpatch_Error, mpatch_errors[-r]);
> >>> +            return NULL;
> >>> +    }
> >>> +    return res;
> >>> +}
> >>
> >> new fold() sets mpatch_Error instead,
> >>
> >>> +static PyObject *
> >>> +patches(PyObject *self, PyObject *args)
> >>> +{
> >>> +    PyObject *text, *bins, *result;
> >>> +    struct mpatch_flist *patch;
> >>> +    const char *in;
> >>> +    char *out;
> >>> +    Py_ssize_t len, outlen, inlen;
> >>> +
> >>> +    if (!PyArg_ParseTuple(args, "OO:mpatch", &text, &bins))
> >>> +            return NULL;
> >>> +
> >>> +    len = PyList_Size(bins);
> >>> +    if (!len) {
> >>> +            /* nothing to do */
> >>> +            Py_INCREF(text);
> >>> +            return text;
> >>> +    }
> >>> +
> >>> +    if (PyObject_AsCharBuffer(text, &in, &inlen))
> >>> +            return NULL;
> >>> +
> >>> +    patch = mpatch_fold(bins, cpygetnextitem, 0, len);
> >>> +    if (!patch) {
> >>> +            return NULL;
> >>
> >> or return NULL without setting any exception.
> >>
> >> I haven't read the code other than lalloc(). I'll need more time to concentrate
> >> on reviewing this. To reviewers, feel free to beat me if you have time.
> >>
> >> And again, I'm not a fan of mixing code moves, naming changes, design changes,
> >> etc. into one patch just because they are things necessary for the cffi mpatch.
> >
> > Thanks to Yuya for trying to review this.
> >
> > To be honest, I'm fairly put off by the patch author insisting on mixing
> > code and design changes. This is at least the third time I've seen an
> > error when trying to do both. Which is a problem because it exhausts too
> > much of a reviewer's time.
> >
> > So, I'm asking: please, please, with a cherry on top, split up your
> > patches for review.
Yuya Nishihara - July 7, 2016, 1:29 p.m.
On Thu, 7 Jul 2016 13:34:51 +0100, Jun Wu wrote:
> Usually, the first patch will be moving code without changing logic
> (including renaming a variable). The second patch will be changing lines
> without moving code. The point is to make "diff" output more friendly to
> review. See 616ea95c8f11 as an example. It's not the exactly the same with
> your case but you can get the idea.

You can't move the code first because this patch splits CPython interface
from the core logic. Instead, this could be:

 1. factor out core functions (which have no Py* function calls)
 2. do trivial renames
 3. extract mpatch.h
 4. copy mpatch.c to mpatch_module.c and remove the core functions from
    mpatch_module.c

That will greatly help reviewing this patch because (1) will have no noise.
Anyway, I'm going to do that by myself to understand what's changed.

Patch

diff -r 3c349a3d45b9 -r 5c7e2b441d64 mercurial/compat.h
--- a/mercurial/compat.h	Mon Jun 06 13:08:13 2016 +0200
+++ b/mercurial/compat.h	Tue Jun 07 14:44:04 2016 +0200
@@ -35,4 +35,8 @@ 
 #define inline __inline
 #endif
 
+#ifndef ssize_t
+#define ssize_t size_t
 #endif
+
+#endif
diff -r 3c349a3d45b9 -r 5c7e2b441d64 mercurial/mpatch.c
--- a/mercurial/mpatch.c	Mon Jun 06 13:08:13 2016 +0200
+++ b/mercurial/mpatch.c	Tue Jun 07 14:44:04 2016 +0200
@@ -20,36 +20,26 @@ 
  of the GNU General Public License, incorporated herein by reference.
 */
 
-#define PY_SSIZE_T_CLEAN
-#include <Python.h>
 #include <stdlib.h>
 #include <string.h>
 
-#include "util.h"
 #include "bitmanipulation.h"
+#include "mpatch.h"
 
-static char mpatch_doc[] = "Efficient binary patching.";
-static PyObject *mpatch_Error;
+char *mpatch_errors[] = {NULL, "no memory", "patch cannot be decoded",
+							"invalid patch"};
 
-struct frag {
-	int start, end, len;
-	const char *data;
-};
-
-struct flist {
-	struct frag *base, *head, *tail;
-};
-
-static struct flist *lalloc(Py_ssize_t size)
+static struct mpatch_flist *lalloc(ssize_t size)
 {
-	struct flist *a = NULL;
+	struct mpatch_flist *a = NULL;
 
 	if (size < 1)
 		size = 1;
 
-	a = (struct flist *)malloc(sizeof(struct flist));
+	a = (struct mpatch_flist *)malloc(sizeof(struct mpatch_flist));
 	if (a) {
-		a->base = (struct frag *)malloc(sizeof(struct frag) * size);
+		a->base = (struct mpatch_frag *)malloc(sizeof(struct mpatch_frag) *
+			size);
 		if (a->base) {
 			a->head = a->tail = a->base;
 			return a;
@@ -57,12 +47,10 @@ 
 		free(a);
 		a = NULL;
 	}
-	if (!PyErr_Occurred())
-		PyErr_NoMemory();
 	return NULL;
 }
 
-static void lfree(struct flist *a)
+void mpatch_lfree(struct mpatch_flist *a)
 {
 	if (a) {
 		free(a->base);
@@ -70,7 +58,7 @@ 
 	}
 }
 
-static Py_ssize_t lsize(struct flist *a)
+static ssize_t lsize(struct mpatch_flist *a)
 {
 	return a->tail - a->head;
 }
@@ -78,9 +66,10 @@ 
 /* move hunks in source that are less cut to dest, compensating
    for changes in offset. the last hunk may be split if necessary.
 */
-static int gather(struct flist *dest, struct flist *src, int cut, int offset)
+static int gather(struct mpatch_flist *dest, struct mpatch_flist *src, int cut,
+	int offset)
 {
-	struct frag *d = dest->tail, *s = src->head;
+	struct mpatch_frag *d = dest->tail, *s = src->head;
 	int postend, c, l;
 
 	while (s != src->tail) {
@@ -123,9 +112,9 @@ 
 }
 
 /* like gather, but with no output list */
-static int discard(struct flist *src, int cut, int offset)
+static int discard(struct mpatch_flist *src, int cut, int offset)
 {
-	struct frag *s = src->head;
+	struct mpatch_frag *s = src->head;
 	int postend, c, l;
 
 	while (s != src->tail) {
@@ -160,10 +149,10 @@ 
 
 /* combine hunk lists a and b, while adjusting b for offset changes in a/
    this deletes a and b and returns the resultant list. */
-static struct flist *combine(struct flist *a, struct flist *b)
+struct mpatch_flist *combine(struct mpatch_flist *a, struct mpatch_flist *b)
 {
-	struct flist *c = NULL;
-	struct frag *bh, *ct;
+	struct mpatch_flist *c = NULL;
+	struct mpatch_frag *bh, *ct;
 	int offset = 0, post;
 
 	if (a && b)
@@ -189,26 +178,26 @@ 
 		}
 
 		/* hold on to tail from a */
-		memcpy(c->tail, a->head, sizeof(struct frag) * lsize(a));
+		memcpy(c->tail, a->head, sizeof(struct mpatch_frag) * lsize(a));
 		c->tail += lsize(a);
 	}
 
-	lfree(a);
-	lfree(b);
+	mpatch_lfree(a);
+	mpatch_lfree(b);
 	return c;
 }
 
 /* decode a binary patch into a hunk list */
-static struct flist *decode(const char *bin, Py_ssize_t len)
+int mpatch_decode(const char *bin, ssize_t len, struct mpatch_flist** res)
 {
-	struct flist *l;
-	struct frag *lt;
+	struct mpatch_flist *l;
+	struct mpatch_frag *lt;
 	int pos = 0;
 
 	/* assume worst case size, we won't have many of these lists */
 	l = lalloc(len / 12 + 1);
 	if (!l)
-		return NULL;
+		return MPATCH_ERR_NO_MEM;
 
 	lt = l->tail;
 
@@ -224,28 +213,24 @@ 
 	}
 
 	if (pos != len) {
-		if (!PyErr_Occurred())
-			PyErr_SetString(mpatch_Error, "patch cannot be decoded");
-		lfree(l);
-		return NULL;
+		mpatch_lfree(l);
+		return MPATCH_ERR_CANNOT_BE_DECODED;
 	}
 
 	l->tail = lt;
-	return l;
+	*res = l;
+	return 0;
 }
 
 /* calculate the size of resultant text */
-static Py_ssize_t calcsize(Py_ssize_t len, struct flist *l)
+ssize_t mpatch_calcsize(ssize_t len, struct mpatch_flist *l)
 {
-	Py_ssize_t outlen = 0, last = 0;
-	struct frag *f = l->head;
+	ssize_t outlen = 0, last = 0;
+	struct mpatch_frag *f = l->head;
 
 	while (f != l->tail) {
 		if (f->start < last || f->end > len) {
-			if (!PyErr_Occurred())
-				PyErr_SetString(mpatch_Error,
-				                "invalid patch");
-			return -1;
+			return MPATCH_ERR_INVALID_PATCH;
 		}
 		outlen += f->start - last;
 		last = f->end;
@@ -257,18 +242,16 @@ 
 	return outlen;
 }
 
-static int apply(char *buf, const char *orig, Py_ssize_t len, struct flist *l)
+int mpatch_apply(char *buf, const char *orig, ssize_t len,
+	struct mpatch_flist *l)
 {
-	struct frag *f = l->head;
+	struct mpatch_frag *f = l->head;
 	int last = 0;
 	char *p = buf;
 
 	while (f != l->tail) {
 		if (f->start < last || f->end > len) {
-			if (!PyErr_Occurred())
-				PyErr_SetString(mpatch_Error,
-				                "invalid patch");
-			return 0;
+			return MPATCH_ERR_INVALID_PATCH;
 		}
 		memcpy(p, orig + last, f->start - last);
 		p += f->start - last;
@@ -278,146 +261,23 @@ 
 		f++;
 	}
 	memcpy(p, orig + last, len - last);
-	return 1;
+	return 0;
 }
 
 /* recursively generate a patch of all bins between start and end */
-static struct flist *fold(PyObject *bins, Py_ssize_t start, Py_ssize_t end)
+struct mpatch_flist *mpatch_fold(void *bins,
+	struct mpatch_flist* (*get_next_item)(void*, ssize_t),
+	ssize_t start, ssize_t end)
 {
-	Py_ssize_t len, blen;
-	const char *buffer;
+	ssize_t len;
 
 	if (start + 1 == end) {
 		/* trivial case, output a decoded list */
-		PyObject *tmp = PyList_GetItem(bins, start);
-		if (!tmp)
-			return NULL;
-		if (PyObject_AsCharBuffer(tmp, &buffer, &blen))
-			return NULL;
-		return decode(buffer, blen);
+		return get_next_item(bins, start);
 	}
 
 	/* divide and conquer, memory management is elsewhere */
 	len = (end - start) / 2;
-	return combine(fold(bins, start, start + len),
-		       fold(bins, start + len, end));
+	return combine(mpatch_fold(bins, get_next_item, start, start + len),
+		       mpatch_fold(bins, get_next_item, start + len, end));
 }
-
-static PyObject *
-patches(PyObject *self, PyObject *args)
-{
-	PyObject *text, *bins, *result;
-	struct flist *patch;
-	const char *in;
-	char *out;
-	Py_ssize_t len, outlen, inlen;
-
-	if (!PyArg_ParseTuple(args, "OO:mpatch", &text, &bins))
-		return NULL;
-
-	len = PyList_Size(bins);
-	if (!len) {
-		/* nothing to do */
-		Py_INCREF(text);
-		return text;
-	}
-
-	if (PyObject_AsCharBuffer(text, &in, &inlen))
-		return NULL;
-
-	patch = fold(bins, 0, len);
-	if (!patch)
-		return NULL;
-
-	outlen = calcsize(inlen, patch);
-	if (outlen < 0) {
-		result = NULL;
-		goto cleanup;
-	}
-	result = PyBytes_FromStringAndSize(NULL, outlen);
-	if (!result) {
-		result = NULL;
-		goto cleanup;
-	}
-	out = PyBytes_AsString(result);
-	if (!apply(out, in, inlen, patch)) {
-		Py_DECREF(result);
-		result = NULL;
-	}
-cleanup:
-	lfree(patch);
-	return result;
-}
-
-/* calculate size of a patched file directly */
-static PyObject *
-patchedsize(PyObject *self, PyObject *args)
-{
-	long orig, start, end, len, outlen = 0, last = 0, pos = 0;
-	Py_ssize_t patchlen;
-	char *bin;
-
-	if (!PyArg_ParseTuple(args, "ls#", &orig, &bin, &patchlen))
-		return NULL;
-
-	while (pos >= 0 && pos < patchlen) {
-		start = getbe32(bin + pos);
-		end = getbe32(bin + pos + 4);
-		len = getbe32(bin + pos + 8);
-		if (start > end)
-			break; /* sanity check */
-		pos += 12 + len;
-		outlen += start - last;
-		last = end;
-		outlen += len;
-	}
-
-	if (pos != patchlen) {
-		if (!PyErr_Occurred())
-			PyErr_SetString(mpatch_Error, "patch cannot be decoded");
-		return NULL;
-	}
-
-	outlen += orig - last;
-	return Py_BuildValue("l", outlen);
-}
-
-static PyMethodDef methods[] = {
-	{"patches", patches, METH_VARARGS, "apply a series of patches\n"},
-	{"patchedsize", patchedsize, METH_VARARGS, "calculed patched size\n"},
-	{NULL, NULL}
-};
-
-#ifdef IS_PY3K
-static struct PyModuleDef mpatch_module = {
-	PyModuleDef_HEAD_INIT,
-	"mpatch",
-	mpatch_doc,
-	-1,
-	methods
-};
-
-PyMODINIT_FUNC PyInit_mpatch(void)
-{
-	PyObject *m;
-
-	m = PyModule_Create(&mpatch_module);
-	if (m == NULL)
-		return NULL;
-
-	mpatch_Error = PyErr_NewException("mercurial.mpatch.mpatchError",
-					  NULL, NULL);
-	Py_INCREF(mpatch_Error);
-	PyModule_AddObject(m, "mpatchError", mpatch_Error);
-
-	return m;
-}
-#else
-PyMODINIT_FUNC
-initmpatch(void)
-{
-	Py_InitModule3("mpatch", methods, mpatch_doc);
-	mpatch_Error = PyErr_NewException("mercurial.mpatch.mpatchError",
-					  NULL, NULL);
-}
-#endif
diff -r 3c349a3d45b9 -r 5c7e2b441d64 mercurial/mpatch.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/mercurial/mpatch.h	Tue Jun 07 14:44:04 2016 +0200
@@ -0,0 +1,28 @@ 
+#ifndef _HG_MPATCH_H_
+#define _HG_MPATCH_H_
+
+extern char *mpatch_errors[];
+
+#define MPATCH_ERR_NO_MEM -1
+#define MPATCH_ERR_CANNOT_BE_DECODED -2
+#define MPATCH_ERR_INVALID_PATCH -3
+
+struct mpatch_frag {
+	int start, end, len;
+	const char *data;
+};
+
+struct mpatch_flist {
+	struct mpatch_frag *base, *head, *tail;
+};
+
+int mpatch_decode(const char *bin, ssize_t len, struct mpatch_flist** res);
+ssize_t mpatch_calcsize(ssize_t len, struct mpatch_flist *l);
+void mpatch_lfree(struct mpatch_flist *a);
+int mpatch_apply(char *buf, const char *orig, ssize_t len,
+	struct mpatch_flist *l);
+struct mpatch_flist *mpatch_fold(void *bins,
+	struct mpatch_flist* (*get_next_item)(void*, ssize_t),
+	ssize_t start, ssize_t end);
+
+#endif
diff -r 3c349a3d45b9 -r 5c7e2b441d64 mercurial/mpatch_module.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/mercurial/mpatch_module.c	Tue Jun 07 14:44:04 2016 +0200
@@ -0,0 +1,153 @@ 
+
+#define PY_SSIZE_T_CLEAN
+#include <Python.h>
+
+#include "util.h"
+#include "mpatch.h"
+#include "bitmanipulation.h"
+
+static char mpatch_doc[] = "Efficient binary patching.";
+
+static PyObject *mpatch_Error;
+
+struct mpatch_flist *cpygetnextitem(void *bins, ssize_t pos)
+{
+	const char *buffer;
+	struct mpatch_flist *res;
+	ssize_t blen;
+	int r;
+
+	PyObject *tmp = PyList_GetItem((PyObject*)bins, pos);
+	if (!tmp)
+		return NULL;
+	if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t*)&blen))
+		return NULL;
+	if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
+		if (!PyErr_Occurred())
+			PyErr_SetString(mpatch_Error, mpatch_errors[-r]);
+		return NULL;
+	}
+	return res;
+}
+
+static PyObject *
+patches(PyObject *self, PyObject *args)
+{
+	PyObject *text, *bins, *result;
+	struct mpatch_flist *patch;
+	const char *in;
+	char *out;
+	Py_ssize_t len, outlen, inlen;
+
+	if (!PyArg_ParseTuple(args, "OO:mpatch", &text, &bins))
+		return NULL;
+
+	len = PyList_Size(bins);
+	if (!len) {
+		/* nothing to do */
+		Py_INCREF(text);
+		return text;
+	}
+
+	if (PyObject_AsCharBuffer(text, &in, &inlen))
+		return NULL;
+
+	patch = mpatch_fold(bins, cpygetnextitem, 0, len);
+	if (!patch) {
+		return NULL;
+	}
+
+	outlen = mpatch_calcsize(inlen, patch);
+	if (outlen < 0) {
+		if (!PyErr_Occurred())
+			PyErr_SetString(mpatch_Error, mpatch_errors[-outlen]);
+		result = NULL;
+		goto cleanup;
+	}
+	result = PyBytes_FromStringAndSize(NULL, outlen);
+	if (!result) {
+		result = NULL;
+		goto cleanup;
+	}
+	out = PyBytes_AsString(result);
+	if (mpatch_apply(out, in, inlen, patch) < 0) {
+		Py_DECREF(result);
+		result = NULL;
+	}
+cleanup:
+	mpatch_lfree(patch);
+	return result;
+}
+
+/* calculate size of a patched file directly */
+static PyObject *
+patchedsize(PyObject *self, PyObject *args)
+{
+	long orig, start, end, len, outlen = 0, last = 0, pos = 0;
+	Py_ssize_t patchlen;
+	char *bin;
+
+	if (!PyArg_ParseTuple(args, "ls#", &orig, &bin, &patchlen))
+		return NULL;
+
+	while (pos >= 0 && pos < patchlen) {
+		start = getbe32(bin + pos);
+		end = getbe32(bin + pos + 4);
+		len = getbe32(bin + pos + 8);
+		if (start > end)
+			break; /* sanity check */
+		pos += 12 + len;
+		outlen += start - last;
+		last = end;
+		outlen += len;
+	}
+
+	if (pos != patchlen) {
+		if (!PyErr_Occurred())
+			PyErr_SetString(mpatch_Error, "patch cannot be decoded");
+		return NULL;
+	}
+
+	outlen += orig - last;
+	return Py_BuildValue("l", outlen);
+}
+
+static PyMethodDef methods[] = {
+	{"patches", patches, METH_VARARGS, "apply a series of patches\n"},
+	{"patchedsize", patchedsize, METH_VARARGS, "calculed patched size\n"},
+	{NULL, NULL}
+};
+
+#ifdef IS_PY3K
+static struct PyModuleDef mpatch_module = {
+	PyModuleDef_HEAD_INIT,
+	"mpatch",
+	mpatch_doc,
+	-1,
+	methods
+};
+
+PyMODINIT_FUNC PyInit_mpatch(void)
+{
+	PyObject *m;
+
+	m = PyModule_Create(&mpatch_module);
+	if (m == NULL)
+		return NULL;
+
+	mpatch_Error = PyErr_NewException("mercurial.mpatch.mpatchError",
+					  NULL, NULL);
+	Py_INCREF(mpatch_Error);
+	PyModule_AddObject(m, "mpatchError", mpatch_Error);
+
+	return m;
+}
+#else
+PyMODINIT_FUNC
+initmpatch(void)
+{
+	Py_InitModule3("mpatch", methods, mpatch_doc);
+	mpatch_Error = PyErr_NewException("mercurial.mpatch.mpatchError",
+					  NULL, NULL);
+}
+#endif
diff -r 3c349a3d45b9 -r 5c7e2b441d64 setup.py
--- a/setup.py	Mon Jun 06 13:08:13 2016 +0200
+++ b/setup.py	Tue Jun 07 14:44:04 2016 +0200
@@ -550,7 +550,8 @@ 
               depends=common_depends),
     Extension('mercurial.diffhelpers', ['mercurial/diffhelpers.c'],
               depends=common_depends),
-    Extension('mercurial.mpatch', ['mercurial/mpatch.c'],
+    Extension('mercurial.mpatch', ['mercurial/mpatch.c',
+                                   'mercurial/mpatch_module.c'],
               depends=common_depends),
     Extension('mercurial.parsers', ['mercurial/dirs.c',
                                     'mercurial/manifest.c',