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

[Jun Wu]

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 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.