Re: [PATCH] attr: fix off-by-one directory component length calculation
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:47
Duy Nguyen [off-list ref] writes:
On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote:quoted
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.
quoted hunk
quoted
-- 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 <redacted> --- 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);