Subject: Re: [PATCH] cache_tree_find(): remove redundant check in while condition

From: David Kastrup <[email-masked]>
Date: Mon, 03 Mar 2014 17:32:37 +0100

Michael Haggerty <[email-masked]> writes:

> Signed-off-by: Michael Haggerty <[email-masked]>
> ---
> A trivial little cleanup.
>
>  cache-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 0bbec43..7f63c23 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -565,7 +565,7 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
>  			return NULL;
>  		it = sub->cache_tree;
>  		if (slash)
> -			while (*slash && *slash == '/')
> +			while (*slash == '/')
>  				slash++;
>  		if (!slash || !*slash)
>  			return it; /* prefix ended with slashes */

That seems dragging around a NULL slash a lot.  How about not checking
for it multiple times?

		if (!slash)
                	return it;
		while (*slash == '/')
			slash++;
		if (!*slash)
			return it; /* prefix ended with slashes */

As a bonus, the comment appears to be actually correct.  Attempting to
verify its correctness by seeing whether a non-NULL slash is guaranteed
to really end with slashes, we find the following code above for
defining slash:

                slash = strchr(path, '/');
                if (!slash)
                        slash = path + strlen(path);

So it is literally impossible for slash to ever be NULL and all the
checking is nonsensical.  In addition, "prefix ended with slashes" does
not seem overly convincing when this code path is reached whether or not
there is a slash at all (I am not sure about it: it depends on the
preceding find_subtree to some degree).

So perhaps all of that should just be

		while (*slash == '/')
			slash++;
		if (!*slash)
			return it;

-- 
David Kastrup