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

[Junio C Hamano <gitster@pobox.com>]

Duy Nguyen <pclouds@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 <pclouds@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);