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

Maciej Fijalkowski fijall at gmail.com
Thu Jul 7 03:02:43 EDT 2016


So how do you guys want this to be split?

On Wed, Jul 6, 2016 at 8:03 PM, Sean Farley <sean at farley.io> wrote:
> 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.


More information about the Mercurial-devel mailing list