Re: [PATCH] attr: fix off-by-one directory component length calculation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 16, 2013, 02:33 UTC
- Message-ID
- <7vbocp26xa.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20130116020901.GA1041@duynguyen-vnpc.dek-tpc.internal>
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.
Show 88 quoted lines
>> -- 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);