[PATCH 0/4] Honor core.ignorecase for attribute patterns

On 09/13/2011 11:22 AM, Brandon Casey wrote:

On 09/13/2011 11:05 AM, Jeff King wrote:

quoted

On Tue, Sep 13, 2011 at 10:15:15AM -0500, Brandon Casey wrote:

quoted

...and I see there is already an fnmatch_icase() in dir.c which adds
FNM_CASEFOLD when the global var ignore_case is set.  So, maybe it's as
easy as:
[...]
-               return (fnmatch(pattern, basename, 0) == 0);
+               return (fnmatch_icase(pattern, basename, 0) == 0);

OK, wow. That's exactly the level of easy I was hoping for. Do you want
to roll that up into a patch with some tests?

I haven't even tested that it runs. :)  No, I was hoping someone
who was more interested would finish it, and maybe even test on
an affected system.

Ok, I lied.  Here's a series that needs testing by people on a  
case-insensitive filesystem and some comments.

The first three patches are just housekeeping and can be accepted 
independently of the fourth patch which is marked WIP.

The last patch implements the case-insensitive matching of attribute
patterns, but I discovered that bad things can happen if git_config()
is called more than once.  Details are in the patch email.

-Brandon

[PATCH 1/4] attr.c: avoid inappropriate access to strbuf "buf"
[PATCH 2/4] cleanup: use internal memory allocation wrapper
[PATCH 3/4] builtin/mv.c: plug miniscule memory leak
[PATCH 4/4] attr.c: respect core.ignorecase when matching attribute

This code sequence performs a strcpy into the buf member of a strbuf
struct.  The strcpy may move the position of the terminating nul of the
string and effectively change the length of string so that it does not
match the len member of the strbuf struct.

Currently, this sequence works since the strbuf was given a hint when it
was initialized to allocate enough space to accomodate the string that will
be strcpy'ed, but this is an implementation detail of strbufs, not a
guarantee.

So, lets rework this sequence so that the strbuf is only manipulated by
strbuf functions, and direct modification of its "buf" member is not
necessary.

Signed-off-by: Brandon Casey <redacted>
---
 attr.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/attr.c b/attr.c
index 33cb4e4..206f233 100644
--- a/attr.c
+++ b/attr.c

@@ -552,7 +552,6 @@ static void prepare_attr_stack(const char *path)
 {
 	struct attr_stack *elem, *info;
 	int dirlen, len;
-	struct strbuf pathbuf;
 	const char *cp;
 
 	cp = strrchr(path, '/');

@@ -561,8 +560,6 @@ static void prepare_attr_stack(const char *path)
 	else
 		dirlen = cp - path;
 
-	strbuf_init(&pathbuf, dirlen+2+strlen(GITATTRIBUTES_FILE));
-
 	/*
 	 * At the bottom of the attribute stack is the built-in
 	 * set of attribute definitions, followed by the contents

@@ -607,27 +604,29 @@ static void prepare_attr_stack(const char *path)
 	 * Read from parent directories and push them down
 	 */
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
-		while (1) {
-			char *cp;
+		struct strbuf pathbuf = STRBUF_INIT;
 
+		while (1) {
 			len = strlen(attr_stack->origin);
 			if (dirlen <= len)
 				break;
-			strbuf_reset(&pathbuf);
-			strbuf_add(&pathbuf, path, dirlen);
+			cp = memchr(path + len + 1, '/', dirlen - len - 1);
+			if (!cp)
+				cp = path + dirlen;
+			strbuf_add(&pathbuf, path, cp - path);
 			strbuf_addch(&pathbuf, '/');
-			cp = strchr(pathbuf.buf + len + 1, '/');
-			strcpy(cp + 1, GITATTRIBUTES_FILE);
+			strbuf_add(&pathbuf, GITATTRIBUTES_FILE,
+				strlen(GITATTRIBUTES_FILE));
 			elem = read_attr(pathbuf.buf, 0);
-			*cp = '\0';
-			elem->origin = strdup(pathbuf.buf);
+			strbuf_setlen(&pathbuf, cp - path);
+			elem->origin = strbuf_detach(&pathbuf, NULL);
 			elem->prev = attr_stack;
 			attr_stack = elem;
 			debug_push(elem);
 		}
-	}
 
-	strbuf_release(&pathbuf);
+		strbuf_release(&pathbuf);
+	}
 
 	/*
 	 * Finally push the "info" one at the top of the stack.

-- 
1.7.6.2

The "x"-prefixed versions of strdup, malloc, etc. will check whether the
allocation was successful and terminate the process otherwise.

A few uses of malloc were left alone since they already implemented a
graceful path of failure or were in a quasi external library like xdiff.

Signed-off-by: Brandon Casey <redacted>
---
 attr.c                |    2 +-
 builtin/mv.c          |    2 +-
 compat/mingw.c        |    2 +-
 compat/qsort.c        |    2 +-
 compat/win32/syslog.c |    2 +-
 remote.c              |    2 +-
 show-index.c          |    2 +-
 transport-helper.c    |    4 ++--
 8 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/attr.c b/attr.c
index 206f233..3359b39 100644
--- a/attr.c
+++ b/attr.c

@@ -533,7 +533,7 @@ static void bootstrap_attr_stack(void)
 
 		if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
 			elem = read_attr(GITATTRIBUTES_FILE, 1);
-			elem->origin = strdup("");
+			elem->origin = xstrdup("");
 			elem->prev = attr_stack;
 			attr_stack = elem;
 			debug_push(elem);

diff --git a/builtin/mv.c b/builtin/mv.c
index 40f33ca..e9d191f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c

@@ -29,7 +29,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,
 			to_copy--;
 		if (to_copy != length || base_name) {
 			char *it = xmemdupz(result[i], to_copy);
-			result[i] = base_name ? strdup(basename(it)) : it;
+			result[i] = base_name ? xstrdup(basename(it)) : it;
 		}
 	}
 	return get_pathspec(prefix, result);

diff --git a/compat/mingw.c b/compat/mingw.c
index 6ef0cc4..8947418 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1183,7 +1183,7 @@ static int WSAAPI getaddrinfo_stub(const char *node, const char *service,
 	}
 	ai->ai_addrlen = sizeof(struct sockaddr_in);
 	if (hints && (hints->ai_flags & AI_CANONNAME))
-		ai->ai_canonname = h ? strdup(h->h_name) : NULL;
+		ai->ai_canonname = h ? xstrdup(h->h_name) : NULL;
 	else
 		ai->ai_canonname = NULL;
 

diff --git a/compat/qsort.c b/compat/qsort.c
index d93dce2..9574d53 100644
--- a/compat/qsort.c
+++ b/compat/qsort.c

@@ -55,7 +55,7 @@ void git_qsort(void *b, size_t n, size_t s,
 		msort_with_tmp(b, n, s, cmp, buf);
 	} else {
 		/* It's somewhat large, so malloc it.  */
-		char *tmp = malloc(size);
+		char *tmp = xmalloc(size);
 		msort_with_tmp(b, n, s, cmp, tmp);
 		free(tmp);
 	}

diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 42b95a9..197ab34 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c

@@ -38,7 +38,7 @@ void syslog(int priority, const char *fmt, ...)
 		return;
 	}
 
-	str = malloc(str_len + 1);
+	str = xmalloc(str_len + 1);
 	va_start(ap, fmt);
 	vsnprintf(str, str_len + 1, fmt, ap);
 	va_end(ap);

diff --git a/remote.c b/remote.c
index b8ecfa5..7840d2f 100644
--- a/remote.c
+++ b/remote.c

@@ -840,7 +840,7 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
 						    refspec->dst, &ret))
 				return ret;
 		} else if (!strcmp(refspec->src, name))
-			return strdup(refspec->dst);
+			return xstrdup(refspec->dst);
 	}
 	return NULL;
 }

diff --git a/show-index.c b/show-index.c
index 4c0ac13..63f9da5 100644
--- a/show-index.c
+++ b/show-index.c

@@ -48,7 +48,7 @@ int main(int argc, char **argv)
 			unsigned char sha1[20];
 			uint32_t crc;
 			uint32_t off;
-		} *entries = malloc(nr * sizeof(entries[0]));
+		} *entries = xmalloc(nr * sizeof(entries[0]));
 		for (i = 0; i < nr; i++)
 			if (fread(entries[i].sha1, 20, 1, stdin) != 1)
 				die("unable to read sha1 %u/%u", i, nr);

diff --git a/transport-helper.c b/transport-helper.c
index 4eab844..0713126 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -183,7 +183,7 @@ static struct child_process *get_helper(struct transport *transport)
 			ALLOC_GROW(refspecs,
 				   refspec_nr + 1,
 				   refspec_alloc);
-			refspecs[refspec_nr++] = strdup(capname + strlen("refspec "));
+			refspecs[refspec_nr++] = xstrdup(capname + strlen("refspec "));
 		} else if (!strcmp(capname, "connect")) {
 			data->connect = 1;
 		} else if (!prefixcmp(capname, "export-marks ")) {

@@ -445,7 +445,7 @@ static int fetch_with_import(struct transport *transport,
 		if (data->refspecs)
 			private = apply_refspecs(data->refspecs, data->refspec_nr, posn->name);
 		else
-			private = strdup(posn->name);
+			private = xstrdup(posn->name);
 		read_ref(private, posn->old_sha1);
 		free(private);
 	}

-- 
1.7.6.2

The "it" string would not be free'ed if base_name was non-NULL.
Let's free it.

Signed-off-by: Brandon Casey <redacted>
---
 builtin/mv.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index e9d191f..5efe6c5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c

@@ -29,7 +29,11 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,
 			to_copy--;
 		if (to_copy != length || base_name) {
 			char *it = xmemdupz(result[i], to_copy);
-			result[i] = base_name ? xstrdup(basename(it)) : it;
+			if (base_name) {
+				result[i] = xstrdup(basename(it));
+				free(it);
+			} else
+				result[i] = it;
 		}
 	}
 	return get_pathspec(prefix, result);

-- 
1.7.6.2

When core.ignorecase is true, the file globs configured in the
.gitattributes file should be matched case-insensitively against the paths
in the working directory.

---

Two points:

1)
I think these two changes of fnmatch to fnmatch_icase should be all
that is necessary.  There are a number of uses of strncmp where the
"origin" path of an attribute entry is compared to the prefix of a file
path, but since the attribute stack is built from the same path string
that it is being compared against, we shouldn't have to do
strncmp_icase everywhere.  The case of the two strings should
necessarily match.

This needs some testing by someone on a case-insensitive filesystem.

Also, notice some of the new tests are marked with a CASE_INSENSITIVE_FS
pre-requisite.  I tested on a USB thumb drive, but it would be nice if
someone tested on a platform that is natively case-insensitive.  Maybe
CASE_INSENSITIVE_FS should be moved to test-lib.sh, and t0005(others?)
should be updated to use it?


2)
The bad news, this breaks t8005.  The breakage is caused by
git_attr_config calling git_default_config, and stomping on the
git_log_output_encoding set by setup_revisions() when it parsed the
 --encoding command line option.

What happens is cmd_blame() calls git_config() which parses the config
files and sets up the global config variables like
git_log_output_encoding, then later blame calls setup_revisions() which
parses the command line option --encoding and overrides the value in
git_log_output_encoding, then, even later, userdiff looks up an
attribute on a path and calls git_check_attr() which calls git_config
_again_, which resets git_log_output_encoding to the value in the config
file (stomping on the value set by --encoding on the git blame command
line).

Since fnmatch_icase depends on the ignore_case global variable being set
correctly, the obvious thing for me to do was to allow
git_default_config to call git_default_core_config and parse the
core.ignorecase config option.  So I modified git_attr_config so it fell
back to git_default_config.  But that can have the undesired side effect
described above.

It's easy to work around this issue.  I could just parse core.ignorecase
in git_attr_config() and set ignore_case myself like:

   if (!strcmp(var, "core.ignorecase")) {
           ignore_case = git_config_bool(var, value);
           return 0;
   }

The big question is whether it should be safe to call git_config()
multiple times?  Right now, it is not.  We also don't protect against
git_config() being called multiple times either.

I suspect that setup_revisions() is not the only place where a command
line option overrides a global config variable and it would be a big
can of worms to try to fix them all.

Thoughts?

---
 attr.c                |    7 +++--
 t/t0003-attributes.sh |   60 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/attr.c b/attr.c
index 3359b39..b8ed7cf 100644
--- a/attr.c
+++ b/attr.c

@@ -11,6 +11,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "attr.h"
+#include "dir.h"
 
 const char git_attr__true[] = "(builtin)true";
 const char git_attr__false[] = "\0(builtin)false";

@@ -499,7 +500,7 @@ static int git_attr_config(const char *var, const char *value, void *dummy)
 	if (!strcmp(var, "core.attributesfile"))
 		return git_config_pathname(&attributes_file, var, value);
 
-	return 0;
+	return git_default_config(var, value, dummy);
 }
 
 static void bootstrap_attr_stack(void)

@@ -643,7 +644,7 @@ static int path_matches(const char *pathname, int pathlen,
 		/* match basename */
 		const char *basename = strrchr(pathname, '/');
 		basename = basename ? basename + 1 : pathname;
-		return (fnmatch(pattern, basename, 0) == 0);
+		return (fnmatch_icase(pattern, basename, 0) == 0);
 	}
 	/*
 	 * match with FNM_PATHNAME; the pattern has base implicitly

@@ -657,7 +658,7 @@ static int path_matches(const char *pathname, int pathlen,
 		return 0;
 	if (baselen != 0)
 		baselen++;
-	return fnmatch(pattern, pathname + baselen, FNM_PATHNAME) == 0;
+	return fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;
 }
 
 static int macroexpand_one(int attr_nr, int rem);

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index ae2f1da..47a70c4 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh

@@ -9,7 +9,7 @@ attr_check () {
 	path="$1"
 	expect="$2"
 
-	git check-attr test -- "$path" >actual 2>err &&
+	git $3 check-attr test -- "$path" >actual 2>err &&
 	echo "$path: test: $2" >expect &&
 	test_cmp expect actual &&
 	test_line_count = 0 err

@@ -27,6 +27,7 @@ test_expect_success 'setup' '
 		echo "onoff test -test"
 		echo "offon -test test"
 		echo "no notest"
+		echo "A/e/F test=A/e/F"
 	) >.gitattributes &&
 	(
 		echo "g test=a/g" &&

@@ -93,6 +94,63 @@ test_expect_success 'attribute test' '
 
 '
 
+test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '
+
+	test_must_fail attr_check F f "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/F f "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/c/F f "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/G a/g "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/B/g a/b/g "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/b/G a/b/g "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/b/H a/b/h "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/b/D/g "a/b/d/*" "-c core.ignorecase=0" &&
+	test_must_fail attr_check oNoFf unset "-c core.ignorecase=0" &&
+	test_must_fail attr_check oFfOn set "-c core.ignorecase=0" &&
+	attr_check NO unspecified "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/b/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+	attr_check a/b/d/YES a/b/d/* "-c core.ignorecase=0" &&
+	test_must_fail attr_check a/E/f "A/e/F" "-c core.ignorecase=0"
+
+'
+
+test_expect_success 'attribute matching is case insensitive when core.ignorecase=1' '
+
+	attr_check F f "-c core.ignorecase=1" &&
+	attr_check a/F f "-c core.ignorecase=1" &&
+	attr_check a/c/F f "-c core.ignorecase=1" &&
+	attr_check a/G a/g "-c core.ignorecase=1" &&
+	attr_check a/B/g a/b/g "-c core.ignorecase=1" &&
+	attr_check a/b/G a/b/g "-c core.ignorecase=1" &&
+	attr_check a/b/H a/b/h "-c core.ignorecase=1" &&
+	attr_check a/b/D/g "a/b/d/*" "-c core.ignorecase=1" &&
+	attr_check oNoFf unset "-c core.ignorecase=1" &&
+	attr_check oFfOn set "-c core.ignorecase=1" &&
+	attr_check NO unspecified "-c core.ignorecase=1" &&
+	attr_check a/b/D/NO "a/b/d/*" "-c core.ignorecase=1" &&
+	attr_check a/b/d/YES unspecified "-c core.ignorecase=1" &&
+	attr_check a/E/f "A/e/F" "-c core.ignorecase=1"
+
+'
+
+test_expect_success 'check whether FS is case-insensitive' '
+	mkdir junk &&
+	echo good >junk/CamelCase &&
+	echo bad >junk/camelcase &&
+	if test "$(cat junk/CamelCase)" != good
+	then
+		test_set_prereq CASE_INSENSITIVE_FS
+	fi
+'
+
+test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '
+	test_must_fail attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=0" &&
+	test_must_fail attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+	attr_check A/b/h a/b/h "-c core.ignorecase=0" &&
+	attr_check A/b/h a/b/h "-c core.ignorecase=1" &&
+	attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=1" &&
+	attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=1"
+'
+
 test_expect_success 'unnormalized paths' '
 
 	attr_check ./f f &&

-- 
1.7.6

Am 9/15/2011 3:59, schrieb Brandon Casey:

The "x"-prefixed versions of strdup, malloc, etc. will check whether the
allocation was successful and terminate the process otherwise.

A few uses of malloc were left alone since they already implemented a
graceful path of failure or were in a quasi external library like xdiff.

Signed-off-by: Brandon Casey <redacted>
---
 ...
 compat/mingw.c        |    2 +-
 compat/qsort.c        |    2 +-
 compat/win32/syslog.c |    2 +-

There is a danger that the high-level die() routine (which is used by the
x-wrappers) uses one of the low-level compat/ routines. IOW, in the case
of errors, recursion might occur. Therefore, I would prefer that the
compat/ routines do their own error reporting (preferably via return
values and errno).

-- Hannes

On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt [off-list ref] wrote:

Am 9/15/2011 3:59, schrieb Brandon Casey:

quoted

The "x"-prefixed versions of strdup, malloc, etc. will check whether the
allocation was successful and terminate the process otherwise.

A few uses of malloc were left alone since they already implemented a
graceful path of failure or were in a quasi external library like xdiff.

Signed-off-by: Brandon Casey <redacted>
---
 ...
 compat/mingw.c        |    2 +-
 compat/qsort.c        |    2 +-
 compat/win32/syslog.c |    2 +-

There is a danger that the high-level die() routine (which is used by the
x-wrappers) uses one of the low-level compat/ routines. IOW, in the case
of errors, recursion might occur. Therefore, I would prefer that the
compat/ routines do their own error reporting (preferably via return
values and errno).

Thanks.  Will do.

-Brandon

On Wed, Sep 14, 2011 at 08:59:35PM -0500, Brandon Casey wrote:

quoted

I haven't even tested that it runs. :)  No, I was hoping someone
who was more interested would finish it, and maybe even test on
an affected system.

Ok, I lied.  Here's a series that needs testing by people on a
case-insensitive filesystem and some comments.

Thanks. I was trying to decide if I was interested enough to work on it,
but procrastination wins again.

I'm not sure I understand why you need a case-insensitive file system
for the final set of tests. If we have a case-sensitive system, we can
force the filesystem to show us whatever cases we want, and check
against them with both core.ignorecase off and on[1]. What are these
tests checking that requires the actual behavior of a case-insensitive
filesystem?

I'm sure there is something subtle that I'm missing. Can you explain it
either here or in the commit message?

-Peff

[1] Actually, I wondered at first if the other tests needed to be marked
for only case-sensitive systems, since we can't rely on the behavior of
insensitive ones (e.g., are they case-preserving, always downcasing,
etc). But looking at t0003, we don't seem to actually create the files
in the filesystem at all.

On Thu, Sep 15, 2011 at 1:12 PM, Jeff King [off-list ref] wrote:

On Wed, Sep 14, 2011 at 08:59:35PM -0500, Brandon Casey wrote:

quoted

quoted

I haven't even tested that it runs. :)  No, I was hoping someone
who was more interested would finish it, and maybe even test on
an affected system.

Ok, I lied.  Here's a series that needs testing by people on a
case-insensitive filesystem and some comments.

Thanks. I was trying to decide if I was interested enough to work on it,
but procrastination wins again.

I'm not sure I understand why you need a case-insensitive file system
for the final set of tests. If we have a case-sensitive system, we can
force the filesystem to show us whatever cases we want, and check
against them with both core.ignorecase off and on[1]. What are these
tests checking that requires the actual behavior of a case-insensitive
filesystem?

This is probably way more detail than this feature deserves, but...

Those tests are making sure that git handles the case where the
.gitignore file resides in a subdirectory and the user supplies a path
that does not match the case in the filesystem.  In that
case^H^H^H^Hsituation, part of the path supplied by the user is
effectively interpreted case-insensitively, and part of it is
dependent on the setting of core.ignorecase.  git should only be
matching the portion of the path below the directory holding the
.gitignore file according to the setting of core.ignorecase.

Imagine a hierarchy that looks like this:

   .gitattributes
   a/.gitattributes

On a case-insensitive filesystem, if you supply the path A/B,
regardless of whether ignorecase is true or false, git will read the
a/.gitattributes file and use it.

Then if you have:

   $ cat a/.gitattributes
   b/c test=a/b/c

then you should get the following results:

   # the case of a/ does not affect the attr check
   $ git -c core.ignorecase=0 check-attr a/b/c
   a/b/c: test: a/b/c
   $ git -c core.ignorecase=0 check-attr A/b/c
   A/b/c: test: a/b/c
   $ git -c core.ignorecase=0 check-attr a/B/c
   a/B/c: test: unspecified
   $ git -c core.ignorecase=1 check-attr a/B/c
   a/B/c: test: a/b/c
   $ git -c core.ignorecase=0 check-attr A/B/c
   A/B/c: test: unspecified
   $ git -c core.ignorecase=1 check-attr A/B/c
   A/B/c: test: a/b/c
   etc.

On a case-sensitive filesystem, a/.gitattributes would never be read
if A/b/c was supplied, regardless of core.ignorecase.

This is also partly future-proofing.  Currently, git builds the attr
stack based on the path supplied by the user, so we don't have to do
anything special (like use strcmp_icase) to handle the parts of that
path that don't match the filesystem with respect to case.  If git
instead built the attr stack by scanning the repository, then the
paths in the origin field would not necessarily match the paths
supplied by the user.  If someone makes a change like that in the
future, these tests will notice.

I'm sure there is something subtle that I'm missing. Can you explain it
either here or in the commit message?

Yeah, that commit message was really just a place-holder.  I meant to
add WIP in the subject field of the last patch too.  I'll try to
explain some of the above when I reroll.

-Brandon