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

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

[Brandon Casey]

On 09/13/2011 11:22 AM, Brandon Casey wrote:
> On 09/13/2011 11:05 AM, Jeff King wrote:
>> On Tue, Sep 13, 2011 at 10:15:15AM -0500, Brandon Casey wrote:
>>
>>> ...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

[PATCH 1/4] attr.c: avoid inappropriate access to strbuf "buf" member

[Brandon Casey]

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 <drafnel@gmail.com>
---
 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

[PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[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 <drafnel@gmail.com>
---
 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

[PATCH 3/4] builtin/mv.c: plug miniscule memory leak

[Brandon Casey]

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

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 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

[PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns

[Brandon Casey]

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

Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> 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;
>    }

I think it is immensely preferrable to do this than cascading to
default_config like this patch does and then piling band-aid on top to fix
the breakage caused by calling default_config.

An alternative approach may be to move reading of core.attributesfile to
default_config, and drop git_config() call from bootstrap_attr_stack(),
getting rid of git_attr_config callback altogether.

Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Brandon Casey <drafnel@gmail.com> writes:
>
>> 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;
>>    }
>
> I think it is immensely preferrable to do this than cascading to
> default_config like this patch does and then piling band-aid on top to fix
> the breakage caused by calling default_config.
>
> An alternative approach may be to move reading of core.attributesfile to
> default_config, and drop git_config() call from bootstrap_attr_stack(),
> getting rid of git_attr_config callback altogether.

That is, something like this on top of your patch.



 attr.c               |   15 ++-------------
 builtin/check-attr.c |    2 ++
 cache.h              |    1 +
 config.c             |    3 +++
 environment.c        |    1 +
 5 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/attr.c b/attr.c
index 79fb11e..76b079f 100644
--- a/attr.c
+++ b/attr.c
@@ -21,8 +21,6 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 #define ATTR__UNSET NULL
 #define ATTR__UNKNOWN git_attr__unknown
 
-static const char *attributes_file;
-
 /* This is a randomly chosen prime. */
 #define HASHSIZE 257
 
@@ -495,14 +493,6 @@ static int git_attr_system(void)
 	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
 }
 
-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 git_default_config(var, value, dummy);
-}
-
 static void bootstrap_attr_stack(void)
 {
 	if (!attr_stack) {
@@ -522,9 +512,8 @@ static void bootstrap_attr_stack(void)
 			}
 		}
 
-		git_config(git_attr_config, NULL);
-		if (attributes_file) {
-			elem = read_attr_from_file(attributes_file, 1);
+		if (git_attributes_file) {
+			elem = read_attr_from_file(git_attributes_file, 1);
 			if (elem) {
 				elem->origin = NULL;
 				elem->prev = attr_stack;
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 708988a..abb1165 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -92,6 +92,8 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 	struct git_attr_check *check;
 	int cnt, i, doubledash, filei;
 
+	git_config(git_default_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, check_attr_options,
 			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
 
diff --git a/cache.h b/cache.h
index 607c2ea..8d95fb2 100644
--- a/cache.h
+++ b/cache.h
@@ -589,6 +589,7 @@ extern int warn_ambiguous_refs;
 extern int shared_repository;
 extern const char *apply_default_whitespace;
 extern const char *apply_default_ignorewhitespace;
+extern const char *git_attributes_file;
 extern int zlib_compression_level;
 extern int core_compression_level;
 extern int core_compression_seen;
diff --git a/config.c b/config.c
index 4183f80..d3bcaa0 100644
--- a/config.c
+++ b/config.c
@@ -491,6 +491,9 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.attributesfile"))
+		return git_config_pathname(&git_attributes_file, var, value);
+
 	if (!strcmp(var, "core.bare")) {
 		is_bare_repository_cfg = git_config_bool(var, value);
 		return 0;
diff --git a/environment.c b/environment.c
index e96edcf..d60b73f 100644
--- a/environment.c
+++ b/environment.c
@@ -29,6 +29,7 @@ const char *git_log_output_encoding;
 int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace;
 const char *apply_default_ignorewhitespace;
+const char *git_attributes_file;
 int zlib_compression_level = Z_BEST_SPEED;
 int core_compression_level;
 int core_compression_seen;

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Johannes Sixt]

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 <drafnel@gmail.com>
> ---
>  ...
>  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

Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns

[Brandon Casey]

On Wed, Sep 14, 2011 at 11:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:

> > An alternative approach may be to move reading of core.attributesfile to
> > default_config, and drop git_config() call from bootstrap_attr_stack(),
> > getting rid of git_attr_config callback altogether.
>
> That is, something like this on top of your patch.

Ok.

I'll send a reroll that slides this underneath my top patch, and
addresses Hanne's comment.

-Brandon


>  attr.c               |   15 ++-------------
>  builtin/check-attr.c |    2 ++
>  cache.h              |    1 +
>  config.c             |    3 +++
>  environment.c        |    1 +
>  5 files changed, 9 insertions(+), 13 deletions(-)

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Brandon Casey]

On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> 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 <drafnel@gmail.com>
>> ---
>>  ...
>>  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

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

[Jeff King]

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

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

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

[Brandon Casey]

On Thu, Sep 15, 2011 at 1:12 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Sep 14, 2011 at 08:59:35PM -0500, Brandon Casey wrote:
>
>> > 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

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Brandon Casey]

[resend without html bits added by "gmail offline"]

So, it seems that of all of Google's email clients, only full desktop
gmail allows you to send plain text email (if you're careful to make
sure "Rich formatting" has not been clicked).  The new offline gmail
sends html, gmail android app sends html, gmail mobile web sends html.
 Google's war on plain text continues...

Or have I overlooked the switch that makes gmail send plain text and
only plain text?

On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
> On Thursday, September 15, 2011, Brandon Casey wrote:
>>
>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>
>> wrote:
>> > 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 <drafnel@gmail.com>
>> >> ---
>> >>  ...
>> >>  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.
>
> Hi Johannes,
> I have taken a closer look at the possibility of recursion with respect to
> die() and the functions in compat/.  It appears the risk is only related to
> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et
> al from within snprintf.c, I think we should be safe from recursion.
> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they
> both already use the x-wrappers or strbuf, even though they could easily be
> worked around.  The other file that was touched is compat/qsort, which
> returns void and doesn't have a good alternative error handling path.  So,
> I'm inclined to keep that one too.
> Sound reasonable?
> -Brandon
>

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Johannes Sixt]

Am 10/6/2011 4:00, schrieb Brandon Casey:
> [resend without html bits added by "gmail offline"]
> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
>> On Thursday, September 15, 2011, Brandon Casey wrote:
>>>
>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>
>>>> 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.
>>
>> Hi Johannes,
>> I have taken a closer look at the possibility of recursion with respect to
>> die() and the functions in compat/.  It appears the risk is only related to
>> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et
>> al from within snprintf.c, I think we should be safe from recursion.
>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they
>> both already use the x-wrappers or strbuf, even though they could easily be
>> worked around.  The other file that was touched is compat/qsort, which
>> returns void and doesn't have a good alternative error handling path.  So,
>> I'm inclined to keep that one too.

I'm fine with keeping the change to mingw.c (getaddrinfo related) and
qsort: both are unlikely to be called from die().

But syslog() *is* called from die() in git-daemon, and it would be better
to back out the other offenders instead of adding to them.

-- Hannes

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Alexey Shumkin]

Offtopic:
Excuse me, guys
May I ask you to exclude me from CC-list for this topic,
it seems to me I got in it for a mistake somehow,
I have no relation to this topic

thanks in advance :)

> Am 10/6/2011 4:00, schrieb Brandon Casey:
> > [resend without html bits added by "gmail offline"]
> > On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com>
> > wrote:
> >> On Thursday, September 15, 2011, Brandon Casey wrote:
> >>>
> >>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt
> >>> <j.sixt@viscovery.net>
> >>>> 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.
> >>
> >> Hi Johannes,
> >> I have taken a closer look at the possibility of recursion with
> >> respect to die() and the functions in compat/.  It appears the
> >> risk is only related to vsnprintf/snprintf at the moment.  So as
> >> long as we avoid calling xmalloc et al from within snprintf.c, I
> >> think we should be safe from recursion. I'm inclined to keep the
> >> additions to mingw.c and win32/syslog.c since they both already
> >> use the x-wrappers or strbuf, even though they could easily be
> >> worked around.  The other file that was touched is compat/qsort,
> >> which returns void and doesn't have a good alternative error
> >> handling path.  So, I'm inclined to keep that one too.
> 
> I'm fine with keeping the change to mingw.c (getaddrinfo related) and
> qsort: both are unlikely to be called from die().
> 
> But syslog() *is* called from die() in git-daemon, and it would be
> better to back out the other offenders instead of adding to them.
> 
> -- Hannes
> 

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Brandon Casey]

[removed Alexey Shumkin from cc]

On Thu, Oct 6, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 10/6/2011 4:00, schrieb Brandon Casey:
>> [resend without html bits added by "gmail offline"]
>> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
>>> On Thursday, September 15, 2011, Brandon Casey wrote:
>>>>
>>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>
>>>>> 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.
>>>
>>> Hi Johannes,
>>> I have taken a closer look at the possibility of recursion with respect to
>>> die() and the functions in compat/.  It appears the risk is only related to
>>> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et
>>> al from within snprintf.c, I think we should be safe from recursion.
>>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they
>>> both already use the x-wrappers or strbuf, even though they could easily be
>>> worked around.  The other file that was touched is compat/qsort, which
>>> returns void and doesn't have a good alternative error handling path.  So,
>>> I'm inclined to keep that one too.
>
> I'm fine with keeping the change to mingw.c (getaddrinfo related) and
> qsort: both are unlikely to be called from die().
>
> But syslog() *is* called from die() in git-daemon, and it would be better
> to back out the other offenders instead of adding to them.

Ah.  Yes, you're right.  x-wrappers should not be used in syslog.c and
the use of strbuf's should be replaced.

Thanks,
-Brandon

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Erik Faye-Lund]

On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:
> [removed Alexey Shumkin from cc]
>
> On Thu, Oct 6, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Am 10/6/2011 4:00, schrieb Brandon Casey:
>>> [resend without html bits added by "gmail offline"]
>>> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
>>>> On Thursday, September 15, 2011, Brandon Casey wrote:
>>>>>
>>>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>
>>>>>> 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.
>>>>
>>>> Hi Johannes,
>>>> I have taken a closer look at the possibility of recursion with respect to
>>>> die() and the functions in compat/.  It appears the risk is only related to
>>>> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et
>>>> al from within snprintf.c, I think we should be safe from recursion.
>>>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they
>>>> both already use the x-wrappers or strbuf, even though they could easily be
>>>> worked around.  The other file that was touched is compat/qsort, which
>>>> returns void and doesn't have a good alternative error handling path.  So,
>>>> I'm inclined to keep that one too.
>>
>> I'm fine with keeping the change to mingw.c (getaddrinfo related) and
>> qsort: both are unlikely to be called from die().
>>
>> But syslog() *is* called from die() in git-daemon, and it would be better
>> to back out the other offenders instead of adding to them.
>
> Ah.  Yes, you're right.  x-wrappers should not be used in syslog.c and
> the use of strbuf's should be replaced.

Good point. The patch for this looks something like this:

diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 42b95a9..243538c 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -1,5 +1,4 @@
 #include "../../git-compat-util.h"
-#include "../../strbuf.h"

 static HANDLE ms_eventlog;

@@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)

 void syslog(int priority, const char *fmt, ...)
 {
-	struct strbuf sb = STRBUF_INIT;
-	struct strbuf_expand_dict_entry dict[] = {
-		{"1", "% 1"},
-		{NULL, NULL}
-	};
 	WORD logtype;
-	char *str;
+	char *str, *pos;
 	int str_len;
 	va_list ap;

@@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
 	}

 	str = malloc(str_len + 1);
+	if (!str)
+		return; /* no chance to report error */
+
 	va_start(ap, fmt);
 	vsnprintf(str, str_len + 1, fmt, ap);
 	va_end(ap);
-	strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
-	free(str);
+
+	while ((pos = strstr(str, "%1")) != NULL) {
+		str = realloc(str, ++str_len + 1);
+		if (!str)
+			return;
+		memmove(pos + 2, pos + 1, strlen(pos));
+		pos[1] = ' ';
+	}

 	switch (priority) {
 	case LOG_EMERG:
@@ -66,7 +69,5 @@ void syslog(int priority, const char *fmt, ...)
 	}

 	ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,
-	    (const char **)&sb.buf, NULL);
-
-	strbuf_release(&sb);
+	    (const char **)&str, NULL);
 }

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Erik Faye-Lund]

On Thu, Oct 6, 2011 at 6:50 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:
>> Ah.  Yes, you're right.  x-wrappers should not be used in syslog.c and
>> the use of strbuf's should be replaced.
>
> Good point. The patch for this looks something like this:
>
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 42b95a9..243538c 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -1,5 +1,4 @@
>  #include "../../git-compat-util.h"
> -#include "../../strbuf.h"
>
>  static HANDLE ms_eventlog;
>
> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
>
>  void syslog(int priority, const char *fmt, ...)
>  {
> -       struct strbuf sb = STRBUF_INIT;
> -       struct strbuf_expand_dict_entry dict[] = {
> -               {"1", "% 1"},
> -               {NULL, NULL}
> -       };
>        WORD logtype;
> -       char *str;
> +       char *str, *pos;
>        int str_len;
>        va_list ap;
>
> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
>        }
>
>        str = malloc(str_len + 1);
> +       if (!str)
> +               return; /* no chance to report error */
> +
>        va_start(ap, fmt);
>        vsnprintf(str, str_len + 1, fmt, ap);
>        va_end(ap);
> -       strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
> -       free(str);
> +
> +       while ((pos = strstr(str, "%1")) != NULL) {
> +               str = realloc(str, ++str_len + 1);
> +               if (!str)
> +                       return;
> +               memmove(pos + 2, pos + 1, strlen(pos));
> +               pos[1] = ' ';
> +       }
>
>        switch (priority) {
>        case LOG_EMERG:
> @@ -66,7 +69,5 @@ void syslog(int priority, const char *fmt, ...)
>        }
>
>        ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,
> -           (const char **)&sb.buf, NULL);
> -
> -       strbuf_release(&sb);
> +           (const char **)&str, NULL);
>  }
>

...aaand this on top to avoid a leak, of course:

diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 243538c..3b8e2b7 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -70,4 +70,5 @@ void syslog(int priority, const char *fmt, ...)

 	ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,
 	    (const char **)&str, NULL);
+	free(str);
 }

Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere

[Brandon Casey]

On Thu, Oct 6, 2011 at 11:50 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:
>> [removed Alexey Shumkin from cc]
>>
>> On Thu, Oct 6, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Am 10/6/2011 4:00, schrieb Brandon Casey:
>>>> [resend without html bits added by "gmail offline"]
>>>> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
>>>>> On Thursday, September 15, 2011, Brandon Casey wrote:
>>>>>>
>>>>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>
>>>>>>> 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.
>>>>>
>>>>> Hi Johannes,
>>>>> I have taken a closer look at the possibility of recursion with respect to
>>>>> die() and the functions in compat/.  It appears the risk is only related to
>>>>> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et
>>>>> al from within snprintf.c, I think we should be safe from recursion.
>>>>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they
>>>>> both already use the x-wrappers or strbuf, even though they could easily be
>>>>> worked around.  The other file that was touched is compat/qsort, which
>>>>> returns void and doesn't have a good alternative error handling path.  So,
>>>>> I'm inclined to keep that one too.
>>>
>>> I'm fine with keeping the change to mingw.c (getaddrinfo related) and
>>> qsort: both are unlikely to be called from die().
>>>
>>> But syslog() *is* called from die() in git-daemon, and it would be better
>>> to back out the other offenders instead of adding to them.
>>
>> Ah.  Yes, you're right.  x-wrappers should not be used in syslog.c and
>> the use of strbuf's should be replaced.
>
> Good point. The patch for this looks something like this:
>
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 42b95a9..243538c 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -1,5 +1,4 @@
>  #include "../../git-compat-util.h"
> -#include "../../strbuf.h"
>
>  static HANDLE ms_eventlog;
>
> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
>
>  void syslog(int priority, const char *fmt, ...)
>  {
> -       struct strbuf sb = STRBUF_INIT;
> -       struct strbuf_expand_dict_entry dict[] = {
> -               {"1", "% 1"},
> -               {NULL, NULL}
> -       };
>        WORD logtype;
> -       char *str;
> +       char *str, *pos;
>        int str_len;
>        va_list ap;
>
> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
>        }
>
>        str = malloc(str_len + 1);
> +       if (!str)
> +               return; /* no chance to report error */
> +
>        va_start(ap, fmt);
>        vsnprintf(str, str_len + 1, fmt, ap);
>        va_end(ap);
> -       strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
> -       free(str);
> +
> +       while ((pos = strstr(str, "%1")) != NULL) {
> +               str = realloc(str, ++str_len + 1);
> +               if (!str)
> +                       return;
> +               memmove(pos + 2, pos + 1, strlen(pos));
> +               pos[1] = ' ';
> +       }

Is there any reason not to just use a different character than '%'
when replacing '%n'?  Like '@'?  Then the replacement could be done
in-place.

e.g. convert this:

   fe80::3%1

into this:

   fe80::3@1

I'm not usually on a windows platform, so maybe adding the space is
the common thing to do when trying to write an ipv6 address to the
event log using ReportEvent().  If not, then I think people would
probably figure out easily enough that '@n' referred to interface 'n'.

-Brandon