Patchwork parsers: remove unused parse_manifest() function

login
register
mail settings
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

Martin von Zweigbergk - April 2, 2015, 3:54 p.m.
# 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

Since 3e5c4af69808 (manifest: split manifestdict into high-level and
low-level logic, 2015-03-07), we no longer use the native
parse_manifest() function, so remove it.
Matt Mackall - April 2, 2015, 4:28 p.m.
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.
Augie Fackler - April 2, 2015, 4:44 p.m.
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.)
Matt Mackall - April 2, 2015, 5:46 p.m.
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.
Martin von Zweigbergk - April 2, 2015, 6:24 p.m.
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"},