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

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

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