Re: [PATCH] attr: fix off-by-one directory component length calculation

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 16, 2013, 06:02 UTC
Message-ID: <20130116060238.GA29523@duynguyen-vnpc.dek-tpc.internal>
In-Reply-To: <7vtxqhzo4m.fsf@alter.siamese.dyndns.org>

On Tue, Jan 15, 2013 at 09:35:21PM -0800, Junio C Hamano wrote:

Show 7 quoted lines

> >> 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 <pclouds@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< --