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

[Sean Farley]

Yuya Nishihara <yuya at tcha.org> writes:

> On Mon, 04 Jul 2016 11:16:23 +0200, Maciej Fijalkowski wrote:
>> # HG changeset patch
>> # User Maciej Fijalkowski <fijall at 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.