Re: [PATCH v4 2/4] compat/basename: make basename() conform to POSIX
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 13, 2016, 07:14 UTC
- Message-ID
- <alpine.DEB.2.20.1601130803440.2964@virtualbox>
- In-Reply-To
- <56959EFF.6050207@ramsayjones.plus.com>
Hi Ramsay,
On Wed, 13 Jan 2016, Ramsay Jones wrote:
Show 37 quoted lines
> On 12/01/16 07:57, Johannes Schindelin wrote:
> > diff --git a/compat/basename.c b/compat/basename.c
> > index 9f00421..0f1b0b0 100644
> > --- a/compat/basename.c
> > +++ b/compat/basename.c
> > @@ -4,10 +4,24 @@
> > char *gitbasename (char *path)
> > {
> > const char *base;
> > - skip_dos_drive_prefix(&path);
> > +
> > + if (path)
> > + skip_dos_drive_prefix(&path);
> > +
> > + if (!path || !*path)
> > + return ".";
> > +
> > for (base = path; *path; path++) {
> > - if (is_dir_sep(*path))
> > - base = path + 1;
> > + if (!is_dir_sep(*path))
> > + continue;
> > + do {
> > + path++;
> > + } while (is_dir_sep(*path));
> > + if (*path)
> > + base = path;
> > + else
> > + while (--path != base && is_dir_sep(*path))
> > + *path = '\0';
> > }
> > return (char *)base;
> > }
> >
>
> I don't suppose it makes much difference, but I find my version
> slightly easier to read:Yours is better documented, yes, but as I said, I started from what Git already had and tried to provide as minimal changes as possible, to make reviewing easy. In any case, I am very reluctant when it comes to wholesale code replacements: in my experience, these frequently lead to new, entertaining and unintended behavior. I worked with somebody who (for the sake of charity) in the following I will reference only by his most frequent commit message: Dr "Completely new version" (and yes, this was the extent of the commit message). If you buy me a beer or three, I will gladly tell you all the fun I had trying to find the regressions in that code.
In short: please accept that my decision to build on the existing code rather than replacing it had nothing to do with your code.
Ciao, Dscho