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

From: David Aguilar <davvid@gmail.com>
Date: May 30, 2009, 22:14 UTC
Message-ID: <20090530221413.GA10850@gmail.com>
In-Reply-To: <20090530140519.GA22905@sigill.intra.peff.net>

On Sat, May 30, 2009 at 10:05:19AM -0400, Jeff King wrote:

Show 33 quoted lines

> On Fri, May 29, 2009 at 07:18:09PM -0700, David Aguilar wrote:
> 
> > +/* 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

You know.. going with the compat/basename.c solution is probably a better idea now that you've pointed out all these issues. Then we can just use the system's basename() when available and use our compat/basename.c otherwise (is it only for Windows?).

Here's basename() from libiberty.

It drops constness (likely to keep in line with the standard) and has some #defines that I'll have to translate into git equivalents (e.g. IS_DIR_SEPARATOR -> is_dir_sep).

Do we have a cross-platform ISALPHA thing (just thinking out loud -- I can check). I'm under the impression that going with something like this should get us in a better place.

What do you think?

char *basename (const char *name)
{
	const char *base;
#if defined (HAVE_DOS_BASED_FILE_SYSTEM)
	/* Skip over the disk name in MSDOS pathnames. */
	if (ISALPHA (name[0]) && name[1] == ':') 
		name += 2;
#endif

	for (base = name; *name; name++) {
		if (IS_DIR_SEPARATOR (*name)) {
			base = name + 1;
		}
	}
	return (char *) base;
}

-- 
		David