* [PATCH 1/4] attr.c: avoid inappropriate access to strbuf "buf" member
2011-09-15 1:59 ` [PATCH 0/4] Honor core.ignorecase for attribute patterns Brandon Casey
@ 2011-09-15 1:59 ` Brandon Casey
2011-09-15 1:59 ` [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere Brandon Casey
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Brandon Casey @ 2011-09-15 1:59 UTC (permalink / raw)
To: peff; +Cc: git, gitster, sunshine, bharrosh, trast, zapped, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere
2011-09-15 1:59 ` [PATCH 0/4] Honor core.ignorecase for attribute patterns Brandon Casey
2011-09-15 1:59 ` [PATCH 1/4] attr.c: avoid inappropriate access to strbuf "buf" member Brandon Casey
@ 2011-09-15 1:59 ` Brandon Casey
2011-09-15 6:52 ` Johannes Sixt
2011-09-15 1:59 ` [PATCH 3/4] builtin/mv.c: plug miniscule memory leak Brandon Casey
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Brandon Casey @ 2011-09-15 1:59 UTC (permalink / raw)
To: peff; +Cc: git, gitster, sunshine, bharrosh, trast, zapped, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere
2011-09-15 1:59 ` [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere Brandon Casey
@ 2011-09-15 6:52 ` Johannes Sixt
2011-09-15 15:39 ` Brandon Casey
0 siblings, 1 reply; 19+ messages in thread
From: Johannes Sixt @ 2011-09-15 6:52 UTC (permalink / raw)
To: Brandon Casey; +Cc: peff, git, gitster, sunshine, bharrosh, trast, zapped
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere
2011-09-15 6:52 ` Johannes Sixt
@ 2011-09-15 15:39 ` Brandon Casey
[not found] ` <CA+sFfMf73K3yv_5K633DKOsVufMV6rTjd+SSunq4sBikt4jCsg@mail.gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Brandon Casey @ 2011-09-15 15:39 UTC (permalink / raw)
To: Johannes Sixt; +Cc: peff, git, gitster, sunshine, bharrosh, trast, zapped
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] builtin/mv.c: plug miniscule memory leak
2011-09-15 1:59 ` [PATCH 0/4] Honor core.ignorecase for attribute patterns Brandon Casey
2011-09-15 1:59 ` [PATCH 1/4] attr.c: avoid inappropriate access to strbuf "buf" member Brandon Casey
2011-09-15 1:59 ` [PATCH 2/4] cleanup: use internal memory allocation wrapper functions everywhere Brandon Casey
@ 2011-09-15 1:59 ` Brandon Casey
2011-09-15 1:59 ` [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns Brandon Casey
2011-09-15 18:12 ` [PATCH 0/4] Honor core.ignorecase for " Jeff King
4 siblings, 0 replies; 19+ messages in thread
From: Brandon Casey @ 2011-09-15 1:59 UTC (permalink / raw)
To: peff; +Cc: git, gitster, sunshine, bharrosh, trast, zapped, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns
2011-09-15 1:59 ` [PATCH 0/4] Honor core.ignorecase for attribute patterns Brandon Casey
` (2 preceding siblings ...)
2011-09-15 1:59 ` [PATCH 3/4] builtin/mv.c: plug miniscule memory leak Brandon Casey
@ 2011-09-15 1:59 ` Brandon Casey
2011-09-15 4:01 ` Junio C Hamano
2011-09-15 18:12 ` [PATCH 0/4] Honor core.ignorecase for " Jeff King
4 siblings, 1 reply; 19+ messages in thread
From: Brandon Casey @ 2011-09-15 1:59 UTC (permalink / raw)
To: peff; +Cc: git, gitster, sunshine, bharrosh, trast, zapped, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns
2011-09-15 1:59 ` [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns Brandon Casey
@ 2011-09-15 4:01 ` Junio C Hamano
2011-09-15 4:06 ` Junio C Hamano
0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2011-09-15 4:01 UTC (permalink / raw)
To: Brandon Casey; +Cc: peff, git, sunshine, bharrosh, trast, zapped
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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns
2011-09-15 4:01 ` Junio C Hamano
@ 2011-09-15 4:06 ` Junio C Hamano
2011-09-15 15:38 ` Brandon Casey
0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2011-09-15 4:06 UTC (permalink / raw)
To: Brandon Casey; +Cc: peff, git, sunshine, bharrosh, trast, zapped
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;
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns
2011-09-15 4:06 ` Junio C Hamano
@ 2011-09-15 15:38 ` Brandon Casey
0 siblings, 0 replies; 19+ messages in thread
From: Brandon Casey @ 2011-09-15 15:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: peff, git, sunshine, bharrosh, trast, zapped
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(-)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] Honor core.ignorecase for attribute patterns
2011-09-15 1:59 ` [PATCH 0/4] Honor core.ignorecase for attribute patterns Brandon Casey
` (3 preceding siblings ...)
2011-09-15 1:59 ` [PATCH 4/4] attr.c: respect core.ignorecase when matching attribute patterns Brandon Casey
@ 2011-09-15 18:12 ` Jeff King
2011-09-15 20:28 ` Brandon Casey
4 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2011-09-15 18:12 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, gitster, sunshine, bharrosh, trast, zapped
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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/4] Honor core.ignorecase for attribute patterns
2011-09-15 18:12 ` [PATCH 0/4] Honor core.ignorecase for " Jeff King
@ 2011-09-15 20:28 ` Brandon Casey
0 siblings, 0 replies; 19+ messages in thread
From: Brandon Casey @ 2011-09-15 20:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, sunshine, bharrosh, trast, zapped
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
^ permalink raw reply [flat|nested] 19+ messages in thread