| Submitter | Martin von Zweigbergk |
|---|---|
| Date | April 2, 2015, 3:54 p.m. |
| Message ID | <ac9ec0e0d98c8939eed8.1427990089@martinvonz.mtv.corp.google.com> |
| Download | mbox | patch |
| Permalink | /patch/8456/ |
| State | Accepted |
| Headers | show |
Comments
On Thu, 2015-04-02 at 08:54 -0700, Martin von Zweigbergk wrote: > # HG changeset patch > # User Martin von Zweigbergk <martinvonz@google.com> > # Date 1427989460 25200 > # Thu Apr 02 08:44:20 2015 -0700 > # Node ID ac9ec0e0d98c8939eed835aff198eef35c368fdc > # Parent 37a2b446985f2ef77b9690a0548c8630828b7412 > parsers: remove unused parse_manifest() function Queued for default, thanks.
On Thu, Apr 2, 2015 at 12:28 PM, Matt Mackall <mpm@selenic.com> wrote: > > parsers: remove unused parse_manifest() function > > Queued for default, thanks. > Won't this break bisecting into the past? I thought we tried to keep things around for a while in case we had to bisect something... (Just trying to better understand the rules around the way we change C extensions.)
On Thu, 2015-04-02 at 12:44 -0400, Augie Fackler wrote: > On Thu, Apr 2, 2015 at 12:28 PM, Matt Mackall <mpm@selenic.com> wrote: > > > > parsers: remove unused parse_manifest() function > > > > Queued for default, thanks. > > > > Won't this break bisecting into the past? I thought we tried to keep things > around for a while in case we had to bisect something... Now that you mention it, probably not. The parse_manifest code is quite old and the fallback has lived in pure/ for quite a while. So even though recent code works without this function, we don't have to go back far to hit a point where it's required. I think if we were to have a bisect that went back a year (not unusual), things would break. Going to drop this for now. > (Just trying to better understand the rules around the way we change C > extensions.) If we think of our C code like we think of Python versions, you get a pretty good model: ideally any version of Hg's .py code should be able to run against any version of the .c code by detecting available features. We didn't do very well with this until the past few years. In fact, early on (pre-1.0), I forcibly broke this because lots of people were reporting benchmarks against Git without ANY extensions compiled. We later brought back pure/, but there's an argument that we should make pure Python more accessible.
On Thu, Apr 2, 2015 at 9:44 AM Augie Fackler <augie@google.com> wrote: > > On Thu, Apr 2, 2015 at 12:28 PM, Matt Mackall <mpm@selenic.com> wrote: > >> > parsers: remove unused parse_manifest() function >> >> Queued for default, thanks. >> > > Won't this break bisecting into the past? I thought we tried to keep > things around for a while in case we had to bisect something... > Good point. In that case, what I messed up more is probably http://selenic.com/hg/rev/49cd847fd69a. But now that it's done, I suspect it's better to leave it alone. It would only break if you bisect to a commit within a range of a month or so. >
Patch
diff -r 37a2b446985f -r ac9ec0e0d98c mercurial/parsers.c --- a/mercurial/parsers.c Wed Apr 01 20:38:36 2015 -0500 +++ b/mercurial/parsers.c Thu Apr 02 08:44:20 2015 -0700 @@ -167,87 +167,6 @@ return _asciitransform(str_obj, uppertable); } -/* - * This code assumes that a manifest is stitched together with newline - * ('\n') characters. - */ -static PyObject *parse_manifest(PyObject *self, PyObject *args) -{ - PyObject *mfdict, *fdict; - char *str, *start, *end; - int len; - - if (!PyArg_ParseTuple(args, "O!O!s#:parse_manifest", - &PyDict_Type, &mfdict, - &PyDict_Type, &fdict, - &str, &len)) - goto quit; - - start = str; - end = str + len; - while (start < end) { - PyObject *file = NULL, *node = NULL; - PyObject *flags = NULL; - char *zero = NULL, *newline = NULL; - ptrdiff_t nlen; - - zero = memchr(start, '\0', end - start); - if (!zero) { - PyErr_SetString(PyExc_ValueError, - "manifest entry has no separator"); - goto quit; - } - - newline = memchr(zero + 1, '\n', end - (zero + 1)); - if (!newline) { - PyErr_SetString(PyExc_ValueError, - "manifest contains trailing garbage"); - goto quit; - } - - file = PyBytes_FromStringAndSize(start, zero - start); - - if (!file) - goto bail; - - nlen = newline - zero - 1; - - node = unhexlify(zero + 1, nlen > 40 ? 40 : (int)nlen); - if (!node) - goto bail; - - if (nlen > 40) { - flags = PyBytes_FromStringAndSize(zero + 41, - nlen - 40); - if (!flags) - goto bail; - - if (PyDict_SetItem(fdict, file, flags) == -1) - goto bail; - } - - if (PyDict_SetItem(mfdict, file, node) == -1) - goto bail; - - start = newline + 1; - - Py_XDECREF(flags); - Py_XDECREF(node); - Py_XDECREF(file); - continue; - bail: - Py_XDECREF(flags); - Py_XDECREF(node); - Py_XDECREF(file); - goto quit; - } - - Py_INCREF(Py_None); - return Py_None; -quit: - return NULL; -} - static inline dirstateTupleObject *make_dirstate_tuple(char state, int mode, int size, int mtime) { @@ -2452,7 +2371,6 @@ static PyMethodDef methods[] = { {"pack_dirstate", pack_dirstate, METH_VARARGS, "pack a dirstate\n"}, - {"parse_manifest", parse_manifest, METH_VARARGS, "parse a manifest\n"}, {"parse_dirstate", parse_dirstate, METH_VARARGS, "parse a dirstate\n"}, {"parse_index2", parse_index2, METH_VARARGS, "parse a revlog index\n"}, {"asciilower", asciilower, METH_VARARGS, "lowercase an ASCII string\n"},