Re: t9902 fails (Was: [PATCH] attr: fix off-by-one directory component length calculation)
Jean-Noël AVILA wrote: OK. I have installed practically everything related to git from the package manager and there is a git-checkout-branches utility available. That result defeats the purpose of the test. This needs a tighter environment to work whatever the configuration of the user may be. Presumably 'git checkout-branches' is from git-stuff. Here's a patch to make the tested command a little less likely to conflict with commands from the user's $PATH. I'm not thrilled with it because the contents of $PATH are still not tightly controlled, and this does nothing to avoid problems due to existence of, for example, a git cherry-pick-branches command. Thoughts? Maybe it would be enough to check that the intended get commands are present in the completion list and other git commands are not, ignoring binaries that might live elsewhere on the $PATH? Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- t/t9902-completion.sh | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3cd53f8..06dcfb2 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -192,19 +192,19 @@ test_expect_success 'general options' ' ' test_expect_success 'general options plus command' ' - test_completion git --version check checkout - test_completion git --paginate check checkout - test_completion git --git-dir=foo check checkout - test_completion git --bare check checkout - test_completion git --help des describe - test_completion git --exec-path=foo check checkout - test_completion git --html-path check checkout - test_completion git --no-pager check checkout - test_completion git --work-tree=foo check checkout - test_completion git --namespace=foo check checkout - test_completion git --paginate check checkout - test_completion git --info-path check checkout - test_completion git --no-replace-objects check checkout + test_completion git --version cherry-p cherry-pick + test_completion git --paginate cherry-p cherry-pick + test_completion git --git-dir=foo cherry-p cherry-pick + test_completion git --bare cherry-p cherry-pick + test_completion git --help cherry-p cherry-pick + test_completion git --exec-path=foo cherry-p cherry-pick + test_completion git --html-path cherry-p cherry-pick + test_completion git --no-pager cherry-p cherry-pick + test_completion git --work-tree=foo cherry-p cherry-pick + test_completion git --namespace=foo cherry-p cherry-pick + test_completion git --paginate cherry-p cherry-pick + test_completion git --info-path cherry-p cherry-pick + test_completion git --no-replace-objects cherry-p cherry-pick ' test_expect_success 'setup for ref completion' ' -- 1.8.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: t9902 fails (Was: [PATCH] attr: fix off-by-one directory component length calculation)
On 18.01.13 01:04, Jonathan Nieder wrote: Jean-Noël AVILA wrote: OK. I have installed practically everything related to git from the package manager and there is a git-checkout-branches utility available. That result defeats the purpose of the test. This needs a tighter environment to work whatever the configuration of the user may be. Presumably 'git checkout-branches' is from git-stuff. Here's a patch to make the tested command a little less likely to conflict with commands from the user's $PATH. I'm not thrilled with it because the contents of $PATH are still not tightly controlled, and this does nothing to avoid problems due to existence of, for example, a git cherry-pick-branches command. Thoughts? Maybe it would be enough to check that the intended get commands are present in the completion list and other git commands are not, ignoring binaries that might live elsewhere on the $PATH? Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- t/t9902-completion.sh | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3cd53f8..06dcfb2 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -192,19 +192,19 @@ test_expect_success 'general options' ' ' test_expect_success 'general options plus command' ' - test_completion git --version check checkout - test_completion git --paginate check checkout - test_completion git --git-dir=foo check checkout - test_completion git --bare check checkout - test_completion git --help des describe - test_completion git --exec-path=foo check checkout - test_completion git --html-path check checkout - test_completion git --no-pager check checkout - test_completion git --work-tree=foo check checkout - test_completion git --namespace=foo check checkout - test_completion git --paginate check checkout - test_completion git --info-path check checkout - test_completion git --no-replace-objects check checkout + test_completion git --version cherry-p cherry-pick + test_completion git --paginate cherry-p cherry-pick + test_completion git --git-dir=foo cherry-p cherry-pick + test_completion git --bare cherry-p cherry-pick + test_completion git --help cherry-p cherry-pick + test_completion git --exec-path=foo cherry-p cherry-pick + test_completion git --html-path cherry-p cherry-pick + test_completion git --no-pager cherry-p cherry-pick + test_completion git --work-tree=foo cherry-p cherry-pick + test_completion git --namespace=foo cherry-p cherry-pick + test_completion git --paginate cherry-p cherry-pick + test_completion git --info-path cherry-p cherry-pick + test_completion git --no-replace-objects cherry-p cherry-pick ' test_expect_success 'setup for ref completion' ' That looks good to me, thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
Duy Nguyen pclo...@gmail.com writes:
OK I get your point now. Something like this?
-- 8 --
Subject: [PATCH] attr: avoid calling find_basename() twice per path
find_basename() is only used inside collect_all_attrs(), called once
in prepare_attr_stack, then again after prepare_attr_stack()
returns. Both calls return exact same value. Reorder the code to do
the same task once. Also avoid strlen() because we knows the length
after finding basename.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Yeah, I think this is a nice code reduction, readability improvement
and micro optimization rolled into one.
attr.c | 45 ++---
1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/attr.c b/attr.c
index cfc6748..880f862 100644
--- a/attr.c
+++ b/attr.c
@@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
attr_stack = elem;
}
-static const char *find_basename(const char *path)
-{
- const char *cp, *last_slash = NULL;
-
- for (cp = path; *cp; cp++) {
- if (*cp == '/' cp[1])
- last_slash = cp;
- }
- return last_slash ? last_slash + 1 : path;
-}
-
-static void prepare_attr_stack(const char *path)
+static void prepare_attr_stack(const char *path, int dirlen)
{
struct attr_stack *elem, *info;
- int dirlen, len;
+ int len;
const char *cp;
- dirlen = find_basename(path) - path;
-
- /*
- * find_basename() includes the trailing slash, but we do
- * _not_ want it.
- */
- if (dirlen)
- dirlen--;
-
/*
* At the bottom of the attribute stack is the built-in
* set of attribute definitions, followed by the contents
@@ -769,15 +749,26 @@ static int macroexpand_one(int attr_nr, int rem)
static void collect_all_attrs(const char *path)
{
struct attr_stack *stk;
- int i, pathlen, rem;
- const char *basename;
+ int i, pathlen, rem, dirlen;
+ const char *basename, *cp, *last_slash = NULL;
+
+ for (cp = path; *cp; cp++) {
+ if (*cp == '/' cp[1])
+ last_slash = cp;
+ }
+ pathlen = cp - path;
+ if (last_slash) {
+ basename = last_slash + 1;
+ dirlen = last_slash - path;
+ } else {
+ basename = path;
+ dirlen = 0;
+ }
- prepare_attr_stack(path);
+ prepare_attr_stack(path, dirlen);
for (i = 0; i attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- basename = find_basename(path);
- pathlen = strlen(path);
rem = attr_nr;
for (stk = attr_stack; 0 rem stk; stk = stk-prev)
rem = fill(path, pathlen, basename, stk, rem);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] attr: fix off-by-one directory component length calculation
94bc671 (Add directory pattern matching to attributes - 2012-12-08)
uses find_basename() to calculate the length of directory part in
prepare_attr_stack. This function expects the directory without the
trailing slash (as origin field in match_attr struct is without the
trailing slash). find_basename() includes the trailing slash and
confuses push/pop algorithm.
Consider path = abc/def and the push down code:
while (1) {
len = strlen(attr_stack-origin);
if (dirlen = len)
break;
cp = memchr(path + len + 1, '/', dirlen - len - 1);
if (!cp)
cp = path + dirlen;
dirlen is 4, not 3, without this patch. So when attr_stack-origin is
abc, it'll miss the exit condition because 4 = 3 is wrong. It'll
then try to push abc/ down the attr stack (because cp would be
NULL). So we have both abc and abc/ in the stack.
Next time when abc/ghi is checked, abc/ is popped out because of
the off-by-one dirlen, only to be pushed back in again by the above
code. This repeats for all files in the same directory. Which means
at least one failed open syscall per file, or more if .gitattributes
exists.
This is the perf result with 10 runs on git.git:
Test 94bc671^ 94bc671
HEAD
--
7810.1: grep worktree, cheap regex 0.02(0.01+0.04) 0.05(0.03+0.05)
+150.0% 0.02(0.01+0.04) +0.0%
7810.2: grep worktree, expensive regex 0.25(0.94+0.01) 0.26(0.94+0.02)
+4.0% 0.25(0.93+0.02) +0.0%
7810.3: grep --cached, cheap regex 0.11(0.10+0.00) 0.12(0.10+0.02)
+9.1% 0.10(0.10+0.00) -9.1%
7810.4: grep --cached, expensive regex 0.61(0.60+0.01) 0.62(0.61+0.01)
+1.6% 0.61(0.60+0.00) +0.0%
Reported-by: Ross Lagerwall rosslagerw...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
This may be an indication that our perf framework is never actively used :-(
attr.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/attr.c b/attr.c
index 466c93f..bb9a470 100644
--- a/attr.c
+++ b/attr.c
@@ -584,6 +584,13 @@ static void prepare_attr_stack(const char *path)
dirlen = find_basename(path) - path;
/*
+* find_basename() includes the trailing slash, but we do
+* _not_ want it.
+*/
+ if (dirlen)
+ dirlen--;
+
+ /*
* At the bottom of the attribute stack is the built-in
* set of attribute definitions, followed by the contents
* of $(prefix)/etc/gitattributes and a file specified by
--
1.8.0.rc2.23.g1fb49df
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
Good spotting and a nicely explained patch. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
Thank you for the explanation. I did not monitor the system calls when writing that patch. Where is the perf framework? As the mistake is located in the find_basename function, I would propose a fix directly into it so that the output fits what the other functions expect. Something in the line of: diff --git a/attr.c b/attr.c index d6d7190..b6e72f3 100644 --- a/attr.c +++ b/attr.c @@ -572,7 +572,7 @@ static const char *find_basename(const char *path) if (*cp == '/' cp[1]) last_slash = cp; } - return last_slash ? last_slash + 1 : path; + return last_slash ? last_slash : path; } static void prepare_attr_stack(const char *path) @@ -770,6 +770,10 @@ static void collect_all_attrs(const char *path) check_all_attr[i].value = ATTR__UNKNOWN; basename = find_basename(path); + /* the slash is included in the basename + so that it can be matched by a directory pattern */ + if (basename != path) + basename++; pathlen = strlen(path); rem = attr_nr; for (stk = attr_stack; 0 rem stk; stk = stk-prev) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
Jean-Noël AVILA avila...@gmail.com writes: Thank you for the explanation. I did not monitor the system calls when writing that patch. Where is the perf framework? As the mistake is located in the find_basename function, I would propose a fix directly into it so that the output fits what the other functions expect. Isn't that a crazy semantics for the function, though? I would expect find_basename(/a/path/to/file) to return file, not /file. Something in the line of: diff --git a/attr.c b/attr.c index d6d7190..b6e72f3 100644 --- a/attr.c +++ b/attr.c @@ -572,7 +572,7 @@ static const char *find_basename(const char *path) if (*cp == '/' cp[1]) last_slash = cp; } - return last_slash ? last_slash + 1 : path; + return last_slash ? last_slash : path; } static void prepare_attr_stack(const char *path) @@ -770,6 +770,10 @@ static void collect_all_attrs(const char *path) check_all_attr[i].value = ATTR__UNKNOWN; basename = find_basename(path); + /* the slash is included in the basename + so that it can be matched by a directory pattern */ + if (basename != path) + basename++; pathlen = strlen(path); rem = attr_nr; for (stk = attr_stack; 0 rem stk; stk = stk-prev) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
Le mardi 15 janvier 2013 20:29:16, Junio C Hamano a écrit : Jean-Noël AVILA avila...@gmail.com writes: Thank you for the explanation. I did not monitor the system calls when writing that patch. Where is the perf framework? As the mistake is located in the find_basename function, I would propose a fix directly into it so that the output fits what the other functions expect. Isn't that a crazy semantics for the function, though? I would expect find_basename(/a/path/to/file) to return file, not /file. Yes, it is. I was wrong on the meaning of basename. Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known issue? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
Jean-Noël AVILA avila...@gmail.com writes: Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known issue? Which branch? If you mean 'master' with the patch in this discussion applied, I didn't even have a chance to start today's integration cycle, so I don't know (it is not known to me). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
On Tue, Jan 15, 2013 at 12:49:05PM -0800, Junio C Hamano wrote: Jean-Noël AVILA avila...@gmail.com writes: Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known issue? Which branch? t9902.10 is overly sensitive to extra git commands in your PATH, as well as cruft in your build dir (especially if you have been building 'pu', which has git-check-ignore). Try make clean make test. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
On Wed, Jan 16, 2013 at 2:14 AM, Jean-Noël AVILA avila...@gmail.com wrote: I did not monitor the system calls when writing that patch. Where is the perf framework? It's in t/perf. I think you can do: ./run HEAD . to run and compare performance of HEAD and working directory (assume you haven't commit yet). Check out the README file. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
On Wed, Jan 16, 2013 at 2:29 AM, Junio C Hamano gits...@pobox.com wrote: Jean-Noël AVILA avila...@gmail.com writes: Thank you for the explanation. I did not monitor the system calls when writing that patch. Where is the perf framework? As the mistake is located in the find_basename function, I would propose a fix directly into it so that the output fits what the other functions expect. Isn't that a crazy semantics for the function, though? I would expect find_basename(/a/path/to/file) to return file, not Actually I'd like to remove that function. The function is called twice: - collect_all_attrs + prepare_attr_stack * find_basename + find_basename which could be reordered to - collect_all_attrs + find_basename + prepare_attr_stack (modified to take dirlen from caller) and because that'll be the only place find_basename is used, we could just inline the code there. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote:
Actually I'd like to remove that function.
This is what I had in mind:
-- 8 --
Subject: [PATCH] attr: avoid calling find_basename() twice per path
find_basename() is only used inside collect_all_attrs(), called once
in prepare_attr_stack, then again after prepare_attr_stack()
returns. Both calls return exact same value. Reorder the code to do it
once.
While at it, make use of pathlen to stop searching early if
possible.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
attr.c | 46 +++---
1 file changed, 19 insertions(+), 27 deletions(-)
diff --git a/attr.c b/attr.c
index cfc6748..04cb9a0 100644
--- a/attr.c
+++ b/attr.c
@@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
attr_stack = elem;
}
-static const char *find_basename(const char *path)
-{
- const char *cp, *last_slash = NULL;
-
- for (cp = path; *cp; cp++) {
- if (*cp == '/' cp[1])
- last_slash = cp;
- }
- return last_slash ? last_slash + 1 : path;
-}
-
-static void prepare_attr_stack(const char *path)
+static void prepare_attr_stack(const char *path, int dirlen)
{
struct attr_stack *elem, *info;
- int dirlen, len;
+ int len;
const char *cp;
- dirlen = find_basename(path) - path;
-
- /*
-* find_basename() includes the trailing slash, but we do
-* _not_ want it.
-*/
- if (dirlen)
- dirlen--;
-
/*
* At the bottom of the attribute stack is the built-in
* set of attribute definitions, followed by the contents
@@ -769,15 +749,27 @@ static int macroexpand_one(int attr_nr, int rem)
static void collect_all_attrs(const char *path)
{
struct attr_stack *stk;
- int i, pathlen, rem;
- const char *basename;
+ int i, pathlen, rem, dirlen = 0;
+ const char *basename = path, *cp;
- prepare_attr_stack(path);
+ pathlen = strlen(path);
+
+ /*
+* This loop is similar to strrchr(path, '/') except that the
+* trailing slash is skipped.
+*/
+ for (cp = path + pathlen - 2; cp = path; cp--) {
+ if (*cp == '/') {
+ basename = cp + 1;
+ dirlen = cp - path;
+ break;
+ }
+ }
+
+ prepare_attr_stack(path, dirlen);
for (i = 0; i attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- basename = find_basename(path);
- pathlen = strlen(path);
rem = attr_nr;
for (stk = attr_stack; 0 rem stk; stk = stk-prev)
rem = fill(path, pathlen, basename, stk, rem);
--
1.8.0.rc3.18.g0d9b108
-- 8 --
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
Duy Nguyen pclo...@gmail.com writes:
On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote:
Actually I'd like to remove that function.
This is what I had in mind:
I think the replacement logic to find the basename is moderately
inferiour to the original. For one thing (this may be somewhat
subjective), it is less readable now. Also the original only
scanned the string from the beginning once (instead of letting
strlen() to scan once and go back).
The new code structure to inline the basename finding part and to
pass the dirlen down the callchain may make sense, though.
-- 8 --
Subject: [PATCH] attr: avoid calling find_basename() twice per path
find_basename() is only used inside collect_all_attrs(), called once
in prepare_attr_stack, then again after prepare_attr_stack()
returns. Both calls return exact same value. Reorder the code to do it
once.
While at it, make use of pathlen to stop searching early if
possible.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
attr.c | 46 +++---
1 file changed, 19 insertions(+), 27 deletions(-)
diff --git a/attr.c b/attr.c
index cfc6748..04cb9a0 100644
--- a/attr.c
+++ b/attr.c
@@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
attr_stack = elem;
}
-static const char *find_basename(const char *path)
-{
- const char *cp, *last_slash = NULL;
-
- for (cp = path; *cp; cp++) {
- if (*cp == '/' cp[1])
- last_slash = cp;
- }
- return last_slash ? last_slash + 1 : path;
-}
-
-static void prepare_attr_stack(const char *path)
+static void prepare_attr_stack(const char *path, int dirlen)
{
struct attr_stack *elem, *info;
- int dirlen, len;
+ int len;
const char *cp;
- dirlen = find_basename(path) - path;
-
- /*
- * find_basename() includes the trailing slash, but we do
- * _not_ want it.
- */
- if (dirlen)
- dirlen--;
-
/*
* At the bottom of the attribute stack is the built-in
* set of attribute definitions, followed by the contents
@@ -769,15 +749,27 @@ static int macroexpand_one(int attr_nr, int rem)
static void collect_all_attrs(const char *path)
{
struct attr_stack *stk;
- int i, pathlen, rem;
- const char *basename;
+ int i, pathlen, rem, dirlen = 0;
+ const char *basename = path, *cp;
- prepare_attr_stack(path);
+ pathlen = strlen(path);
+
+ /*
+ * This loop is similar to strrchr(path, '/') except that the
+ * trailing slash is skipped.
+ */
+ for (cp = path + pathlen - 2; cp = path; cp--) {
+ if (*cp == '/') {
+ basename = cp + 1;
+ dirlen = cp - path;
+ break;
+ }
+ }
+
+ prepare_attr_stack(path, dirlen);
for (i = 0; i attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- basename = find_basename(path);
- pathlen = strlen(path);
rem = attr_nr;
for (stk = attr_stack; 0 rem stk; stk = stk-prev)
rem = fill(path, pathlen, basename, stk, rem);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
On Wed, Jan 16, 2013 at 9:33 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote: Actually I'd like to remove that function. This is what I had in mind: I think the replacement logic to find the basename is moderately inferiour to the original. For one thing (this may be somewhat subjective), it is less readable now. Yeah, maybe it's micro optimization. Also the original only scanned the string from the beginning once (instead of letting strlen() to scan once and go back). But we do need to strlen() anyway in collect_all_attrs(). So we scan the string 3 times (strlen + 2 * find_basename) in the original. Now we do it twice -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
Duy Nguyen pclo...@gmail.com writes: On Wed, Jan 16, 2013 at 9:33 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote: Actually I'd like to remove that function. This is what I had in mind: I think the replacement logic to find the basename is moderately inferiour to the original. For one thing (this may be somewhat subjective), it is less readable now. Yeah, maybe it's micro optimization. Your change is micro unoptimization (and making the result less readable). I wouldn't worry too much about micro-optimizing an existing piece of code, but making an efficient code into a worse one without a good reason is a different story. Also the original only scanned the string from the beginning once (instead of letting strlen() to scan once and go back). But we do need to strlen() anyway in collect_all_attrs(). That is exactly my point, isn't it? The loop to find the basename has to run to the end of the string at least once, as it cannot not stop at the last slash---it goes from front to back and it won't know which one is the last slash until it sees the end of the string. After the loop exits, you know the length of the string without running a separate strlen() to assign to pathlen. So we scan the string 3 times (strlen + 2 * find_basename) in the original. Now we do it twice. I already said that overall restructure of the code may be a good idea to reduce the calls to the function. I was only comparing the implementations of the loop that finds the basename, so I do not understand what you mean by that 2 * in that comparison. It does not make sense to me. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] attr: fix off-by-one directory component length calculation
On Tue, Jan 15, 2013 at 09:35:21PM -0800, Junio C Hamano wrote:
Also the original only
scanned the string from the beginning once (instead of letting
strlen() to scan once and go back).
But we do need to strlen() anyway in collect_all_attrs().
That is exactly my point, isn't it?
OK I get your point now. Something like this?
-- 8 --
Subject: [PATCH] attr: avoid calling find_basename() twice per path
find_basename() is only used inside collect_all_attrs(), called once
in prepare_attr_stack, then again after prepare_attr_stack()
returns. Both calls return exact same value. Reorder the code to do
the same task once. Also avoid strlen() because we knows the length
after finding basename.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
attr.c | 45 ++---
1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/attr.c b/attr.c
index cfc6748..880f862 100644
--- a/attr.c
+++ b/attr.c
@@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
attr_stack = elem;
}
-static const char *find_basename(const char *path)
-{
- const char *cp, *last_slash = NULL;
-
- for (cp = path; *cp; cp++) {
- if (*cp == '/' cp[1])
- last_slash = cp;
- }
- return last_slash ? last_slash + 1 : path;
-}
-
-static void prepare_attr_stack(const char *path)
+static void prepare_attr_stack(const char *path, int dirlen)
{
struct attr_stack *elem, *info;
- int dirlen, len;
+ int len;
const char *cp;
- dirlen = find_basename(path) - path;
-
- /*
-* find_basename() includes the trailing slash, but we do
-* _not_ want it.
-*/
- if (dirlen)
- dirlen--;
-
/*
* At the bottom of the attribute stack is the built-in
* set of attribute definitions, followed by the contents
@@ -769,15 +749,26 @@ static int macroexpand_one(int attr_nr, int rem)
static void collect_all_attrs(const char *path)
{
struct attr_stack *stk;
- int i, pathlen, rem;
- const char *basename;
+ int i, pathlen, rem, dirlen;
+ const char *basename, *cp, *last_slash = NULL;
+
+ for (cp = path; *cp; cp++) {
+ if (*cp == '/' cp[1])
+ last_slash = cp;
+ }
+ pathlen = cp - path;
+ if (last_slash) {
+ basename = last_slash + 1;
+ dirlen = last_slash - path;
+ } else {
+ basename = path;
+ dirlen = 0;
+ }
- prepare_attr_stack(path);
+ prepare_attr_stack(path, dirlen);
for (i = 0; i attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- basename = find_basename(path);
- pathlen = strlen(path);
rem = attr_nr;
for (stk = attr_stack; 0 rem stk; stk = stk-prev)
rem = fill(path, pathlen, basename, stk, rem);
--
1.8.0.rc3.18.g0d9b108
-- 8 --
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: t9902 fails (Was: [PATCH] attr: fix off-by-one directory component length calculation)
On 01/16/2013 12:24 AM, Jeff King wrote: On Tue, Jan 15, 2013 at 12:49:05PM -0800, Junio C Hamano wrote: Jean-Noël AVILAavila...@gmail.com writes: Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known issue? Which branch? t9902.10 is overly sensitive to extra git commands in your PATH, as well as cruft in your build dir (especially if you have been building 'pu', which has git-check-ignore). Try make clean make test. -Peff This may help, or it may not. If there are other binaries like git-check-email or git-check-ignore in the PATH . When you switch to a branch generating a file like git-check-ignore then make clean will know about it and will remove it. If you switch to master, then make clean will not remove it. What does git status say? We had a discussion about this some weeks ago, but never concluded. How about this: http://comments.gmane.org/gmane.comp.version-control.git/211907 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
