Re: [PATCH] Provide a dirname() function when NO_LIBGEN_H=YesPlease

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 8, 2016, 16:17 UTC
Message-ID: <alpine.DEB.2.20.1601081707340.2964@virtualbox>
In-Reply-To: <xmqq8u7n934i.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Wed, 30 Sep 2015, Junio C Hamano wrote:

Show 24 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/compat/basename.c b/compat/basename.c
> > index d8f8a3c..10dba38 100644
> > --- a/compat/basename.c
> > +++ b/compat/basename.c
> > @@ -13,3 +13,29 @@ char *gitbasename (char *path)
> >  	}
> >  	return (char *)base;
> >  }
> > +
> > +char *gitdirname(char *path)
> > +{
> > +	char *p = path, *slash, c;
> > +
> > +	/* Skip over the disk name in MSDOS pathnames. */
> > +	if (has_dos_drive_prefix(p))
> > +		p += 2;
> 
> Not a new problem, but many callers of has_dos_drive_prefix()
> hardcodes that "2" in various forms.  I wonder if this is something
> we should relieve callers of by tweaking the semantics of it, e.g.
> by returning 2 (or howmanyever bytes should be skipped) from the
> function, changing it to skip_dos_drive_prefix(&p), etc.

In the upcoming v2, this is addressed.

Show 6 quoted lines

> > +	/* POSIX.1-2001 says dirname("/") should return "/" */
> > +	slash = is_dir_sep(*p) ? ++p : NULL;
> > +	while ((c = *(p++)))
> 
> I am confused by this.  What is the invariant on 'p' at the
> beginning of the body of this while loop in each iteration?

The idea was that 'p' looks at whatever is the next character. And 'slash' records the location of the latest slash we have seen (to be overwritten with a '\0' to terminate the dirname).

Since '/' should not be shortened to the empty string, I special-cased absolute paths to actually shift the 'slash' to one byte later. A little dirty, but it worked.

Except that Ramsay's tests pointed out that I did not even look at what the specs say, and I even fixed basename() in the meantime (and I think dirname() looks more readable, but feel free to contradict me there).

Show 11 quoted lines

> > +		if (is_dir_sep(c)) {
> > +			char *tentative = p - 1;
> > +
> > +			/* POSIX.1-2001 says to ignore trailing slashes */
> > +			while (is_dir_sep(*p))
> > +				p++;
> > +			if (*p)
> > +				slash = tentative;
> > +		}
> 
> I would have expected the function to scan from the end/right/tail.

Why scan twice (once to find the end, then to find the slashes)?

Ciao, Dscho