{"thread":{"id":"28392","subject":"[PATCH 0/4] Honor core.ignorecase for attribute patterns","startedAt":"2011-09-15T01:59:35Z","lastAt":"2011-10-06T17:17:24Z","messageCount":19,"participants":["Brandon Casey","Junio C Hamano","Johannes Sixt","Jeff King","Alexey Shumkin","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"175533","messageId":"1316051979-19671-1-git-send-email-drafnel@gmail.com","threadId":"28392","inReplyTo":"5XXEFw0WjtXKd9dpXSxpkskCcgVyG9Db1_zzVSEBNey-kpXSBbmQfYaxZ2Szg6Pbck6hZZTQ5hHzBwG4rhKYXshrdmveEFLPZ9W0V8P_lw@cipher.nrlssc.navy.mil","subject":"[PATCH 0/4] Honor core.ignorecase for attribute patterns","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-09-15T01:59:35Z","receivedAt":"2011-09-15T01:59:35Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On 09/13/2011 11:22 AM, Brandon Casey wrote:\n> On 09/13/2011 11:05 AM, Jeff King wrote:\n>> On Tue, Sep 13, 2011 at 10:15:15AM -0500, Brandon Casey wrote:\n>>\n>>> ...and I see there is already an fnmatch_icase() in dir.c which adds\n>>> FNM_CASEFOLD when the global var ignore_case is set.  So, maybe it's as\n>>> easy as:\n>>> [...]\n>>> -               return (fnmatch(pattern, basename, 0) == 0);\n>>> +               return (fnmatch_icase(pattern, basename, 0) == 0);\n>>\n>> OK, wow. That's exactly the level of easy I was hoping for. Do you want\n>> to roll that up into a patch with some tests?\n>\n> I haven't even tested that it runs. :)  No, I was hoping someone\n> who was more interested would finish it, and maybe even test on\n> an affected system.\n\nOk, I lied.  Here's a series that needs testing by people on a  \ncase-insensitive filesystem and some comments.\n\nThe first three patches are just housekeeping and can be accepted \nindependently of the fourth patch which is marked WIP.\n\nThe last patch implements the case-insensitive matching of attribute\npatterns, but I discovered that bad things can happen if git_config()\nis called more than once.  Details are in the patch email.\n\n-Brandon\n\n[PATCH 1/4] attr.c: avoid inappropriate access to strbuf \"buf\"\n[PATCH 2/4] cleanup: use internal memory allocation wrapper\n[PATCH 3/4] builtin/mv.c: plug miniscule memory leak\n[PATCH 4/4] attr.c: respect core.ignorecase when matching attribute\n"},{"id":"175534","messageId":"1316051979-19671-2-git-send-email-drafnel@gmail.com","threadId":"28392","inReplyTo":"1316051979-19671-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 1/4] attr.c: avoid inappropriate access to strbuf \"buf\" member","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-09-15T01:59:36Z","receivedAt":"2011-09-15T01:59:36Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"This code sequence performs a strcpy into the buf member of a strbuf\nstruct.  The strcpy may move the position of the terminating nul of the\nstring and effectively change the length of string so that it does not\nmatch the len member of the strbuf struct.\n\nCurrently, this sequence works since the strbuf was given a hint when it\nwas initialized to allocate enough space to accomodate the string that will\nbe strcpy'ed, but this is an implementation detail of strbufs, not a\nguarantee.\n\nSo, lets rework this sequence so that the strbuf is only manipulated by\nstrbuf functions, and direct modification of its \"buf\" member is not\nnecessary.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n attr.c |   25 ++++++++++++-------------\n 1 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 33cb4e4..206f233 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -552,7 +552,6 @@ static void prepare_attr_stack(const char *path)\n {\n \tstruct attr_stack *elem, *info;\n \tint dirlen, len;\n-\tstruct strbuf pathbuf;\n \tconst char *cp;\n \n \tcp = strrchr(path, '/');\n@@ -561,8 +560,6 @@ static void prepare_attr_stack(const char *path)\n \telse\n \t\tdirlen = cp - path;\n \n-\tstrbuf_init(&pathbuf, dirlen+2+strlen(GITATTRIBUTES_FILE));\n-\n \t/*\n \t * At the bottom of the attribute stack is the built-in\n \t * set of attribute definitions, followed by the contents\n@@ -607,27 +604,29 @@ static void prepare_attr_stack(const char *path)\n \t * Read from parent directories and push them down\n \t */\n \tif (!is_bare_repository() || direction == GIT_ATTR_INDEX) {\n-\t\twhile (1) {\n-\t\t\tchar *cp;\n+\t\tstruct strbuf pathbuf = STRBUF_INIT;\n \n+\t\twhile (1) {\n \t\t\tlen = strlen(attr_stack->origin);\n \t\t\tif (dirlen <= len)\n \t\t\t\tbreak;\n-\t\t\tstrbuf_reset(&pathbuf);\n-\t\t\tstrbuf_add(&pathbuf, path, dirlen);\n+\t\t\tcp = memchr(path + len + 1, '/', dirlen - len - 1);\n+\t\t\tif (!cp)\n+\t\t\t\tcp = path + dirlen;\n+\t\t\tstrbuf_add(&pathbuf, path, cp - path);\n \t\t\tstrbuf_addch(&pathbuf, '/');\n-\t\t\tcp = strchr(pathbuf.buf + len + 1, '/');\n-\t\t\tstrcpy(cp + 1, GITATTRIBUTES_FILE);\n+\t\t\tstrbuf_add(&pathbuf, GITATTRIBUTES_FILE,\n+\t\t\t\tstrlen(GITATTRIBUTES_FILE));\n \t\t\telem = read_attr(pathbuf.buf, 0);\n-\t\t\t*cp = '\\0';\n-\t\t\telem->origin = strdup(pathbuf.buf);\n+\t\t\tstrbuf_setlen(&pathbuf, cp - path);\n+\t\t\telem->origin = strbuf_detach(&pathbuf, NULL);\n \t\t\telem->prev = attr_stack;\n \t\t\tattr_stack = elem;\n \t\t\tdebug_push(elem);\n \t\t}\n-\t}\n \n-\tstrbuf_release(&pathbuf);\n+\t\tstrbuf_release(&pathbuf);\n+\t}\n \n \t/*\n \t * Finally push the \"info\" one at the top of the stack.\n-- \n1.7.6.2\n"},{"id":"175535","messageId":"1316051979-19671-3-git-send-email-drafnel@gmail.com","threadId":"28392","inReplyTo":"1316051979-19671-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-09-15T01:59:37Z","receivedAt":"2011-09-15T01:59:37Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"The \"x\"-prefixed versions of strdup, malloc, etc. will check whether the\nallocation was successful and terminate the process otherwise.\n\nA few uses of malloc were left alone since they already implemented a\ngraceful path of failure or were in a quasi external library like xdiff.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n attr.c                |    2 +-\n builtin/mv.c          |    2 +-\n compat/mingw.c        |    2 +-\n compat/qsort.c        |    2 +-\n compat/win32/syslog.c |    2 +-\n remote.c              |    2 +-\n show-index.c          |    2 +-\n transport-helper.c    |    4 ++--\n 8 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 206f233..3359b39 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -533,7 +533,7 @@ static void bootstrap_attr_stack(void)\n \n \t\tif (!is_bare_repository() || direction == GIT_ATTR_INDEX) {\n \t\t\telem = read_attr(GITATTRIBUTES_FILE, 1);\n-\t\t\telem->origin = strdup(\"\");\n+\t\t\telem->origin = xstrdup(\"\");\n \t\t\telem->prev = attr_stack;\n \t\t\tattr_stack = elem;\n \t\t\tdebug_push(elem);\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 40f33ca..e9d191f 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -29,7 +29,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n \t\t\tto_copy--;\n \t\tif (to_copy != length || base_name) {\n \t\t\tchar *it = xmemdupz(result[i], to_copy);\n-\t\t\tresult[i] = base_name ? strdup(basename(it)) : it;\n+\t\t\tresult[i] = base_name ? xstrdup(basename(it)) : it;\n \t\t}\n \t}\n \treturn get_pathspec(prefix, result);\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 6ef0cc4..8947418 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1183,7 +1183,7 @@ static int WSAAPI getaddrinfo_stub(const char *node, const char *service,\n \t}\n \tai->ai_addrlen = sizeof(struct sockaddr_in);\n \tif (hints && (hints->ai_flags & AI_CANONNAME))\n-\t\tai->ai_canonname = h ? strdup(h->h_name) : NULL;\n+\t\tai->ai_canonname = h ? xstrdup(h->h_name) : NULL;\n \telse\n \t\tai->ai_canonname = NULL;\n \ndiff --git a/compat/qsort.c b/compat/qsort.c\nindex d93dce2..9574d53 100644\n--- a/compat/qsort.c\n+++ b/compat/qsort.c\n@@ -55,7 +55,7 @@ void git_qsort(void *b, size_t n, size_t s,\n \t\tmsort_with_tmp(b, n, s, cmp, buf);\n \t} else {\n \t\t/* It's somewhat large, so malloc it.  */\n-\t\tchar *tmp = malloc(size);\n+\t\tchar *tmp = xmalloc(size);\n \t\tmsort_with_tmp(b, n, s, cmp, tmp);\n \t\tfree(tmp);\n \t}\ndiff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\nindex 42b95a9..197ab34 100644\n--- a/compat/win32/syslog.c\n+++ b/compat/win32/syslog.c\n@@ -38,7 +38,7 @@ void syslog(int priority, const char *fmt, ...)\n \t\treturn;\n \t}\n \n-\tstr = malloc(str_len + 1);\n+\tstr = xmalloc(str_len + 1);\n \tva_start(ap, fmt);\n \tvsnprintf(str, str_len + 1, fmt, ap);\n \tva_end(ap);\ndiff --git a/remote.c b/remote.c\nindex b8ecfa5..7840d2f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -840,7 +840,7 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,\n \t\t\t\t\t\t    refspec->dst, &ret))\n \t\t\t\treturn ret;\n \t\t} else if (!strcmp(refspec->src, name))\n-\t\t\treturn strdup(refspec->dst);\n+\t\t\treturn xstrdup(refspec->dst);\n \t}\n \treturn NULL;\n }\ndiff --git a/show-index.c b/show-index.c\nindex 4c0ac13..63f9da5 100644\n--- a/show-index.c\n+++ b/show-index.c\n@@ -48,7 +48,7 @@ int main(int argc, char **argv)\n \t\t\tunsigned char sha1[20];\n \t\t\tuint32_t crc;\n \t\t\tuint32_t off;\n-\t\t} *entries = malloc(nr * sizeof(entries[0]));\n+\t\t} *entries = xmalloc(nr * sizeof(entries[0]));\n \t\tfor (i = 0; i < nr; i++)\n \t\t\tif (fread(entries[i].sha1, 20, 1, stdin) != 1)\n \t\t\t\tdie(\"unable to read sha1 %u/%u\", i, nr);\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4eab844..0713126 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -183,7 +183,7 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t\tALLOC_GROW(refspecs,\n \t\t\t\t   refspec_nr + 1,\n \t\t\t\t   refspec_alloc);\n-\t\t\trefspecs[refspec_nr++] = strdup(capname + strlen(\"refspec \"));\n+\t\t\trefspecs[refspec_nr++] = xstrdup(capname + strlen(\"refspec \"));\n \t\t} else if (!strcmp(capname, \"connect\")) {\n \t\t\tdata->connect = 1;\n \t\t} else if (!prefixcmp(capname, \"export-marks \")) {\n@@ -445,7 +445,7 @@ static int fetch_with_import(struct transport *transport,\n \t\tif (data->refspecs)\n \t\t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, posn->name);\n \t\telse\n-\t\t\tprivate = strdup(posn->name);\n+\t\t\tprivate = xstrdup(posn->name);\n \t\tread_ref(private, posn->old_sha1);\n \t\tfree(private);\n \t}\n-- \n1.7.6.2\n"},{"id":"175536","messageId":"1316051979-19671-4-git-send-email-drafnel@gmail.com","threadId":"28392","inReplyTo":"1316051979-19671-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 3/4] builtin/mv.c: plug miniscule memory leak","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-09-15T01:59:38Z","receivedAt":"2011-09-15T01:59:38Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"The \"it\" string would not be free'ed if base_name was non-NULL.\nLet's free it.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n builtin/mv.c |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex e9d191f..5efe6c5 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -29,7 +29,11 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n \t\t\tto_copy--;\n \t\tif (to_copy != length || base_name) {\n \t\t\tchar *it = xmemdupz(result[i], to_copy);\n-\t\t\tresult[i] = base_name ? xstrdup(basename(it)) : it;\n+\t\t\tif (base_name) {\n+\t\t\t\tresult[i] = xstrdup(basename(it));\n+\t\t\t\tfree(it);\n+\t\t\t} else\n+\t\t\t\tresult[i] = it;\n \t\t}\n \t}\n \treturn get_pathspec(prefix, result);\n-- \n1.7.6.2\n"},{"id":"175537","messageId":"1316051979-19671-5-git-send-email-drafnel@gmail.com","threadId":"28392","inReplyTo":"1316051979-19671-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-09-15T01:59:39Z","receivedAt":"2011-09-15T01:59:39Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When core.ignorecase is true, the file globs configured in the\n.gitattributes file should be matched case-insensitively against the paths\nin the working directory.\n\n---\n\nTwo points:\n\n1)\nI think these two changes of fnmatch to fnmatch_icase should be all\nthat is necessary.  There are a number of uses of strncmp where the\n\"origin\" path of an attribute entry is compared to the prefix of a file\npath, but since the attribute stack is built from the same path string\nthat it is being compared against, we shouldn't have to do\nstrncmp_icase everywhere.  The case of the two strings should\nnecessarily match.\n\nThis needs some testing by someone on a case-insensitive filesystem.\n\nAlso, notice some of the new tests are marked with a CASE_INSENSITIVE_FS\npre-requisite.  I tested on a USB thumb drive, but it would be nice if\nsomeone tested on a platform that is natively case-insensitive.  Maybe\nCASE_INSENSITIVE_FS should be moved to test-lib.sh, and t0005(others?)\nshould be updated to use it?\n\n\n2)\nThe bad news, this breaks t8005.  The breakage is caused by\ngit_attr_config calling git_default_config, and stomping on the\ngit_log_output_encoding set by setup_revisions() when it parsed the\n --encoding command line option.\n\nWhat happens is cmd_blame() calls git_config() which parses the config\nfiles and sets up the global config variables like\ngit_log_output_encoding, then later blame calls setup_revisions() which\nparses the command line option --encoding and overrides the value in\ngit_log_output_encoding, then, even later, userdiff looks up an\nattribute on a path and calls git_check_attr() which calls git_config\n_again_, which resets git_log_output_encoding to the value in the config\nfile (stomping on the value set by --encoding on the git blame command\nline).\n\nSince fnmatch_icase depends on the ignore_case global variable being set\ncorrectly, the obvious thing for me to do was to allow\ngit_default_config to call git_default_core_config and parse the\ncore.ignorecase config option.  So I modified git_attr_config so it fell\nback to git_default_config.  But that can have the undesired side effect\ndescribed above.\n\nIt's easy to work around this issue.  I could just parse core.ignorecase\nin git_attr_config() and set ignore_case myself like:\n\n   if (!strcmp(var, \"core.ignorecase\")) {\n           ignore_case = git_config_bool(var, value);\n           return 0;\n   }\n\nThe big question is whether it should be safe to call git_config()\nmultiple times?  Right now, it is not.  We also don't protect against\ngit_config() being called multiple times either.\n\nI suspect that setup_revisions() is not the only place where a command\nline option overrides a global config variable and it would be a big\ncan of worms to try to fix them all.\n\nThoughts?\n\n---\n attr.c                |    7 +++--\n t/t0003-attributes.sh |   60 ++++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 63 insertions(+), 4 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 3359b39..b8ed7cf 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -11,6 +11,7 @@\n #include \"cache.h\"\n #include \"exec_cmd.h\"\n #include \"attr.h\"\n+#include \"dir.h\"\n \n const char git_attr__true[] = \"(builtin)true\";\n const char git_attr__false[] = \"\\0(builtin)false\";\n@@ -499,7 +500,7 @@ static int git_attr_config(const char *var, const char *value, void *dummy)\n \tif (!strcmp(var, \"core.attributesfile\"))\n \t\treturn git_config_pathname(&attributes_file, var, value);\n \n-\treturn 0;\n+\treturn git_default_config(var, value, dummy);\n }\n \n static void bootstrap_attr_stack(void)\n@@ -643,7 +644,7 @@ static int path_matches(const char *pathname, int pathlen,\n \t\t/* match basename */\n \t\tconst char *basename = strrchr(pathname, '/');\n \t\tbasename = basename ? basename + 1 : pathname;\n-\t\treturn (fnmatch(pattern, basename, 0) == 0);\n+\t\treturn (fnmatch_icase(pattern, basename, 0) == 0);\n \t}\n \t/*\n \t * match with FNM_PATHNAME; the pattern has base implicitly\n@@ -657,7 +658,7 @@ static int path_matches(const char *pathname, int pathlen,\n \t\treturn 0;\n \tif (baselen != 0)\n \t\tbaselen++;\n-\treturn fnmatch(pattern, pathname + baselen, FNM_PATHNAME) == 0;\n+\treturn fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;\n }\n \n static int macroexpand_one(int attr_nr, int rem);\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex ae2f1da..47a70c4 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -9,7 +9,7 @@ attr_check () {\n \tpath=\"$1\"\n \texpect=\"$2\"\n \n-\tgit check-attr test -- \"$path\" >actual 2>err &&\n+\tgit $3 check-attr test -- \"$path\" >actual 2>err &&\n \techo \"$path: test: $2\" >expect &&\n \ttest_cmp expect actual &&\n \ttest_line_count = 0 err\n@@ -27,6 +27,7 @@ test_expect_success 'setup' '\n \t\techo \"onoff test -test\"\n \t\techo \"offon -test test\"\n \t\techo \"no notest\"\n+\t\techo \"A/e/F test=A/e/F\"\n \t) >.gitattributes &&\n \t(\n \t\techo \"g test=a/g\" &&\n@@ -93,6 +94,63 @@ test_expect_success 'attribute test' '\n \n '\n \n+test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '\n+\n+\ttest_must_fail attr_check F f \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/F f \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/c/F f \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/G a/g \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/B/g a/b/g \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/b/G a/b/g \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/b/H a/b/h \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/b/D/g \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check oNoFf unset \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check oFfOn set \"-c core.ignorecase=0\" &&\n+\tattr_check NO unspecified \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/b/D/NO \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/d/YES a/b/d/* \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check a/E/f \"A/e/F\" \"-c core.ignorecase=0\"\n+\n+'\n+\n+test_expect_success 'attribute matching is case insensitive when core.ignorecase=1' '\n+\n+\tattr_check F f \"-c core.ignorecase=1\" &&\n+\tattr_check a/F f \"-c core.ignorecase=1\" &&\n+\tattr_check a/c/F f \"-c core.ignorecase=1\" &&\n+\tattr_check a/G a/g \"-c core.ignorecase=1\" &&\n+\tattr_check a/B/g a/b/g \"-c core.ignorecase=1\" &&\n+\tattr_check a/b/G a/b/g \"-c core.ignorecase=1\" &&\n+\tattr_check a/b/H a/b/h \"-c core.ignorecase=1\" &&\n+\tattr_check a/b/D/g \"a/b/d/*\" \"-c core.ignorecase=1\" &&\n+\tattr_check oNoFf unset \"-c core.ignorecase=1\" &&\n+\tattr_check oFfOn set \"-c core.ignorecase=1\" &&\n+\tattr_check NO unspecified \"-c core.ignorecase=1\" &&\n+\tattr_check a/b/D/NO \"a/b/d/*\" \"-c core.ignorecase=1\" &&\n+\tattr_check a/b/d/YES unspecified \"-c core.ignorecase=1\" &&\n+\tattr_check a/E/f \"A/e/F\" \"-c core.ignorecase=1\"\n+\n+'\n+\n+test_expect_success 'check whether FS is case-insensitive' '\n+\tmkdir junk &&\n+\techo good >junk/CamelCase &&\n+\techo bad >junk/camelcase &&\n+\tif test \"$(cat junk/CamelCase)\" != good\n+\tthen\n+\t\ttest_set_prereq CASE_INSENSITIVE_FS\n+\tfi\n+'\n+\n+test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '\n+\ttest_must_fail attr_check a/B/D/g \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\ttest_must_fail attr_check A/B/D/NO \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\tattr_check A/b/h a/b/h \"-c core.ignorecase=0\" &&\n+\tattr_check A/b/h a/b/h \"-c core.ignorecase=1\" &&\n+\tattr_check a/B/D/g \"a/b/d/*\" \"-c core.ignorecase=1\" &&\n+\tattr_check A/B/D/NO \"a/b/d/*\" \"-c core.ignorecase=1\"\n+'\n+\n test_expect_success 'unnormalized paths' '\n \n \tattr_check ./f f &&\n-- \n1.7.6\n"},{"id":"175544","messageId":"7vsjnyqws3.fsf@alter.siamese.dyndns.org","threadId":"28392","inReplyTo":"1316051979-19671-5-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-15T04:01:16Z","receivedAt":"2011-09-15T04:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey <drafnel@gmail.com> writes:\n\n> It's easy to work around this issue.  I could just parse core.ignorecase\n> in git_attr_config() and set ignore_case myself like:\n>\n>    if (!strcmp(var, \"core.ignorecase\")) {\n>            ignore_case = git_config_bool(var, value);\n>            return 0;\n>    }\n\nI think it is immensely preferrable to do this than cascading to\ndefault_config like this patch does and then piling band-aid on top to fix\nthe breakage caused by calling default_config.\n\nAn alternative approach may be to move reading of core.attributesfile to\ndefault_config, and drop git_config() call from bootstrap_attr_stack(),\ngetting rid of git_attr_config callback altogether.\n"},{"id":"175545","messageId":"7vobymqwjf.fsf@alter.siamese.dyndns.org","threadId":"28392","inReplyTo":"7vsjnyqws3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-15T04:06:28Z","receivedAt":"2011-09-15T04:06:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Brandon Casey <drafnel@gmail.com> writes:\n>\n>> It's easy to work around this issue.  I could just parse core.ignorecase\n>> in git_attr_config() and set ignore_case myself like:\n>>\n>>    if (!strcmp(var, \"core.ignorecase\")) {\n>>            ignore_case = git_config_bool(var, value);\n>>            return 0;\n>>    }\n>\n> I think it is immensely preferrable to do this than cascading to\n> default_config like this patch does and then piling band-aid on top to fix\n> the breakage caused by calling default_config.\n>\n> An alternative approach may be to move reading of core.attributesfile to\n> default_config, and drop git_config() call from bootstrap_attr_stack(),\n> getting rid of git_attr_config callback altogether.\n\nThat is, something like this on top of your patch.\n\n\n\n attr.c               |   15 ++-------------\n builtin/check-attr.c |    2 ++\n cache.h              |    1 +\n config.c             |    3 +++\n environment.c        |    1 +\n 5 files changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 79fb11e..76b079f 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -21,8 +21,6 @@ static const char git_attr__unknown[] = \"(builtin)unknown\";\n #define ATTR__UNSET NULL\n #define ATTR__UNKNOWN git_attr__unknown\n \n-static const char *attributes_file;\n-\n /* This is a randomly chosen prime. */\n #define HASHSIZE 257\n \n@@ -495,14 +493,6 @@ static int git_attr_system(void)\n \treturn !git_env_bool(\"GIT_ATTR_NOSYSTEM\", 0);\n }\n \n-static int git_attr_config(const char *var, const char *value, void *dummy)\n-{\n-\tif (!strcmp(var, \"core.attributesfile\"))\n-\t\treturn git_config_pathname(&attributes_file, var, value);\n-\n-\treturn git_default_config(var, value, dummy);\n-}\n-\n static void bootstrap_attr_stack(void)\n {\n \tif (!attr_stack) {\n@@ -522,9 +512,8 @@ static void bootstrap_attr_stack(void)\n \t\t\t}\n \t\t}\n \n-\t\tgit_config(git_attr_config, NULL);\n-\t\tif (attributes_file) {\n-\t\t\telem = read_attr_from_file(attributes_file, 1);\n+\t\tif (git_attributes_file) {\n+\t\t\telem = read_attr_from_file(git_attributes_file, 1);\n \t\t\tif (elem) {\n \t\t\t\telem->origin = NULL;\n \t\t\t\telem->prev = attr_stack;\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 708988a..abb1165 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -92,6 +92,8 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \tstruct git_attr_check *check;\n \tint cnt, i, doubledash, filei;\n \n+\tgit_config(git_default_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, check_attr_options,\n \t\t\t     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n \ndiff --git a/cache.h b/cache.h\nindex 607c2ea..8d95fb2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -589,6 +589,7 @@ extern int warn_ambiguous_refs;\n extern int shared_repository;\n extern const char *apply_default_whitespace;\n extern const char *apply_default_ignorewhitespace;\n+extern const char *git_attributes_file;\n extern int zlib_compression_level;\n extern int core_compression_level;\n extern int core_compression_seen;\ndiff --git a/config.c b/config.c\nindex 4183f80..d3bcaa0 100644\n--- a/config.c\n+++ b/config.c\n@@ -491,6 +491,9 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.attributesfile\"))\n+\t\treturn git_config_pathname(&git_attributes_file, var, value);\n+\n \tif (!strcmp(var, \"core.bare\")) {\n \t\tis_bare_repository_cfg = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex e96edcf..d60b73f 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -29,6 +29,7 @@ const char *git_log_output_encoding;\n int shared_repository = PERM_UMASK;\n const char *apply_default_whitespace;\n const char *apply_default_ignorewhitespace;\n+const char *git_attributes_file;\n int zlib_compression_level = Z_BEST_SPEED;\n int core_compression_level;\n int core_compression_seen;\n"},{"id":"175555","messageId":"4E71A0C7.8080602@viscovery.net","threadId":"28392","inReplyTo":"1316051979-19671-3-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-09-15T06:52:55Z","receivedAt":"2011-09-15T06:52:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/15/2011 3:59, schrieb Brandon Casey:\n> The \"x\"-prefixed versions of strdup, malloc, etc. will check whether the\n> allocation was successful and terminate the process otherwise.\n> \n> A few uses of malloc were left alone since they already implemented a\n> graceful path of failure or were in a quasi external library like xdiff.\n> \n> Signed-off-by: Brandon Casey <drafnel@gmail.com>\n> ---\n>  ...\n>  compat/mingw.c        |    2 +-\n>  compat/qsort.c        |    2 +-\n>  compat/win32/syslog.c |    2 +-\n\nThere is a danger that the high-level die() routine (which is used by the\nx-wrappers) uses one of the low-level compat/ routines. IOW, in the case\nof errors, recursion might occur. Therefore, I would prefer that the\ncompat/ routines do their own error reporting (preferably via return\nvalues and errno).\n\n-- Hannes\n"},{"id":"175584","messageId":"CA+sFfMd7B6suV1YQppJGJmoCr+du3-GaG5dFhp=DyEngb-wR4g@mail.gmail.com","threadId":"28392","inReplyTo":"7vobymqwjf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-09-15T15:38:23Z","receivedAt":"2011-09-15T15:38:23Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Sep 14, 2011 at 11:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n\n> > An alternative approach may be to move reading of core.attributesfile to\n> > default_config, and drop git_config() call from bootstrap_attr_stack(),\n> > getting rid of git_attr_config callback altogether.\n>\n> That is, something like this on top of your patch.\n\nOk.\n\nI'll send a reroll that slides this underneath my top patch, and\naddresses Hanne's comment.\n\n-Brandon\n\n\n>  attr.c               |   15 ++-------------\n>  builtin/check-attr.c |    2 ++\n>  cache.h              |    1 +\n>  config.c             |    3 +++\n>  environment.c        |    1 +\n>  5 files changed, 9 insertions(+), 13 deletions(-)\n"},{"id":"175585","messageId":"CA+sFfMdVntk+U13UeMO=k1SCKJGhPfTpC9_i9kFOkbUJXrF-qg@mail.gmail.com","threadId":"28392","inReplyTo":"4E71A0C7.8080602@viscovery.net","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-09-15T15:39:50Z","receivedAt":"2011-09-15T15:39:50Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Am 9/15/2011 3:59, schrieb Brandon Casey:\n>> The \"x\"-prefixed versions of strdup, malloc, etc. will check whether the\n>> allocation was successful and terminate the process otherwise.\n>>\n>> A few uses of malloc were left alone since they already implemented a\n>> graceful path of failure or were in a quasi external library like xdiff.\n>>\n>> Signed-off-by: Brandon Casey <drafnel@gmail.com>\n>> ---\n>>  ...\n>>  compat/mingw.c        |    2 +-\n>>  compat/qsort.c        |    2 +-\n>>  compat/win32/syslog.c |    2 +-\n>\n> There is a danger that the high-level die() routine (which is used by the\n> x-wrappers) uses one of the low-level compat/ routines. IOW, in the case\n> of errors, recursion might occur. Therefore, I would prefer that the\n> compat/ routines do their own error reporting (preferably via return\n> values and errno).\n\nThanks.  Will do.\n\n-Brandon\n"},{"id":"175595","messageId":"20110915181258.GA1227@sigill.intra.peff.net","threadId":"28392","inReplyTo":"1316051979-19671-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 0/4] Honor core.ignorecase for attribute patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-15T18:12:58Z","receivedAt":"2011-09-15T18:12:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 14, 2011 at 08:59:35PM -0500, Brandon Casey wrote:\n\n> > I haven't even tested that it runs. :)  No, I was hoping someone\n> > who was more interested would finish it, and maybe even test on\n> > an affected system.\n> \n> Ok, I lied.  Here's a series that needs testing by people on a\n> case-insensitive filesystem and some comments.\n\nThanks. I was trying to decide if I was interested enough to work on it,\nbut procrastination wins again.\n\nI'm not sure I understand why you need a case-insensitive file system\nfor the final set of tests. If we have a case-sensitive system, we can\nforce the filesystem to show us whatever cases we want, and check\nagainst them with both core.ignorecase off and on[1]. What are these\ntests checking that requires the actual behavior of a case-insensitive\nfilesystem?\n\nI'm sure there is something subtle that I'm missing. Can you explain it\neither here or in the commit message?\n\n-Peff\n\n[1] Actually, I wondered at first if the other tests needed to be marked\nfor only case-sensitive systems, since we can't rely on the behavior of\ninsensitive ones (e.g., are they case-preserving, always downcasing,\netc). But looking at t0003, we don't seem to actually create the files\nin the filesystem at all.\n"},{"id":"175603","messageId":"CA+sFfMeLgdmUHq6YTymkC2bpMEGScQuwJ1SgyR0GGFjo-Gt=4g@mail.gmail.com","threadId":"28392","inReplyTo":"20110915181258.GA1227@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Honor core.ignorecase for attribute patterns","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-09-15T20:28:33Z","receivedAt":"2011-09-15T20:28:33Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Sep 15, 2011 at 1:12 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Sep 14, 2011 at 08:59:35PM -0500, Brandon Casey wrote:\n>\n>> > I haven't even tested that it runs. :)  No, I was hoping someone\n>> > who was more interested would finish it, and maybe even test on\n>> > an affected system.\n>>\n>> Ok, I lied.  Here's a series that needs testing by people on a\n>> case-insensitive filesystem and some comments.\n>\n> Thanks. I was trying to decide if I was interested enough to work on it,\n> but procrastination wins again.\n>\n> I'm not sure I understand why you need a case-insensitive file system\n> for the final set of tests. If we have a case-sensitive system, we can\n> force the filesystem to show us whatever cases we want, and check\n> against them with both core.ignorecase off and on[1]. What are these\n> tests checking that requires the actual behavior of a case-insensitive\n> filesystem?\n\nThis is probably way more detail than this feature deserves, but...\n\nThose tests are making sure that git handles the case where the\n.gitignore file resides in a subdirectory and the user supplies a path\nthat does not match the case in the filesystem.  In that\ncase^H^H^H^Hsituation, part of the path supplied by the user is\neffectively interpreted case-insensitively, and part of it is\ndependent on the setting of core.ignorecase.  git should only be\nmatching the portion of the path below the directory holding the\n.gitignore file according to the setting of core.ignorecase.\n\nImagine a hierarchy that looks like this:\n\n   .gitattributes\n   a/.gitattributes\n\nOn a case-insensitive filesystem, if you supply the path A/B,\nregardless of whether ignorecase is true or false, git will read the\na/.gitattributes file and use it.\n\nThen if you have:\n\n   $ cat a/.gitattributes\n   b/c test=a/b/c\n\nthen you should get the following results:\n\n   # the case of a/ does not affect the attr check\n   $ git -c core.ignorecase=0 check-attr a/b/c\n   a/b/c: test: a/b/c\n   $ git -c core.ignorecase=0 check-attr A/b/c\n   A/b/c: test: a/b/c\n   $ git -c core.ignorecase=0 check-attr a/B/c\n   a/B/c: test: unspecified\n   $ git -c core.ignorecase=1 check-attr a/B/c\n   a/B/c: test: a/b/c\n   $ git -c core.ignorecase=0 check-attr A/B/c\n   A/B/c: test: unspecified\n   $ git -c core.ignorecase=1 check-attr A/B/c\n   A/B/c: test: a/b/c\n   etc.\n\nOn a case-sensitive filesystem, a/.gitattributes would never be read\nif A/b/c was supplied, regardless of core.ignorecase.\n\nThis is also partly future-proofing.  Currently, git builds the attr\nstack based on the path supplied by the user, so we don't have to do\nanything special (like use strcmp_icase) to handle the parts of that\npath that don't match the filesystem with respect to case.  If git\ninstead built the attr stack by scanning the repository, then the\npaths in the origin field would not necessarily match the paths\nsupplied by the user.  If someone makes a change like that in the\nfuture, these tests will notice.\n\n> I'm sure there is something subtle that I'm missing. Can you explain it\n> either here or in the commit message?\n\nYeah, that commit message was really just a place-holder.  I meant to\nadd WIP in the subject field of the last patch too.  I'll try to\nexplain some of the above when I reroll.\n\n-Brandon\n"},{"id":"176993","messageId":"CA+sFfMdHpvdMU==a2sUR9sZZCcgqPfGF7+dy6yi8RVoMZ+uZVA@mail.gmail.com","threadId":"28392","inReplyTo":"CA+sFfMf73K3yv_5K633DKOsVufMV6rTjd+SSunq4sBikt4jCsg@mail.gmail.com","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-10-06T02:00:44Z","receivedAt":"2011-10-06T02:00:44Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"[resend without html bits added by \"gmail offline\"]\n\nSo, it seems that of all of Google's email clients, only full desktop\ngmail allows you to send plain text email (if you're careful to make\nsure \"Rich formatting\" has not been clicked).  The new offline gmail\nsends html, gmail android app sends html, gmail mobile web sends html.\n Google's war on plain text continues...\n\nOr have I overlooked the switch that makes gmail send plain text and\nonly plain text?\n\nOn Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:\n> On Thursday, September 15, 2011, Brandon Casey wrote:\n>>\n>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>\n>> wrote:\n>> > Am 9/15/2011 3:59, schrieb Brandon Casey:\n>> >> The \"x\"-prefixed versions of strdup, malloc, etc. will check whether\n>> >> the\n>> >> allocation was successful and terminate the process otherwise.\n>> >>\n>> >> A few uses of malloc were left alone since they already implemented a\n>> >> graceful path of failure or were in a quasi external library like\n>> >> xdiff.\n>> >>\n>> >> Signed-off-by: Brandon Casey <drafnel@gmail.com>\n>> >> ---\n>> >>  ...\n>> >>  compat/mingw.c        |    2 +-\n>> >>  compat/qsort.c        |    2 +-\n>> >>  compat/win32/syslog.c |    2 +-\n>> >\n>> > There is a danger that the high-level die() routine (which is used by\n>> > the\n>> > x-wrappers) uses one of the low-level compat/ routines. IOW, in the case\n>> > of errors, recursion might occur. Therefore, I would prefer that the\n>> > compat/ routines do their own error reporting (preferably via return\n>> > values and errno).\n>>\n>> Thanks.  Will do.\n>\n> Hi Johannes,\n> I have taken a closer look at the possibility of recursion with respect to\n> die() and the functions in compat/.  It appears the risk is only related to\n> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et\n> al from within snprintf.c, I think we should be safe from recursion.\n> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they\n> both already use the x-wrappers or strbuf, even though they could easily be\n> worked around.  The other file that was touched is compat/qsort, which\n> returns void and doesn't have a good alternative error handling path.  So,\n> I'm inclined to keep that one too.\n> Sound reasonable?\n> -Brandon\n>\n"},{"id":"177004","messageId":"4E8D4812.9090102@viscovery.net","threadId":"28392","inReplyTo":"CA+sFfMdHpvdMU==a2sUR9sZZCcgqPfGF7+dy6yi8RVoMZ+uZVA@mail.gmail.com","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-10-06T06:17:54Z","receivedAt":"2011-10-06T06:17:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/6/2011 4:00, schrieb Brandon Casey:\n> [resend without html bits added by \"gmail offline\"]\n> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:\n>> On Thursday, September 15, 2011, Brandon Casey wrote:\n>>>\n>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>\n>>>> There is a danger that the high-level die() routine (which is used by\n>>>> the\n>>>> x-wrappers) uses one of the low-level compat/ routines. IOW, in the case\n>>>> of errors, recursion might occur. Therefore, I would prefer that the\n>>>> compat/ routines do their own error reporting (preferably via return\n>>>> values and errno).\n>>>\n>>> Thanks.  Will do.\n>>\n>> Hi Johannes,\n>> I have taken a closer look at the possibility of recursion with respect to\n>> die() and the functions in compat/.  It appears the risk is only related to\n>> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et\n>> al from within snprintf.c, I think we should be safe from recursion.\n>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they\n>> both already use the x-wrappers or strbuf, even though they could easily be\n>> worked around.  The other file that was touched is compat/qsort, which\n>> returns void and doesn't have a good alternative error handling path.  So,\n>> I'm inclined to keep that one too.\n\nI'm fine with keeping the change to mingw.c (getaddrinfo related) and\nqsort: both are unlikely to be called from die().\n\nBut syslog() *is* called from die() in git-daemon, and it would be better\nto back out the other offenders instead of adding to them.\n\n-- Hannes\n"},{"id":"177006","messageId":"20111006110113.45f47e58@ashu.dyn.rarus.ru","threadId":"28392","inReplyTo":"4E8D4812.9090102@viscovery.net","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Alexey Shumkin","fromEmail":"zapped@mail.ru","sentAt":"2011-10-06T07:01:13Z","receivedAt":"2011-10-06T07:01:13Z","isPatch":true,"sender":{"key":"alex.crezoff@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1183752?v=4"},"body":"Offtopic:\nExcuse me, guys\nMay I ask you to exclude me from CC-list for this topic,\nit seems to me I got in it for a mistake somehow,\nI have no relation to this topic\n\nthanks in advance :)\n\n> Am 10/6/2011 4:00, schrieb Brandon Casey:\n> > [resend without html bits added by \"gmail offline\"]\n> > On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com>\n> > wrote:\n> >> On Thursday, September 15, 2011, Brandon Casey wrote:\n> >>>\n> >>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt\n> >>> <j.sixt@viscovery.net>\n> >>>> There is a danger that the high-level die() routine (which is\n> >>>> used by the\n> >>>> x-wrappers) uses one of the low-level compat/ routines. IOW, in\n> >>>> the case of errors, recursion might occur. Therefore, I would\n> >>>> prefer that the compat/ routines do their own error reporting\n> >>>> (preferably via return values and errno).\n> >>>\n> >>> Thanks.  Will do.\n> >>\n> >> Hi Johannes,\n> >> I have taken a closer look at the possibility of recursion with\n> >> respect to die() and the functions in compat/.  It appears the\n> >> risk is only related to vsnprintf/snprintf at the moment.  So as\n> >> long as we avoid calling xmalloc et al from within snprintf.c, I\n> >> think we should be safe from recursion. I'm inclined to keep the\n> >> additions to mingw.c and win32/syslog.c since they both already\n> >> use the x-wrappers or strbuf, even though they could easily be\n> >> worked around.  The other file that was touched is compat/qsort,\n> >> which returns void and doesn't have a good alternative error\n> >> handling path.  So, I'm inclined to keep that one too.\n> \n> I'm fine with keeping the change to mingw.c (getaddrinfo related) and\n> qsort: both are unlikely to be called from die().\n> \n> But syslog() *is* called from die() in git-daemon, and it would be\n> better to back out the other offenders instead of adding to them.\n> \n> -- Hannes\n> \n"},{"id":"177042","messageId":"CA+sFfMf8_7ccC9kjEq=2NrehVgS=ahnQA8VibEF10VaULot7=A@mail.gmail.com","threadId":"28392","inReplyTo":"4E8D4812.9090102@viscovery.net","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-10-06T16:14:23Z","receivedAt":"2011-10-06T16:14:23Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"[removed Alexey Shumkin from cc]\n\nOn Thu, Oct 6, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Am 10/6/2011 4:00, schrieb Brandon Casey:\n>> [resend without html bits added by \"gmail offline\"]\n>> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:\n>>> On Thursday, September 15, 2011, Brandon Casey wrote:\n>>>>\n>>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>\n>>>>> There is a danger that the high-level die() routine (which is used by\n>>>>> the\n>>>>> x-wrappers) uses one of the low-level compat/ routines. IOW, in the case\n>>>>> of errors, recursion might occur. Therefore, I would prefer that the\n>>>>> compat/ routines do their own error reporting (preferably via return\n>>>>> values and errno).\n>>>>\n>>>> Thanks.  Will do.\n>>>\n>>> Hi Johannes,\n>>> I have taken a closer look at the possibility of recursion with respect to\n>>> die() and the functions in compat/.  It appears the risk is only related to\n>>> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et\n>>> al from within snprintf.c, I think we should be safe from recursion.\n>>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they\n>>> both already use the x-wrappers or strbuf, even though they could easily be\n>>> worked around.  The other file that was touched is compat/qsort, which\n>>> returns void and doesn't have a good alternative error handling path.  So,\n>>> I'm inclined to keep that one too.\n>\n> I'm fine with keeping the change to mingw.c (getaddrinfo related) and\n> qsort: both are unlikely to be called from die().\n>\n> But syslog() *is* called from die() in git-daemon, and it would be better\n> to back out the other offenders instead of adding to them.\n\nAh.  Yes, you're right.  x-wrappers should not be used in syslog.c and\nthe use of strbuf's should be replaced.\n\nThanks,\n-Brandon\n"},{"id":"177052","messageId":"CABPQNSak_jDbNQhzMoSN=NdWmyby3xJRwED54Ck5H1Y12HoGCQ@mail.gmail.com","threadId":"28392","inReplyTo":"CA+sFfMf8_7ccC9kjEq=2NrehVgS=ahnQA8VibEF10VaULot7=A@mail.gmail.com","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-10-06T16:50:59Z","receivedAt":"2011-10-06T16:50:59Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:\n> [removed Alexey Shumkin from cc]\n>\n> On Thu, Oct 6, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>> Am 10/6/2011 4:00, schrieb Brandon Casey:\n>>> [resend without html bits added by \"gmail offline\"]\n>>> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:\n>>>> On Thursday, September 15, 2011, Brandon Casey wrote:\n>>>>>\n>>>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>\n>>>>>> There is a danger that the high-level die() routine (which is used by\n>>>>>> the\n>>>>>> x-wrappers) uses one of the low-level compat/ routines. IOW, in the case\n>>>>>> of errors, recursion might occur. Therefore, I would prefer that the\n>>>>>> compat/ routines do their own error reporting (preferably via return\n>>>>>> values and errno).\n>>>>>\n>>>>> Thanks.  Will do.\n>>>>\n>>>> Hi Johannes,\n>>>> I have taken a closer look at the possibility of recursion with respect to\n>>>> die() and the functions in compat/.  It appears the risk is only related to\n>>>> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et\n>>>> al from within snprintf.c, I think we should be safe from recursion.\n>>>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they\n>>>> both already use the x-wrappers or strbuf, even though they could easily be\n>>>> worked around.  The other file that was touched is compat/qsort, which\n>>>> returns void and doesn't have a good alternative error handling path.  So,\n>>>> I'm inclined to keep that one too.\n>>\n>> I'm fine with keeping the change to mingw.c (getaddrinfo related) and\n>> qsort: both are unlikely to be called from die().\n>>\n>> But syslog() *is* called from die() in git-daemon, and it would be better\n>> to back out the other offenders instead of adding to them.\n>\n> Ah.  Yes, you're right.  x-wrappers should not be used in syslog.c and\n> the use of strbuf's should be replaced.\n\nGood point. The patch for this looks something like this:\n\ndiff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\nindex 42b95a9..243538c 100644\n--- a/compat/win32/syslog.c\n+++ b/compat/win32/syslog.c\n@@ -1,5 +1,4 @@\n #include \"../../git-compat-util.h\"\n-#include \"../../strbuf.h\"\n\n static HANDLE ms_eventlog;\n\n@@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)\n\n void syslog(int priority, const char *fmt, ...)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct strbuf_expand_dict_entry dict[] = {\n-\t\t{\"1\", \"% 1\"},\n-\t\t{NULL, NULL}\n-\t};\n \tWORD logtype;\n-\tchar *str;\n+\tchar *str, *pos;\n \tint str_len;\n \tva_list ap;\n\n@@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)\n \t}\n\n \tstr = malloc(str_len + 1);\n+\tif (!str)\n+\t\treturn; /* no chance to report error */\n+\n \tva_start(ap, fmt);\n \tvsnprintf(str, str_len + 1, fmt, ap);\n \tva_end(ap);\n-\tstrbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);\n-\tfree(str);\n+\n+\twhile ((pos = strstr(str, \"%1\")) != NULL) {\n+\t\tstr = realloc(str, ++str_len + 1);\n+\t\tif (!str)\n+\t\t\treturn;\n+\t\tmemmove(pos + 2, pos + 1, strlen(pos));\n+\t\tpos[1] = ' ';\n+\t}\n\n \tswitch (priority) {\n \tcase LOG_EMERG:\n@@ -66,7 +69,5 @@ void syslog(int priority, const char *fmt, ...)\n \t}\n\n \tReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,\n-\t    (const char **)&sb.buf, NULL);\n-\n-\tstrbuf_release(&sb);\n+\t    (const char **)&str, NULL);\n }\n"},{"id":"177053","messageId":"CABPQNSaQFFj0vP1+wuoL2=GyHZPRgCXLxXqD+5WX3Le_PyuH4w@mail.gmail.com","threadId":"28392","inReplyTo":"CABPQNSak_jDbNQhzMoSN=NdWmyby3xJRwED54Ck5H1Y12HoGCQ@mail.gmail.com","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-10-06T16:52:43Z","receivedAt":"2011-10-06T16:52:43Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Oct 6, 2011 at 6:50 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:\n>> Ah.  Yes, you're right.  x-wrappers should not be used in syslog.c and\n>> the use of strbuf's should be replaced.\n>\n> Good point. The patch for this looks something like this:\n>\n> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\n> index 42b95a9..243538c 100644\n> --- a/compat/win32/syslog.c\n> +++ b/compat/win32/syslog.c\n> @@ -1,5 +1,4 @@\n>  #include \"../../git-compat-util.h\"\n> -#include \"../../strbuf.h\"\n>\n>  static HANDLE ms_eventlog;\n>\n> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)\n>\n>  void syslog(int priority, const char *fmt, ...)\n>  {\n> -       struct strbuf sb = STRBUF_INIT;\n> -       struct strbuf_expand_dict_entry dict[] = {\n> -               {\"1\", \"% 1\"},\n> -               {NULL, NULL}\n> -       };\n>        WORD logtype;\n> -       char *str;\n> +       char *str, *pos;\n>        int str_len;\n>        va_list ap;\n>\n> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)\n>        }\n>\n>        str = malloc(str_len + 1);\n> +       if (!str)\n> +               return; /* no chance to report error */\n> +\n>        va_start(ap, fmt);\n>        vsnprintf(str, str_len + 1, fmt, ap);\n>        va_end(ap);\n> -       strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);\n> -       free(str);\n> +\n> +       while ((pos = strstr(str, \"%1\")) != NULL) {\n> +               str = realloc(str, ++str_len + 1);\n> +               if (!str)\n> +                       return;\n> +               memmove(pos + 2, pos + 1, strlen(pos));\n> +               pos[1] = ' ';\n> +       }\n>\n>        switch (priority) {\n>        case LOG_EMERG:\n> @@ -66,7 +69,5 @@ void syslog(int priority, const char *fmt, ...)\n>        }\n>\n>        ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,\n> -           (const char **)&sb.buf, NULL);\n> -\n> -       strbuf_release(&sb);\n> +           (const char **)&str, NULL);\n>  }\n>\n\n...aaand this on top to avoid a leak, of course:\n\ndiff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\nindex 243538c..3b8e2b7 100644\n--- a/compat/win32/syslog.c\n+++ b/compat/win32/syslog.c\n@@ -70,4 +70,5 @@ void syslog(int priority, const char *fmt, ...)\n\n \tReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,\n \t    (const char **)&str, NULL);\n+\tfree(str);\n }\n"},{"id":"177061","messageId":"CA+sFfMdxxZJd6t+KZMz67mSEw=FizYukod2Rd+9Fu17NLwrAhg@mail.gmail.com","threadId":"28392","inReplyTo":"CABPQNSak_jDbNQhzMoSN=NdWmyby3xJRwED54Ck5H1Y12HoGCQ@mail.gmail.com","subject":"Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-10-06T17:17:24Z","receivedAt":"2011-10-06T17:17:24Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Oct 6, 2011 at 11:50 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> On Thu, Oct 6, 2011 at 6:14 PM, Brandon Casey <drafnel@gmail.com> wrote:\n>> [removed Alexey Shumkin from cc]\n>>\n>> On Thu, Oct 6, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>>> Am 10/6/2011 4:00, schrieb Brandon Casey:\n>>>> [resend without html bits added by \"gmail offline\"]\n>>>> On Wed, Oct 5, 2011 at 7:53 PM, Brandon Casey <drafnel@gmail.com> wrote:\n>>>>> On Thursday, September 15, 2011, Brandon Casey wrote:\n>>>>>>\n>>>>>> On Thu, Sep 15, 2011 at 1:52 AM, Johannes Sixt <j.sixt@viscovery.net>\n>>>>>>> There is a danger that the high-level die() routine (which is used by\n>>>>>>> the\n>>>>>>> x-wrappers) uses one of the low-level compat/ routines. IOW, in the case\n>>>>>>> of errors, recursion might occur. Therefore, I would prefer that the\n>>>>>>> compat/ routines do their own error reporting (preferably via return\n>>>>>>> values and errno).\n>>>>>>\n>>>>>> Thanks.  Will do.\n>>>>>\n>>>>> Hi Johannes,\n>>>>> I have taken a closer look at the possibility of recursion with respect to\n>>>>> die() and the functions in compat/.  It appears the risk is only related to\n>>>>> vsnprintf/snprintf at the moment.  So as long as we avoid calling xmalloc et\n>>>>> al from within snprintf.c, I think we should be safe from recursion.\n>>>>> I'm inclined to keep the additions to mingw.c and win32/syslog.c since they\n>>>>> both already use the x-wrappers or strbuf, even though they could easily be\n>>>>> worked around.  The other file that was touched is compat/qsort, which\n>>>>> returns void and doesn't have a good alternative error handling path.  So,\n>>>>> I'm inclined to keep that one too.\n>>>\n>>> I'm fine with keeping the change to mingw.c (getaddrinfo related) and\n>>> qsort: both are unlikely to be called from die().\n>>>\n>>> But syslog() *is* called from die() in git-daemon, and it would be better\n>>> to back out the other offenders instead of adding to them.\n>>\n>> Ah.  Yes, you're right.  x-wrappers should not be used in syslog.c and\n>> the use of strbuf's should be replaced.\n>\n> Good point. The patch for this looks something like this:\n>\n> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\n> index 42b95a9..243538c 100644\n> --- a/compat/win32/syslog.c\n> +++ b/compat/win32/syslog.c\n> @@ -1,5 +1,4 @@\n>  #include \"../../git-compat-util.h\"\n> -#include \"../../strbuf.h\"\n>\n>  static HANDLE ms_eventlog;\n>\n> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)\n>\n>  void syslog(int priority, const char *fmt, ...)\n>  {\n> -       struct strbuf sb = STRBUF_INIT;\n> -       struct strbuf_expand_dict_entry dict[] = {\n> -               {\"1\", \"% 1\"},\n> -               {NULL, NULL}\n> -       };\n>        WORD logtype;\n> -       char *str;\n> +       char *str, *pos;\n>        int str_len;\n>        va_list ap;\n>\n> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)\n>        }\n>\n>        str = malloc(str_len + 1);\n> +       if (!str)\n> +               return; /* no chance to report error */\n> +\n>        va_start(ap, fmt);\n>        vsnprintf(str, str_len + 1, fmt, ap);\n>        va_end(ap);\n> -       strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);\n> -       free(str);\n> +\n> +       while ((pos = strstr(str, \"%1\")) != NULL) {\n> +               str = realloc(str, ++str_len + 1);\n> +               if (!str)\n> +                       return;\n> +               memmove(pos + 2, pos + 1, strlen(pos));\n> +               pos[1] = ' ';\n> +       }\n\nIs there any reason not to just use a different character than '%'\nwhen replacing '%n'?  Like '@'?  Then the replacement could be done\nin-place.\n\ne.g. convert this:\n\n   fe80::3%1\n\ninto this:\n\n   fe80::3@1\n\nI'm not usually on a windows platform, so maybe adding the space is\nthe common thing to do when trying to write an ipv6 address to the\nevent log using ReportEvent().  If not, then I think people would\nprobably figure out easily enough that '@n' referred to interface 'n'.\n\n-Brandon\n"}]}