Re: [PATCH v5 2/3] path: add a find_basename() portability function

From: Jeff King <peff@peff.net>
Date: May 30, 2009, 14:05 UTC
Message-ID: <20090530140519.GA22905@sigill.intra.peff.net>
In-Reply-To: <1243649890-4522-2-git-send-email-davvid@gmail.com>

On Fri, May 29, 2009 at 07:18:09PM -0700, David Aguilar wrote:

Show 13 quoted lines

> +/* return the basename of a path */
> +const char *find_basename(const char *path)
> +{
> +	const char *basename = path + strlen(path) - 1;
> +	while(*basename && basename > path) {
> +		basename--;
> +		if (is_dir_sep(*basename)) {
> +			basename++;
> +			break;
> +		}
> +	}
> +	return basename;
> +}

Hmm. Is there any point to the *basename condition in the loop? By using strlen, you are not going to go past any NULs, and you are already checking that you don't go past the beginning of the string.

Speaking of which, how does this handle an input of ""? It seems that it would return a pointer to one character before the string. Given your loop, you need to special-case when *path is NUL.

Also, how should trailing dir_seps be handled? basename() will actually return "" if given "foo/". Your implementation, when given "/foo/bar/" will return "bar/" (and it must keep the trailing bit since we are neither reallocating nor munging the input string). But given "/foo/bar//", it will return simply "/". I could see an argument for either "bar//" or "", but I think behaving differently for trailing "/" versus trailing "//" doesn't make sense.

-Peff