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

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

[Johannes Schindelin]

When there is no `libgen.h` to our disposal, we miss the `dirname()`
function.

So far, we only had one user of that function: credential-cache--daemon
(which was only compiled when Unix sockets are available, anyway). But
now we also have `builtin/am.c` as user, so we need it.

Since `dirname()` is a sibling of `basename()`, we simply put our very
own `gitdirname()` implementation next to `gitbasename()` and use it
if `NO_LIBGEN_H` has been set.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I stumbled over the compile warning when upgrading Git for Windows
	to 2.6.0. There was a left-over NO_LIBGEN_H=YesPlease (which we
	no longer need in Git for Windows 2.x), but it did point to the
	fact that we use `dirname()` in builtin/am.c now, so we better
	have a fall-back implementation for platforms without libgen.h.

	I tested this implementation a bit, but I still would appreciate
	a few eye-balls to go over it.

 compat/basename.c | 26 ++++++++++++++++++++++++++
 git-compat-util.h |  2 ++
 2 files changed, 28 insertions(+)

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;
+	/* POSIX.1-2001 says dirname("/") should return "/" */
+	slash = is_dir_sep(*p) ? ++p : NULL;
+	while ((c = *(p++)))
+		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;
+		}
+
+	if (!slash)
+		return ".";
+	*slash = '\0';
+	return path;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index f649e81..8b01aa5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -253,6 +253,8 @@ struct itimerval {
 #else
 #define basename gitbasename
 extern char *gitbasename(char *);
+#define dirname gitdirname
+extern char *gitdirname(char *);
 #endif
 
 #ifndef NO_ICONV
-- 
2.5.3.windows.1.3.gc322723

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> 	I stumbled over the compile warning when upgrading Git for Windows
> 	to 2.6.0. There was a left-over NO_LIBGEN_H=YesPlease (which we
> 	no longer need in Git for Windows 2.x), but it did point to the
> 	fact that we use `dirname()` in builtin/am.c now, so we better
> 	have a fall-back implementation for platforms without libgen.h.

Thanks for being careful.

>
> 	I tested this implementation a bit, but I still would appreciate
> 	a few eye-balls to go over it.
>
>  compat/basename.c | 26 ++++++++++++++++++++++++++
>  git-compat-util.h |  2 ++
>  2 files changed, 28 insertions(+)
>
> 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.

> +	/* 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?

Inside the body, p skips over dir-sep characters, so p must point at
the byte past the last run of slashes?

If that is the invariant, upon entry, shouldn't the initialization
of "slash" be skipping over all slashes, not just the first one,
when the input is "///foo", for example?  Instead the above skips '/'
and sets slash to the byte past the first '/' (which is OK because
you want to NUL-terminate to remove "//foo" from the input) but does
not move p to 'f', so the invariant is not "p must point at the byte
past the last run of slashes".

> +		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.

> +	if (!slash)
> +		return ".";
> +	*slash = '\0';
> +	return path;
> +}
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f649e81..8b01aa5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -253,6 +253,8 @@ struct itimerval {
>  #else
>  #define basename gitbasename
>  extern char *gitbasename(char *);
> +#define dirname gitdirname
> +extern char *gitdirname(char *);
>  #endif
>  
>  #ifndef NO_ICONV

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

[Johannes Schindelin]

Hi Junio,

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

> 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.

> > +	/* 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).

> > +		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

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

[Ramsay Jones]

[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]

Hi Johannes,

On 30/09/15 15:50, Johannes Schindelin wrote:
> When there is no `libgen.h` to our disposal, we miss the `dirname()`
> function.
> 
> So far, we only had one user of that function: credential-cache--daemon
> (which was only compiled when Unix sockets are available, anyway). But
> now we also have `builtin/am.c` as user, so we need it.

Yes, many moons ago (on my old 32-bit laptop) when I was still 'working'
with MinGW I noticed this same thing while looking into providing a win32
emulation of unix sockets. So, I had to look into this at the same time.
Since this didn't progress, I didn't mention the libgen issue.

Anyway, I still have a 'test-libgen.c' file (attached) from back then that
contains some tests. I don't quite recall what the final state of this
code was, but it was intended to test _existing_ libgen implementations
as well as provide a 'git' version which would work on MinGW, cygwin and
linux. Note that some of the existing implementations didn't all agree on
what the tests should report! I don't remember if I looked at the POSIX
spec or not.

So, I don't know how useful it will be - if nothing else, there are some
tests! :-D

HTH

Ramsay Jones


> 
> Since `dirname()` is a sibling of `basename()`, we simply put our very
> own `gitdirname()` implementation next to `gitbasename()` and use it
> if `NO_LIBGEN_H` has been set.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	I stumbled over the compile warning when upgrading Git for Windows
> 	to 2.6.0. There was a left-over NO_LIBGEN_H=YesPlease (which we
> 	no longer need in Git for Windows 2.x), but it did point to the
> 	fact that we use `dirname()` in builtin/am.c now, so we better
> 	have a fall-back implementation for platforms without libgen.h.
> 
> 	I tested this implementation a bit, but I still would appreciate
> 	a few eye-balls to go over it.
> 
>  compat/basename.c | 26 ++++++++++++++++++++++++++
>  git-compat-util.h |  2 ++
>  2 files changed, 28 insertions(+)
> 
> 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;
> +	/* POSIX.1-2001 says dirname("/") should return "/" */
> +	slash = is_dir_sep(*p) ? ++p : NULL;
> +	while ((c = *(p++)))
> +		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;
> +		}
> +
> +	if (!slash)
> +		return ".";
> +	*slash = '\0';
> +	return path;
> +}
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f649e81..8b01aa5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -253,6 +253,8 @@ struct itimerval {
>  #else
>  #define basename gitbasename
>  extern char *gitbasename(char *);
> +#define dirname gitdirname
> +extern char *gitdirname(char *);
>  #endif
>  
>  #ifndef NO_ICONV
> 

[-- Attachment #2: test-libgen.c --]
[-- Type: text/x-csrc, Size: 6506 bytes --]

#include <stdio.h>
#include <string.h>
#include <ctype.h>
#ifndef NO_LIBGEN_H
# include <libgen.h>
#endif

struct test_data {
	char *from;  /* input:  transform from this ... */
	char *to;    /* output: ... to this.            */
};

#ifdef NO_LIBGEN_H

#if defined(__MINGW32__) || defined(_MSC_VER)
#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
#define is_dir_sep(c) ((c) == '/' || (c) == '\\')
#else
#define has_dos_drive_prefix(path) 0
#define is_dir_sep(c) ((c) == '/')
#endif

#define basename gitbasename
#define dirname gitdirname

char *gitbasename (char *path)
{
	char *p;

	if (!path || !*path)
		return ".";
	/* skip drive designator, if any */
	if (has_dos_drive_prefix(path))
		path += 2;
	if (!*path)
		return ".";
	/* trim trailing directory separators */
	p = path + strlen(path) - 1;
	while (is_dir_sep(*p)) {
		if (p == path)
			return path;
		*p-- = '\0';
	}
	/* find begining of last path component */
	while (p >= path && !is_dir_sep(*p))
		p--;
	return p + 1;
}

char *gitdirname(char *path)
{
	char *p, *start;

	if (!path || !*path)
		return ".";
	start = path;
	/* skip drive designator, if any */
	if (has_dos_drive_prefix(path))
		start += 2;
	/* check for // */
	if (strcmp(start, "//") == 0)
		return path;
	/* check for \\ */
	if (is_dir_sep('\\') && strcmp(start, "\\\\") == 0)
		return path;
	/* trim trailing directory separators */
	p = path + strlen(path) - 1;
	while (is_dir_sep(*p)) {
		if (p == start)
			return path;
		*p-- = '\0';
	}
	/* find begining of last path component */
	while (p >= start && !is_dir_sep(*p))
		p--;
	/* terminate dirname */
	if (p < start) {
		p = start;
		*p++ = '.';
	} else if (p == start)
		p++;
	*p = '\0';
	return path;
}

#endif

static int test_basename(void)
{
	static struct test_data t[] = {

		/* --- POSIX type paths --- */
		{ NULL,              "."    },
		{ "",                "."    },
		{ ".",               "."    },
		{ "..",              ".."   },
		{ "/",               "/"    },
#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
		{ "//",              "//"   },
		{ "///",             "//"   },
		{ "////",            "//"   },
#else
		{ "//",              "/"    },
		{ "///",             "/"    },
		{ "////",            "/"    },
#endif
		{ "usr",             "usr"  },
		{ "/usr",            "usr"  },
		{ "/usr/",           "usr"  },
		{ "/usr//",          "usr"  },
		{ "/usr/lib",        "lib"  },
		{ "usr/lib",         "lib"  },
		{ "usr/lib///",      "lib"  },

#if defined(__MINGW32__) || defined(_MSC_VER)

		/* --- win32 type paths --- */
		{ "\\usr",           "usr"  },
		{ "\\usr\\",         "usr"  },
		{ "\\usr\\\\",       "usr"  },
		{ "\\usr\\lib",      "lib"  },
		{ "usr\\lib",        "lib"  },
		{ "usr\\lib\\\\\\",  "lib"  },
		{ "C:/usr",          "usr"  },
		{ "C:/usr",          "usr"  },
		{ "C:/usr/",         "usr"  },
		{ "C:/usr//",        "usr"  },
		{ "C:/usr/lib",      "lib"  },
		{ "C:usr/lib",       "lib"  },
		{ "C:usr/lib///",    "lib"  },
		{ "C:",              "."    },
		{ "C:a",             "a"    },
		{ "C:/",             "/"    },
		{ "C:///",           "/"    },
#if defined(NO_LIBGEN_H)
		{ "\\",              "\\"   },
		{ "\\\\",            "\\"   },
		{ "\\\\\\",          "\\"   },
#else

		/* win32 platform variations: */
#if defined(__MINGW32__)
		{ "\\",              "/"    },
		{ "\\\\",            "/"    },
		{ "\\\\\\",          "/"    },
#endif

#if defined(_MSC_VER)
		{ "\\",              "\\"   },
		{ "\\\\",            "\\"   },
		{ "\\\\\\",          "\\"   },
#endif

#endif
#endif
		{ NULL,              "."    }
	};
	static char input[1024];
	char *from, *to;
	int i, failed = 0;

	for (i = 0; i < sizeof(t)/sizeof(t[0]); i++) {
		from = NULL;
		if (t[i].from) {
			strcpy(input, t[i].from);
			from = input;
		}
		to = basename(from);
		if (strcmp(to, t[i].to) != 0) {
			fprintf(stderr, "FAIL: basename(%s) => '%s' != '%s'\n",
				t[i].from, to, t[i].to);
			failed++;
		}
	}
	return failed != 0;
}

static int test_dirname(void)
{
	static struct test_data t[] = {

		/* --- POSIX type paths --- */
		{ NULL,              "."      },
		{ "",                "."      },
		{ ".",               "."      },
		{ "..",              "."      },
		{ "/",               "/"      },
		{ "//",              "//"     },
#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
		{ "///",             "//"     },
		{ "////",            "//"     },
#else
		{ "///",             "/"      },
		{ "////",            "/"      },
#endif
		{ "usr",             "."      },
		{ "/usr",            "/"      },
		{ "/usr/",           "/"      },
		{ "/usr//",          "/"      },
		{ "/usr/lib",        "/usr"   },
		{ "usr/lib",         "usr"    },
		{ "usr/lib///",      "usr"    },

#if defined(__MINGW32__) || defined(_MSC_VER)

		/* --- win32 type paths --- */
		{ "\\",              "\\"     },
		{ "\\\\",            "\\\\"   },
		{ "\\usr",           "\\"     },
		{ "\\usr\\",         "\\"     },
		{ "\\usr\\\\",       "\\"     },
		{ "\\usr\\lib",      "\\usr"  },
		{ "usr\\lib",        "usr"    },
		{ "usr\\lib\\\\\\",  "usr"    },
		{ "C:a",             "C:."    },
		{ "C:/",             "C:/"    },
		{ "C:///",           "C:/"    },
		{ "C:/usr",          "C:/"    },
		{ "C:/usr/",         "C:/"    },
		{ "C:/usr//",        "C:/"    },
		{ "C:/usr/lib",      "C:/usr" },
		{ "C:usr/lib",       "C:usr"  },
		{ "C:usr/lib///",    "C:usr"  },
		{ "\\\\\\",          "\\"     },
		{ "\\\\\\\\",        "\\"     },
#if defined(NO_LIBGEN_H)
		{ "C:",              "C:."    },
#else

		/* win32 platform variations: */
#if defined(__MINGW32__)
		/* the following is clearly wrong ... */
		{ "C:",              "."      },
#endif

#if defined(_MSC_VER)
		{ "C:",              "C:."    },
#endif

#endif
#endif
		{ NULL,              "."      }
	};
	static char input[1024];
	char *from, *to;
	int i, failed = 0;

	for (i = 0; i < sizeof(t)/sizeof(t[0]); i++) {
		from = NULL;
		if (t[i].from) {
			strcpy(input, t[i].from);
			from = input;
		}
		to = dirname(from);
		if (strcmp(to, t[i].to) != 0) {
			fprintf(stderr, "FAIL: dirname(%s) => '%s' != '%s'\n",
				t[i].from, to, t[i].to);
			failed++;
		}
	}
	return failed != 0;
}

int main(int argc, char **argv)
{
	if (argc == 2 && !strcmp(argv[1], "basename"))
		return test_basename();

	if (argc == 2 && !strcmp(argv[1], "dirname"))
		return test_dirname();

	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
		argv[1] ? argv[1] : "(there was none)");
	return 1;
}

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

[Johannes Schindelin]

Hi Ramsay,

On Wed, 30 Sep 2015, Ramsay Jones wrote:

> On 30/09/15 15:50, Johannes Schindelin wrote:
> > When there is no `libgen.h` to our disposal, we miss the `dirname()`
> > function.
> > 
> > So far, we only had one user of that function:
> > credential-cache--daemon (which was only compiled when Unix sockets
> > are available, anyway). But now we also have `builtin/am.c` as user,
> > so we need it.
> 
> Yes, many moons ago (on my old 32-bit laptop) when I was still 'working'
> with MinGW I noticed this same thing while looking into providing a win32
> emulation of unix sockets. So, I had to look into this at the same time.
> Since this didn't progress, I didn't mention the libgen issue.
> 
> Anyway, I still have a 'test-libgen.c' file (attached) from back then that
> contains some tests.

Awesome. Thank you! I integrated the tests back into test-path-utils.c
(from where the framework clearly came) and made it part of the regression
test suite in the upcoming v2.

Ciao,
Dscho

[PATCH v2 0/4] Ensure that we can build without libgen.h

[Johannes Schindelin]

This mini series adds a fall-back for the `dirname()` function that we use
e.g. in git-am. This is necessary because not all platforms have a working
libgen.h. MSys2 (on which Git for Windows relies) does have a libgen.h, but
its `basename()` implementation is broken, thus we cannot use it.


Johannes Schindelin (4):
  Refactor skipping DOS drive prefixes
  compat/basename: make basename() conform to POSIX
  Provide a dirname() function when NO_LIBGEN_H=YesPlease
  t0060: verify that basename() and dirname() work as expected

 compat/basename.c     |  66 ++++++++++++++++++--
 compat/mingw.c        |  14 ++---
 compat/mingw.h        |  10 ++-
 git-compat-util.h     |  10 +++
 path.c                |  14 ++---
 t/t0060-path-utils.sh |   3 +
 test-path-utils.c     | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 261 insertions(+), 24 deletions(-)

Interdiff vs v1:

 diff --git a/compat/basename.c b/compat/basename.c
 index 10dba38..0a2ed25 100644
 --- a/compat/basename.c
 +++ b/compat/basename.c
 @@ -1,28 +1,58 @@
  #include "../git-compat-util.h"
 +#include "../strbuf.h"
  
  /* Adapted from libiberty's basename.c.  */
  char *gitbasename (char *path)
  {
  	const char *base;
 -	/* Skip over the disk name in MSDOS pathnames. */
 -	if (has_dos_drive_prefix(path))
 -		path += 2;
 +
 +	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;
  }
  
  char *gitdirname(char *path)
  {
 -	char *p = path, *slash, c;
 +	char *p = path, *slash = NULL, c;
 +	int dos_drive_prefix;
 +
 +	if (!p)
 +		return ".";
  
 -	/* Skip over the disk name in MSDOS pathnames. */
 -	if (has_dos_drive_prefix(p))
 -		p += 2;
 -	/* POSIX.1-2001 says dirname("/") should return "/" */
 -	slash = is_dir_sep(*p) ? ++p : NULL;
 +	if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p) {
 +		static struct strbuf buf = STRBUF_INIT;
 +
 +dot:
 +		strbuf_reset(&buf);
 +		strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
 +		return buf.buf;
 +	}
 +
 +	/*
 +	 * POSIX.1-2001 says dirname("/") should return "/", and dirname("//")
 +	 * should return "//", but dirname("///") should return "/" again.
 +	 */
 +	if (is_dir_sep(*p)) {
 +		if (!p[1] || (is_dir_sep(p[1]) && !p[2]))
 +			return path;
 +		slash = ++p;
 +	}
  	while ((c = *(p++)))
  		if (is_dir_sep(c)) {
  			char *tentative = p - 1;
 @@ -35,7 +65,7 @@ char *gitdirname(char *path)
  		}
  
  	if (!slash)
 -		return ".";
 +		goto dot;
  	*slash = '\0';
  	return path;
  }
 diff --git a/compat/mingw.c b/compat/mingw.c
 index 5edea29..1b3530a 100644
 --- a/compat/mingw.c
 +++ b/compat/mingw.c
 @@ -1934,26 +1934,22 @@ pid_t waitpid(pid_t pid, int *status, int options)
  
  int mingw_offset_1st_component(const char *path)
  {
 -	int offset = 0;
 -	if (has_dos_drive_prefix(path))
 -		offset = 2;
 +	char *pos = (char *)path;
  
  	/* unc paths */
 -	else if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
 -
 +	if (!skip_dos_drive_prefix(&pos) &&
 +			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
  		/* skip server name */
 -		char *pos = strpbrk(path + 2, "\\/");
 +		pos = strpbrk(pos + 2, "\\/");
  		if (!pos)
  			return 0; /* Error: malformed unc path */
  
  		do {
  			pos++;
  		} while (*pos && !is_dir_sep(*pos));
 -
 -		offset = pos - path;
  	}
  
 -	return offset + is_dir_sep(path[offset]);
 +	return pos + is_dir_sep(*pos) - path;
  }
  
  int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
 diff --git a/compat/mingw.h b/compat/mingw.h
 index 57ca477..b3e5044 100644
 --- a/compat/mingw.h
 +++ b/compat/mingw.h
 @@ -361,7 +361,15 @@ HANDLE winansi_get_osfhandle(int fd);
   * git specific compatibility
   */
  
 -#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
 +#define has_dos_drive_prefix(path) \
 +	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
 +static inline int mingw_skip_dos_drive_prefix(char **path)
 +{
 +	int ret = has_dos_drive_prefix(*path);
 +	*path += ret;
 +	return ret;
 +}
 +#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
  #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
  static inline char *mingw_find_last_dir_sep(const char *path)
  {
 diff --git a/git-compat-util.h b/git-compat-util.h
 index 996ee17..94f311a 100644
 --- a/git-compat-util.h
 +++ b/git-compat-util.h
 @@ -337,6 +337,14 @@ static inline int git_has_dos_drive_prefix(const char *path)
  #define has_dos_drive_prefix git_has_dos_drive_prefix
  #endif
  
 +#ifndef skip_dos_drive_prefix
 +static inline int git_skip_dos_drive_prefix(const char **path)
 +{
 +	return 0;
 +}
 +#define skip_dos_drive_prefix git_skip_dos_drive_prefix
 +#endif
 +
  #ifndef is_dir_sep
  static inline int git_is_dir_sep(int c)
  {
 diff --git a/path.c b/path.c
 index 3cd155e..8b7e168 100644
 --- a/path.c
 +++ b/path.c
 @@ -782,13 +782,10 @@ const char *relative_path(const char *in, const char *prefix,
  	else if (!prefix_len)
  		return in;
  
 -	if (have_same_root(in, prefix)) {
 +	if (have_same_root(in, prefix))
  		/* bypass dos_drive, for "c:" is identical to "C:" */
 -		if (has_dos_drive_prefix(in)) {
 -			i = 2;
 -			j = 2;
 -		}
 -	} else {
 +		i = j = has_dos_drive_prefix(in);
 +	else {
  		return in;
  	}
  
 @@ -943,11 +940,10 @@ const char *remove_leading_path(const char *in, const char *prefix)
  int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
  {
  	char *dst0;
 +	int i;
  
 -	if (has_dos_drive_prefix(src)) {
 +	for (i = has_dos_drive_prefix(src); i > 0; i--)
  		*dst++ = *src++;
 -		*dst++ = *src++;
 -	}
  	dst0 = dst;
  
  	if (is_dir_sep(*src)) {
 diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
 index 627ef85..f0152a7 100755
 --- a/t/t0060-path-utils.sh
 +++ b/t/t0060-path-utils.sh
 @@ -59,6 +59,9 @@ case $(uname -s) in
  	;;
  esac
  
 +test_expect_success basename 'test-path-utils basename'
 +test_expect_success dirname 'test-path-utils dirname'
 +
  norm_path "" ""
  norm_path . ""
  norm_path ./ ""
 diff --git a/test-path-utils.c b/test-path-utils.c
 index c67bf65..74e74c9 100644
 --- a/test-path-utils.c
 +++ b/test-path-utils.c
 @@ -39,6 +39,168 @@ static void normalize_argv_string(const char **var, const char *input)
  		die("Bad value: %s\n", input);
  }
  
 +struct test_data {
 +	char *from;  /* input:  transform from this ... */
 +	char *to;    /* output: ... to this.            */
 +};
 +
 +static int test_function(struct test_data *data, char *(*func)(char *input),
 +	const char *funcname)
 +{
 +	int failed = 0, i;
 +	static char buffer[1024];
 +	char *to;
 +
 +	for (i = 0; data[i].to; i++) {
 +		if (!data[i].from)
 +			to = func(NULL);
 +		else {
 +			strcpy(buffer, data[i].from);
 +			to = func(buffer);
 +		}
 +		if (strcmp(to, data[i].to)) {
 +			error("FAIL: %s(%s) => '%s' != '%s'\n",
 +				funcname, data[i].from, to, data[i].to);
 +			failed++;
 +		}
 +	}
 +	return !!failed;
 +}
 +
 +static struct test_data basename_data[] = {
 +	/* --- POSIX type paths --- */
 +	{ NULL,              "."    },
 +	{ "",                "."    },
 +	{ ".",               "."    },
 +	{ "..",              ".."   },
 +	{ "/",               "/"    },
 +#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
 +	{ "//",              "//"   },
 +	{ "///",             "//"   },
 +	{ "////",            "//"   },
 +#else
 +	{ "//",              "/"    },
 +	{ "///",             "/"    },
 +	{ "////",            "/"    },
 +#endif
 +	{ "usr",             "usr"  },
 +	{ "/usr",            "usr"  },
 +	{ "/usr/",           "usr"  },
 +	{ "/usr//",          "usr"  },
 +	{ "/usr/lib",        "lib"  },
 +	{ "usr/lib",         "lib"  },
 +	{ "usr/lib///",      "lib"  },
 +
 +#if defined(__MINGW32__) || defined(_MSC_VER)
 +
 +	/* --- win32 type paths --- */
 +	{ "\\usr",           "usr"  },
 +	{ "\\usr\\",         "usr"  },
 +	{ "\\usr\\\\",       "usr"  },
 +	{ "\\usr\\lib",      "lib"  },
 +	{ "usr\\lib",        "lib"  },
 +	{ "usr\\lib\\\\\\",  "lib"  },
 +	{ "C:/usr",          "usr"  },
 +	{ "C:/usr",          "usr"  },
 +	{ "C:/usr/",         "usr"  },
 +	{ "C:/usr//",        "usr"  },
 +	{ "C:/usr/lib",      "lib"  },
 +	{ "C:usr/lib",       "lib"  },
 +	{ "C:usr/lib///",    "lib"  },
 +	{ "C:",              "."    },
 +	{ "C:a",             "a"    },
 +	{ "C:/",             "/"    },
 +	{ "C:///",           "/"    },
 +#if defined(NO_LIBGEN_H)
 +	{ "\\",              "\\"   },
 +	{ "\\\\",            "\\"   },
 +	{ "\\\\\\",          "\\"   },
 +#else
 +
 +	/* win32 platform variations: */
 +#if defined(__MINGW32__)
 +	{ "\\",              "/"    },
 +	{ "\\\\",            "/"    },
 +	{ "\\\\\\",          "/"    },
 +#endif
 +
 +#if defined(_MSC_VER)
 +	{ "\\",              "\\"   },
 +	{ "\\\\",            "\\"   },
 +	{ "\\\\\\",          "\\"   },
 +#endif
 +
 +#endif
 +#endif
 +	{ NULL,              "."    },
 +	{ NULL,              NULL   }
 +};
 +
 +static struct test_data dirname_data[] = {
 +	/* --- POSIX type paths --- */
 +	{ NULL,              "."      },
 +	{ "",                "."      },
 +	{ ".",               "."      },
 +	{ "..",              "."      },
 +	{ "/",               "/"      },
 +	{ "//",              "//"     },
 +#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
 +	{ "///",             "//"     },
 +	{ "////",            "//"     },
 +#else
 +	{ "///",             "/"      },
 +	{ "////",            "/"      },
 +#endif
 +	{ "usr",             "."      },
 +	{ "/usr",            "/"      },
 +	{ "/usr/",           "/"      },
 +	{ "/usr//",          "/"      },
 +	{ "/usr/lib",        "/usr"   },
 +	{ "usr/lib",         "usr"    },
 +	{ "usr/lib///",      "usr"    },
 +
 +#if defined(__MINGW32__) || defined(_MSC_VER)
 +
 +	/* --- win32 type paths --- */
 +	{ "\\",              "\\"     },
 +	{ "\\\\",            "\\\\"   },
 +	{ "\\usr",           "\\"     },
 +	{ "\\usr\\",         "\\"     },
 +	{ "\\usr\\\\",       "\\"     },
 +	{ "\\usr\\lib",      "\\usr"  },
 +	{ "usr\\lib",        "usr"    },
 +	{ "usr\\lib\\\\\\",  "usr"    },
 +	{ "C:a",             "C:."    },
 +	{ "C:/",             "C:/"    },
 +	{ "C:///",           "C:/"    },
 +	{ "C:/usr",          "C:/"    },
 +	{ "C:/usr/",         "C:/"    },
 +	{ "C:/usr//",        "C:/"    },
 +	{ "C:/usr/lib",      "C:/usr" },
 +	{ "C:usr/lib",       "C:usr"  },
 +	{ "C:usr/lib///",    "C:usr"  },
 +	{ "\\\\\\",          "\\"     },
 +	{ "\\\\\\\\",        "\\"     },
 +#if defined(NO_LIBGEN_H)
 +	{ "C:",              "C:."    },
 +#else
 +
 +	/* win32 platform variations: */
 +#if defined(__MINGW32__)
 +	/* the following is clearly wrong ... */
 +	{ "C:",              "."      },
 +#endif
 +
 +#if defined(_MSC_VER)
 +	{ "C:",              "C:."    },
 +#endif
 +
 +#endif
 +#endif
 +	{ NULL,              "."      },
 +	{ NULL,              NULL     }
 +};
 +
  int main(int argc, char **argv)
  {
  	if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
 @@ -133,6 +295,12 @@ int main(int argc, char **argv)
  		return 0;
  	}
  
 +	if (argc == 2 && !strcmp(argv[1], "basename"))
 +		return test_function(basename_data, basename, argv[1]);
 +
 +	if (argc == 2 && !strcmp(argv[1], "dirname"))
 +		return test_function(dirname_data, dirname, argv[1]);
 +
  	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
  		argv[1] ? argv[1] : "(there was none)");
  	return 1;

-- 
2.6.3.windows.1.300.g1c25e49

[PATCH v2 1/4] Refactor skipping DOS drive prefixes

[Johannes Schindelin]

Junio Hamano pointed out that there is an implicit assumption in pretty
much all the code calling has_dos_drive_prefix(): it assumes that the
DOS drive prefix is always two bytes long.

While this assumption is pretty safe, we can still make the code more
readable and less error-prone by introducing a function that skips the
DOS drive prefix safely.

While at it, we change the has_dos_drive_prefix() return value: it now
returns the number of bytes to be skipped if there is a DOS drive prefix.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c |  4 +---
 compat/mingw.c    | 14 +++++---------
 compat/mingw.h    | 10 +++++++++-
 git-compat-util.h |  8 ++++++++
 path.c            | 14 +++++---------
 5 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..9f00421 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -4,9 +4,7 @@
 char *gitbasename (char *path)
 {
 	const char *base;
-	/* Skip over the disk name in MSDOS pathnames. */
-	if (has_dos_drive_prefix(path))
-		path += 2;
+	skip_dos_drive_prefix(&path);
 	for (base = path; *path; path++) {
 		if (is_dir_sep(*path))
 			base = path + 1;
diff --git a/compat/mingw.c b/compat/mingw.c
index 5edea29..1b3530a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1934,26 +1934,22 @@ pid_t waitpid(pid_t pid, int *status, int options)
 
 int mingw_offset_1st_component(const char *path)
 {
-	int offset = 0;
-	if (has_dos_drive_prefix(path))
-		offset = 2;
+	char *pos = (char *)path;
 
 	/* unc paths */
-	else if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
-
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
 		/* skip server name */
-		char *pos = strpbrk(path + 2, "\\/");
+		pos = strpbrk(pos + 2, "\\/");
 		if (!pos)
 			return 0; /* Error: malformed unc path */
 
 		do {
 			pos++;
 		} while (*pos && !is_dir_sep(*pos));
-
-		offset = pos - path;
 	}
 
-	return offset + is_dir_sep(path[offset]);
+	return pos + is_dir_sep(*pos) - path;
 }
 
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
diff --git a/compat/mingw.h b/compat/mingw.h
index 57ca477..b3e5044 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -361,7 +361,15 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+static inline int mingw_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 2da0a75..0d66f3a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -335,6 +335,14 @@ static inline int git_has_dos_drive_prefix(const char *path)
 #define has_dos_drive_prefix git_has_dos_drive_prefix
 #endif
 
+#ifndef skip_dos_drive_prefix
+static inline int git_skip_dos_drive_prefix(const char **path)
+{
+	return 0;
+}
+#define skip_dos_drive_prefix git_skip_dos_drive_prefix
+#endif
+
 #ifndef is_dir_sep
 static inline int git_is_dir_sep(int c)
 {
diff --git a/path.c b/path.c
index 3cd155e..8b7e168 100644
--- a/path.c
+++ b/path.c
@@ -782,13 +782,10 @@ const char *relative_path(const char *in, const char *prefix,
 	else if (!prefix_len)
 		return in;
 
-	if (have_same_root(in, prefix)) {
+	if (have_same_root(in, prefix))
 		/* bypass dos_drive, for "c:" is identical to "C:" */
-		if (has_dos_drive_prefix(in)) {
-			i = 2;
-			j = 2;
-		}
-	} else {
+		i = j = has_dos_drive_prefix(in);
+	else {
 		return in;
 	}
 
@@ -943,11 +940,10 @@ const char *remove_leading_path(const char *in, const char *prefix)
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 {
 	char *dst0;
+	int i;
 
-	if (has_dos_drive_prefix(src)) {
+	for (i = has_dos_drive_prefix(src); i > 0; i--)
 		*dst++ = *src++;
-		*dst++ = *src++;
-	}
 	dst0 = dst;
 
 	if (is_dir_sep(*src)) {
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v2 1/4] Refactor skipping DOS drive prefixes

[Eric Sunshine]

On Fri, Jan 8, 2016 at 11:21 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Junio Hamano pointed out that there is an implicit assumption in pretty
> much all the code calling has_dos_drive_prefix(): it assumes that the
> DOS drive prefix is always two bytes long.
>
> While this assumption is pretty safe, we can still make the code more
> readable and less error-prone by introducing a function that skips the
> DOS drive prefix safely.
>
> While at it, we change the has_dos_drive_prefix() return value: it now
> returns the number of bytes to be skipped if there is a DOS drive prefix.

With this change, code such as:

    for (i = has_dos_drive_prefix(src); i > 0; i--)
        ...

in path.c reads a bit oddly. Renaming the function might help. For instance:

    for (i = dos_drive_prefix_len(src); i > 0; i--)
        ...

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Re: [PATCH v2 1/4] Refactor skipping DOS drive prefixes

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> With this change, code such as:
>
>     for (i = has_dos_drive_prefix(src); i > 0; i--)
>         ...
>
> in path.c reads a bit oddly. Renaming the function might help. For instance:
>
>     for (i = dos_drive_prefix_len(src); i > 0; i--)
>         ...

Renaming may be unnecessary churn, but I do not think we mind an
additional synonym, e.g.

    #define has_dos_drive_prefix(x) dos_drive_prefix_len(x)

if some people prefer.

Re: [PATCH v2 1/4] Refactor skipping DOS drive prefixes

[Johannes Schindelin]

Hi Eric & Junio,

On Fri, 8 Jan 2016, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > With this change, code such as:
> >
> >     for (i = has_dos_drive_prefix(src); i > 0; i--)
> >         ...
> >
> > in path.c reads a bit oddly. Renaming the function might help. For instance:
> >
> >     for (i = dos_drive_prefix_len(src); i > 0; i--)
> >         ...
> 
> Renaming may be unnecessary churn, but I do not think we mind an
> additional synonym, e.g.
> 
>     #define has_dos_drive_prefix(x) dos_drive_prefix_len(x)
> 
> if some people prefer.

I am actually not so sure about this: if I read
`dos_drive_prefix_len(path)` I would have assumed the return value to be
-1 if `path` does not, in fact, have a DOS drive prefix.

Sure, returning the length of the DOS drive prefix when just asking
whether it has one is a bit surprising at first, but it also makes sense:
we already have that information, so we might just as well use it.

In any case, I think this change (if it is really considered desirable)
could easily be an add-on patch by people who care about this ;-)

Ciao,
Dscho

Re: [PATCH v2 1/4] Refactor skipping DOS drive prefixes

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> In any case, I think this change (if it is really considered desirable)
> could easily be an add-on patch by people who care about this ;-)

Yup, in case if it was unclear, that is what I meant.

[PATCH v2 2/4] compat/basename: make basename() conform to POSIX

[Johannes Schindelin]

According to POSIX, basename("/path/") should return "path", not
"path/". Likewise, basename(NULL) and basename("abc") should both
return ".".

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

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;
 }
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v2 2/4] compat/basename: make basename() conform to POSIX

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> According to POSIX, basename("/path/") should return "path", not
> "path/". Likewise, basename(NULL) and basename("abc") should both
> return ".".

Did you mean basename("abc"), not basename(""), here?  


> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/basename.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> 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;
>  }

Re: [PATCH v2 2/4] compat/basename: make basename() conform to POSIX

[Johannes Schindelin]

Hi Junio,

On Fri, 8 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > According to POSIX, basename("/path/") should return "path", not
> > "path/". Likewise, basename(NULL) and basename("abc") should both
> > return ".".
> 
> Did you mean basename("abc"), not basename(""), here?  

I don't understand: I wrote basename("abc")... ;-)

Ciao,
Dscho

Re: [PATCH v2 2/4] compat/basename: make basename() conform to POSIX

[Junio C Hamano]

Johannes Schindelin <schindelin@wisc.edu> writes:

> Hi Junio,
>
> On Fri, 8 Jan 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > According to POSIX, basename("/path/") should return "path", not
>> > "path/". Likewise, basename(NULL) and basename("abc") should both
>> > return ".".
>> 
>> Did you mean basename("abc"), not basename(""), here?  
>
> I don't understand: I wrote basename("abc")... ;-)

Yeah, I read that.  What I didn't read was you wrote 'path' between
slashes in the first example (the MUA was trying to render it as
"path" in italic or something silly like that), and made me
confused: If 'path' has to go to 'path', what's so special about
'abc' to make it go '.'?

Upon second reading I noticed that slashes around the first one, but
forgot to remove the "why 'abc'" comment.

[PATCH v2 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Johannes Schindelin]

When there is no `libgen.h` to our disposal, we miss the `dirname()`
function.

So far, we only had one user of that function: credential-cache--daemon
(which was only compiled when Unix sockets are available, anyway). But
now we also have `builtin/am.c` as user, so we need it.

Since `dirname()` is a sibling of `basename()`, we simply put our very
own `gitdirname()` implementation next to `gitbasename()` and use it
if `NO_LIBGEN_H` has been set.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/compat/basename.c b/compat/basename.c
index 0f1b0b0..0a2ed25 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -1,4 +1,5 @@
 #include "../git-compat-util.h"
+#include "../strbuf.h"
 
 /* Adapted from libiberty's basename.c.  */
 char *gitbasename (char *path)
@@ -25,3 +26,46 @@ char *gitbasename (char *path)
 	}
 	return (char *)base;
 }
+
+char *gitdirname(char *path)
+{
+	char *p = path, *slash = NULL, c;
+	int dos_drive_prefix;
+
+	if (!p)
+		return ".";
+
+	if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p) {
+		static struct strbuf buf = STRBUF_INIT;
+
+dot:
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
+		return buf.buf;
+	}
+
+	/*
+	 * POSIX.1-2001 says dirname("/") should return "/", and dirname("//")
+	 * should return "//", but dirname("///") should return "/" again.
+	 */
+	if (is_dir_sep(*p)) {
+		if (!p[1] || (is_dir_sep(p[1]) && !p[2]))
+			return path;
+		slash = ++p;
+	}
+	while ((c = *(p++)))
+		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;
+		}
+
+	if (!slash)
+		goto dot;
+	*slash = '\0';
+	return path;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 0d66f3a..94f311a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -253,6 +253,8 @@ struct itimerval {
 #else
 #define basename gitbasename
 extern char *gitbasename(char *);
+#define dirname gitdirname
+extern char *gitdirname(char *);
 #endif
 
 #ifndef NO_ICONV
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v2 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> When there is no `libgen.h` to our disposal, we miss the `dirname()`
> function.
>
> So far, we only had one user of that function: credential-cache--daemon
> (which was only compiled when Unix sockets are available, anyway). But
> now we also have `builtin/am.c` as user, so we need it.
>
> Since `dirname()` is a sibling of `basename()`, we simply put our very
> own `gitdirname()` implementation next to `gitbasename()` and use it
> if `NO_LIBGEN_H` has been set.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/basename.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  git-compat-util.h |  2 ++
>  2 files changed, 46 insertions(+)
>
> diff --git a/compat/basename.c b/compat/basename.c
> index 0f1b0b0..0a2ed25 100644
> --- a/compat/basename.c
> +++ b/compat/basename.c
> @@ -1,4 +1,5 @@
>  #include "../git-compat-util.h"
> +#include "../strbuf.h"
>  
>  /* Adapted from libiberty's basename.c.  */
>  char *gitbasename (char *path)
> @@ -25,3 +26,46 @@ char *gitbasename (char *path)
>  	}
>  	return (char *)base;
>  }
> +
> +char *gitdirname(char *path)
> +{
> +	char *p = path, *slash = NULL, c;
> +	int dos_drive_prefix;
> +
> +	if (!p)
> +		return ".";
> +
> +	if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p) {
> +		static struct strbuf buf = STRBUF_INIT;
> +
> +dot:
> +		strbuf_reset(&buf);
> +		strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);

OK, so "Z:" becomes "Z:." (I missed the final '.' in my first reading),
which sounds sensible.

> +		return buf.buf;
> +	}
> +
> +	/*
> +	 * POSIX.1-2001 says dirname("/") should return "/", and dirname("//")
> +	 * should return "//", but dirname("///") should return "/" again.
> +	 */
> +	if (is_dir_sep(*p)) {
> +		if (!p[1] || (is_dir_sep(p[1]) && !p[2]))
> +			return path;
> +		slash = ++p;
> +	}
> +	while ((c = *(p++)))
> +		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;
> +		}

OK, so we find the first slash in a run of slashes, unless that run
of slashes is at the very end of the path.  Either slash is left NUL
(when there is no dir-sep) in which case we want to say "current
directory", or that slash is at the end of the directory name we
want to return in which case we just stuff NUL and return the path.

Sounds sensible.

> +
> +	if (!slash)
> +		goto dot;

;-) this is tricky.  I wondered what the value of dos_drive_prefix
at this point in my first reading, but this is correct.  We must
have done the skip_dos_drive_prefix() thing when we came into the
function already, so it is either 2 (when the original had Z: in
front) or 0 (for all other cases).

OK.

> +	*slash = '\0';
> +	return path;
> +}
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 0d66f3a..94f311a 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -253,6 +253,8 @@ struct itimerval {
>  #else
>  #define basename gitbasename
>  extern char *gitbasename(char *);
> +#define dirname gitdirname
> +extern char *gitdirname(char *);
>  #endif
>  
>  #ifndef NO_ICONV

[PATCH v2 4/4] t0060: verify that basename() and dirname() work as expected

[Johannes Schindelin]

Unfortunately, some libgen implementations yield outcomes different from
what Git expects. For example, mingw-w64-crt provides a basename()
function, that shortens `path0/` to `path`!

So let's verify that the basename() and dirname() functions we use conform
to what Git expects.

Derived-from-code-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0060-path-utils.sh |   3 +
 test-path-utils.c     | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 171 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 627ef85..f0152a7 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -59,6 +59,9 @@ case $(uname -s) in
 	;;
 esac
 
+test_expect_success basename 'test-path-utils basename'
+test_expect_success dirname 'test-path-utils dirname'
+
 norm_path "" ""
 norm_path . ""
 norm_path ./ ""
diff --git a/test-path-utils.c b/test-path-utils.c
index c67bf65..74e74c9 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -39,6 +39,168 @@ static void normalize_argv_string(const char **var, const char *input)
 		die("Bad value: %s\n", input);
 }
 
+struct test_data {
+	char *from;  /* input:  transform from this ... */
+	char *to;    /* output: ... to this.            */
+};
+
+static int test_function(struct test_data *data, char *(*func)(char *input),
+	const char *funcname)
+{
+	int failed = 0, i;
+	static char buffer[1024];
+	char *to;
+
+	for (i = 0; data[i].to; i++) {
+		if (!data[i].from)
+			to = func(NULL);
+		else {
+			strcpy(buffer, data[i].from);
+			to = func(buffer);
+		}
+		if (strcmp(to, data[i].to)) {
+			error("FAIL: %s(%s) => '%s' != '%s'\n",
+				funcname, data[i].from, to, data[i].to);
+			failed++;
+		}
+	}
+	return !!failed;
+}
+
+static struct test_data basename_data[] = {
+	/* --- POSIX type paths --- */
+	{ NULL,              "."    },
+	{ "",                "."    },
+	{ ".",               "."    },
+	{ "..",              ".."   },
+	{ "/",               "/"    },
+#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
+	{ "//",              "//"   },
+	{ "///",             "//"   },
+	{ "////",            "//"   },
+#else
+	{ "//",              "/"    },
+	{ "///",             "/"    },
+	{ "////",            "/"    },
+#endif
+	{ "usr",             "usr"  },
+	{ "/usr",            "usr"  },
+	{ "/usr/",           "usr"  },
+	{ "/usr//",          "usr"  },
+	{ "/usr/lib",        "lib"  },
+	{ "usr/lib",         "lib"  },
+	{ "usr/lib///",      "lib"  },
+
+#if defined(__MINGW32__) || defined(_MSC_VER)
+
+	/* --- win32 type paths --- */
+	{ "\\usr",           "usr"  },
+	{ "\\usr\\",         "usr"  },
+	{ "\\usr\\\\",       "usr"  },
+	{ "\\usr\\lib",      "lib"  },
+	{ "usr\\lib",        "lib"  },
+	{ "usr\\lib\\\\\\",  "lib"  },
+	{ "C:/usr",          "usr"  },
+	{ "C:/usr",          "usr"  },
+	{ "C:/usr/",         "usr"  },
+	{ "C:/usr//",        "usr"  },
+	{ "C:/usr/lib",      "lib"  },
+	{ "C:usr/lib",       "lib"  },
+	{ "C:usr/lib///",    "lib"  },
+	{ "C:",              "."    },
+	{ "C:a",             "a"    },
+	{ "C:/",             "/"    },
+	{ "C:///",           "/"    },
+#if defined(NO_LIBGEN_H)
+	{ "\\",              "\\"   },
+	{ "\\\\",            "\\"   },
+	{ "\\\\\\",          "\\"   },
+#else
+
+	/* win32 platform variations: */
+#if defined(__MINGW32__)
+	{ "\\",              "/"    },
+	{ "\\\\",            "/"    },
+	{ "\\\\\\",          "/"    },
+#endif
+
+#if defined(_MSC_VER)
+	{ "\\",              "\\"   },
+	{ "\\\\",            "\\"   },
+	{ "\\\\\\",          "\\"   },
+#endif
+
+#endif
+#endif
+	{ NULL,              "."    },
+	{ NULL,              NULL   }
+};
+
+static struct test_data dirname_data[] = {
+	/* --- POSIX type paths --- */
+	{ NULL,              "."      },
+	{ "",                "."      },
+	{ ".",               "."      },
+	{ "..",              "."      },
+	{ "/",               "/"      },
+	{ "//",              "//"     },
+#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
+	{ "///",             "//"     },
+	{ "////",            "//"     },
+#else
+	{ "///",             "/"      },
+	{ "////",            "/"      },
+#endif
+	{ "usr",             "."      },
+	{ "/usr",            "/"      },
+	{ "/usr/",           "/"      },
+	{ "/usr//",          "/"      },
+	{ "/usr/lib",        "/usr"   },
+	{ "usr/lib",         "usr"    },
+	{ "usr/lib///",      "usr"    },
+
+#if defined(__MINGW32__) || defined(_MSC_VER)
+
+	/* --- win32 type paths --- */
+	{ "\\",              "\\"     },
+	{ "\\\\",            "\\\\"   },
+	{ "\\usr",           "\\"     },
+	{ "\\usr\\",         "\\"     },
+	{ "\\usr\\\\",       "\\"     },
+	{ "\\usr\\lib",      "\\usr"  },
+	{ "usr\\lib",        "usr"    },
+	{ "usr\\lib\\\\\\",  "usr"    },
+	{ "C:a",             "C:."    },
+	{ "C:/",             "C:/"    },
+	{ "C:///",           "C:/"    },
+	{ "C:/usr",          "C:/"    },
+	{ "C:/usr/",         "C:/"    },
+	{ "C:/usr//",        "C:/"    },
+	{ "C:/usr/lib",      "C:/usr" },
+	{ "C:usr/lib",       "C:usr"  },
+	{ "C:usr/lib///",    "C:usr"  },
+	{ "\\\\\\",          "\\"     },
+	{ "\\\\\\\\",        "\\"     },
+#if defined(NO_LIBGEN_H)
+	{ "C:",              "C:."    },
+#else
+
+	/* win32 platform variations: */
+#if defined(__MINGW32__)
+	/* the following is clearly wrong ... */
+	{ "C:",              "."      },
+#endif
+
+#if defined(_MSC_VER)
+	{ "C:",              "C:."    },
+#endif
+
+#endif
+#endif
+	{ NULL,              "."      },
+	{ NULL,              NULL     }
+};
+
 int main(int argc, char **argv)
 {
 	if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
@@ -133,6 +295,12 @@ int main(int argc, char **argv)
 		return 0;
 	}
 
+	if (argc == 2 && !strcmp(argv[1], "basename"))
+		return test_function(basename_data, basename, argv[1]);
+
+	if (argc == 2 && !strcmp(argv[1], "dirname"))
+		return test_function(dirname_data, dirname, argv[1]);
+
 	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
 		argv[1] ? argv[1] : "(there was none)");
 	return 1;
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v2 4/4] t0060: verify that basename() and dirname() work as expected

[Eric Sunshine]

On Fri, Jan 8, 2016 at 11:21 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Unfortunately, some libgen implementations yield outcomes different from
> what Git expects. For example, mingw-w64-crt provides a basename()
> function, that shortens `path0/` to `path`!
>
> So let's verify that the basename() and dirname() functions we use conform
> to what Git expects.
>
> Derived-from-code-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/test-path-utils.c b/test-path-utils.c
> @@ -39,6 +39,168 @@ static void normalize_argv_string(const char **var, const char *input)
> +struct test_data {
> +       char *from;  /* input:  transform from this ... */
> +       char *to;    /* output: ... to this.            */

Can these be 'const'? If I'm reading the code correctly, I don't think
these values ever get passed directly to functions expecting non-const
strings.

> +};
> +
> +static int test_function(struct test_data *data, char *(*func)(char *input),
> +       const char *funcname)
> +{
> +       int failed = 0, i;
> +       static char buffer[1024];

Why is this 'static'? It is never accessed outside of this scope.

> +       char *to;
> +
> +       for (i = 0; data[i].to; i++) {
> +               if (!data[i].from)
> +                       to = func(NULL);
> +               else {
> +                       strcpy(buffer, data[i].from);
> +                       to = func(buffer);
> +               }
> +               if (strcmp(to, data[i].to)) {
> +                       error("FAIL: %s(%s) => '%s' != '%s'\n",
> +                               funcname, data[i].from, to, data[i].to);
> +                       failed++;

Since 'failed' is only ever used as a boolean, it might be clearer to say:

    failed = 1;

> +               }
> +       }
> +       return !!failed;

And then simply:

    return failed;

> +}
> +
> +static struct test_data basename_data[] = {
> +       /* --- POSIX type paths --- */
> +       { NULL,              "."    },

NULL is tested here.

> +       { "",                "."    },
> +       { ".",               "."    },
> [...]
> +#endif
> +       { NULL,              "."    },

And also here. Is that intentional?

> +       { NULL,              NULL   }
> +};
> +
> +static struct test_data dirname_data[] = {
> +       /* --- POSIX type paths --- */
> +       { NULL,              "."      },
> [...]
> +#endif
> +       { NULL,              "."      },

Ditto.

> +       { NULL,              NULL     }
> +};

Re: [PATCH v2 4/4] t0060: verify that basename() and dirname() work as expected

[Johannes Schindelin]

Hi Eric,

On Sat, 9 Jan 2016, Eric Sunshine wrote:

> On Fri, Jan 8, 2016 at 11:21 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > Unfortunately, some libgen implementations yield outcomes different from
> > what Git expects. For example, mingw-w64-crt provides a basename()
> > function, that shortens `path0/` to `path`!
> >
> > So let's verify that the basename() and dirname() functions we use conform
> > to what Git expects.
> >
> > Derived-from-code-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/test-path-utils.c b/test-path-utils.c
> > @@ -39,6 +39,168 @@ static void normalize_argv_string(const char **var, const char *input)
> > +struct test_data {
> > +       char *from;  /* input:  transform from this ... */
> > +       char *to;    /* output: ... to this.            */
> 
> Can these be 'const'? If I'm reading the code correctly, I don't think
> these values ever get passed directly to functions expecting non-const
> strings.

This, and ...

> > +};
> > +
> > +static int test_function(struct test_data *data, char *(*func)(char *input),
> > +       const char *funcname)
> > +{
> > +       int failed = 0, i;
> > +       static char buffer[1024];
> 
> Why is this 'static'? It is never accessed outside of this scope.

... this, and ...

> > +       char *to;
> > +
> > +       for (i = 0; data[i].to; i++) {
> > +               if (!data[i].from)
> > +                       to = func(NULL);
> > +               else {
> > +                       strcpy(buffer, data[i].from);
> > +                       to = func(buffer);
> > +               }
> > +               if (strcmp(to, data[i].to)) {
> > +                       error("FAIL: %s(%s) => '%s' != '%s'\n",
> > +                               funcname, data[i].from, to, data[i].to);
> > +                       failed++;
> 
> Since 'failed' is only ever used as a boolean, it might be clearer to say:
> 
>     failed = 1;

... this and ...

> > +               }
> > +       }
> > +       return !!failed;
> 
> And then simply:
> 
>     return failed;
> 
> > +}
> > +
> > +static struct test_data basename_data[] = {
> > +       /* --- POSIX type paths --- */
> > +       { NULL,              "."    },
> 
> NULL is tested here.
> 
> > +       { "",                "."    },
> > +       { ".",               "."    },
> > [...]
> > +#endif
> > +       { NULL,              "."    },
> 
> And also here. Is that intentional?

... this are all valid concerns that I now addressed locally, so they will
be fixed in the next iteration.

Thanks,
Dscho

Re: [PATCH v2 4/4] t0060: verify that basename() and dirname() work as expected

[Michael Blume]

On Fri, Jan 8, 2016 at 8:21 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Unfortunately, some libgen implementations yield outcomes different from
> what Git expects. For example, mingw-w64-crt provides a basename()
> function, that shortens `path0/` to `path`!
>
> So let's verify that the basename() and dirname() functions we use conform
> to what Git expects.
>
> Derived-from-code-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t0060-path-utils.sh |   3 +
>  test-path-utils.c     | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 171 insertions(+)
>
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index 627ef85..f0152a7 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -59,6 +59,9 @@ case $(uname -s) in
>         ;;
>  esac
>
> +test_expect_success basename 'test-path-utils basename'
> +test_expect_success dirname 'test-path-utils dirname'
> +
>  norm_path "" ""
>  norm_path . ""
>  norm_path ./ ""
> diff --git a/test-path-utils.c b/test-path-utils.c
> index c67bf65..74e74c9 100644
> --- a/test-path-utils.c
> +++ b/test-path-utils.c
> @@ -39,6 +39,168 @@ static void normalize_argv_string(const char **var, const char *input)
>                 die("Bad value: %s\n", input);
>  }
>
> +struct test_data {
> +       char *from;  /* input:  transform from this ... */
> +       char *to;    /* output: ... to this.            */
> +};
> +
> +static int test_function(struct test_data *data, char *(*func)(char *input),
> +       const char *funcname)
> +{
> +       int failed = 0, i;
> +       static char buffer[1024];
> +       char *to;
> +
> +       for (i = 0; data[i].to; i++) {
> +               if (!data[i].from)
> +                       to = func(NULL);
> +               else {
> +                       strcpy(buffer, data[i].from);
> +                       to = func(buffer);
> +               }
> +               if (strcmp(to, data[i].to)) {
> +                       error("FAIL: %s(%s) => '%s' != '%s'\n",
> +                               funcname, data[i].from, to, data[i].to);
> +                       failed++;
> +               }
> +       }
> +       return !!failed;
> +}
> +
> +static struct test_data basename_data[] = {
> +       /* --- POSIX type paths --- */
> +       { NULL,              "."    },
> +       { "",                "."    },
> +       { ".",               "."    },
> +       { "..",              ".."   },
> +       { "/",               "/"    },
> +#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
> +       { "//",              "//"   },
> +       { "///",             "//"   },
> +       { "////",            "//"   },
> +#else
> +       { "//",              "/"    },
> +       { "///",             "/"    },
> +       { "////",            "/"    },
> +#endif
> +       { "usr",             "usr"  },
> +       { "/usr",            "usr"  },
> +       { "/usr/",           "usr"  },
> +       { "/usr//",          "usr"  },
> +       { "/usr/lib",        "lib"  },
> +       { "usr/lib",         "lib"  },
> +       { "usr/lib///",      "lib"  },
> +
> +#if defined(__MINGW32__) || defined(_MSC_VER)
> +
> +       /* --- win32 type paths --- */
> +       { "\\usr",           "usr"  },
> +       { "\\usr\\",         "usr"  },
> +       { "\\usr\\\\",       "usr"  },
> +       { "\\usr\\lib",      "lib"  },
> +       { "usr\\lib",        "lib"  },
> +       { "usr\\lib\\\\\\",  "lib"  },
> +       { "C:/usr",          "usr"  },
> +       { "C:/usr",          "usr"  },
> +       { "C:/usr/",         "usr"  },
> +       { "C:/usr//",        "usr"  },
> +       { "C:/usr/lib",      "lib"  },
> +       { "C:usr/lib",       "lib"  },
> +       { "C:usr/lib///",    "lib"  },
> +       { "C:",              "."    },
> +       { "C:a",             "a"    },
> +       { "C:/",             "/"    },
> +       { "C:///",           "/"    },
> +#if defined(NO_LIBGEN_H)
> +       { "\\",              "\\"   },
> +       { "\\\\",            "\\"   },
> +       { "\\\\\\",          "\\"   },
> +#else
> +
> +       /* win32 platform variations: */
> +#if defined(__MINGW32__)
> +       { "\\",              "/"    },
> +       { "\\\\",            "/"    },
> +       { "\\\\\\",          "/"    },
> +#endif
> +
> +#if defined(_MSC_VER)
> +       { "\\",              "\\"   },
> +       { "\\\\",            "\\"   },
> +       { "\\\\\\",          "\\"   },
> +#endif
> +
> +#endif
> +#endif
> +       { NULL,              "."    },
> +       { NULL,              NULL   }
> +};
> +
> +static struct test_data dirname_data[] = {
> +       /* --- POSIX type paths --- */
> +       { NULL,              "."      },
> +       { "",                "."      },
> +       { ".",               "."      },
> +       { "..",              "."      },
> +       { "/",               "/"      },
> +       { "//",              "//"     },
> +#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
> +       { "///",             "//"     },
> +       { "////",            "//"     },
> +#else
> +       { "///",             "/"      },
> +       { "////",            "/"      },
> +#endif
> +       { "usr",             "."      },
> +       { "/usr",            "/"      },
> +       { "/usr/",           "/"      },
> +       { "/usr//",          "/"      },
> +       { "/usr/lib",        "/usr"   },
> +       { "usr/lib",         "usr"    },
> +       { "usr/lib///",      "usr"    },
> +
> +#if defined(__MINGW32__) || defined(_MSC_VER)
> +
> +       /* --- win32 type paths --- */
> +       { "\\",              "\\"     },
> +       { "\\\\",            "\\\\"   },
> +       { "\\usr",           "\\"     },
> +       { "\\usr\\",         "\\"     },
> +       { "\\usr\\\\",       "\\"     },
> +       { "\\usr\\lib",      "\\usr"  },
> +       { "usr\\lib",        "usr"    },
> +       { "usr\\lib\\\\\\",  "usr"    },
> +       { "C:a",             "C:."    },
> +       { "C:/",             "C:/"    },
> +       { "C:///",           "C:/"    },
> +       { "C:/usr",          "C:/"    },
> +       { "C:/usr/",         "C:/"    },
> +       { "C:/usr//",        "C:/"    },
> +       { "C:/usr/lib",      "C:/usr" },
> +       { "C:usr/lib",       "C:usr"  },
> +       { "C:usr/lib///",    "C:usr"  },
> +       { "\\\\\\",          "\\"     },
> +       { "\\\\\\\\",        "\\"     },
> +#if defined(NO_LIBGEN_H)
> +       { "C:",              "C:."    },
> +#else
> +
> +       /* win32 platform variations: */
> +#if defined(__MINGW32__)
> +       /* the following is clearly wrong ... */
> +       { "C:",              "."      },
> +#endif
> +
> +#if defined(_MSC_VER)
> +       { "C:",              "C:."    },
> +#endif
> +
> +#endif
> +#endif
> +       { NULL,              "."      },
> +       { NULL,              NULL     }
> +};
> +
>  int main(int argc, char **argv)
>  {
>         if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
> @@ -133,6 +295,12 @@ int main(int argc, char **argv)
>                 return 0;
>         }
>
> +       if (argc == 2 && !strcmp(argv[1], "basename"))
> +               return test_function(basename_data, basename, argv[1]);
> +
> +       if (argc == 2 && !strcmp(argv[1], "dirname"))
> +               return test_function(dirname_data, dirname, argv[1]);
> +
>         fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
>                 argv[1] ? argv[1] : "(there was none)");
>         return 1;
> --
> 2.6.3.windows.1.300.g1c25e49
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Test fails on my Mac:

expecting success: test-path-utils dirname
error: FAIL: dirname(//) => '/' != '//'

not ok 2 - dirname
#    test-path-utils dirname

Re: [PATCH v2 4/4] t0060: verify that basename() and dirname() work as expected

[Junio C Hamano]

Michael Blume <blume.mike@gmail.com> writes:

> Test fails on my Mac:
>
> expecting success: test-path-utils dirname
> error: FAIL: dirname(//) => '/' != '//'
>
> not ok 2 - dirname
> #    test-path-utils dirname

Thanks for reporting.

Ramsay gave a nice analysis at

  http://thread.gmane.org/gmane.comp.version-control.git/283928

and Dscho already is working on it, IIUC.

Thanks.

Re: [PATCH v2 4/4] t0060: verify that basename() and dirname() work as expected

[Johannes Schindelin]

Hi,

[thanks, Junio, for culling the quoted text to the essential part]

On Wed, 13 Jan 2016, Junio C Hamano wrote:

> Michael Blume <blume.mike@gmail.com> writes:
> 
> > Test fails on my Mac:
> >
> > expecting success: test-path-utils dirname
> > error: FAIL: dirname(//) => '/' != '//'
> >
> > not ok 2 - dirname
> > #    test-path-utils dirname
> 
> Thanks for reporting.
> 
> Ramsay gave a nice analysis at
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/283928
> 
> and Dscho already is working on it, IIUC.

I already provided a hot fix:
http://article.gmane.org/gmane.comp.version-control.git/283893

And yes, I also work on a proper fix.

Ciao,
Dscho

[PATCH v3 0/4] Ensure that we can build without libgen.h

[Johannes Schindelin]

This mini series adds a fall-back for the `dirname()` function that we use
e.g. in git-am. This is necessary because not all platforms have a working
libgen.h.

While at it, we ensure that our basename() drop-in conforms to the POSIX
specifications.

In addition to the interdiff vs v2, the commit message was fixed to
mention basename("") as cornercase (not basename("abc")).


Johannes Schindelin (4):
  Refactor skipping DOS drive prefixes
  compat/basename: make basename() conform to POSIX
  Provide a dirname() function when NO_LIBGEN_H=YesPlease
  t0060: verify that basename() and dirname() work as expected

 compat/basename.c     |  66 ++++++++++++++++++--
 compat/mingw.c        |  14 ++---
 compat/mingw.h        |  10 ++-
 git-compat-util.h     |  10 +++
 path.c                |  14 ++---
 t/t0060-path-utils.sh |   3 +
 test-path-utils.c     | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 259 insertions(+), 24 deletions(-)

Interdiff vs v2:

 diff --git a/test-path-utils.c b/test-path-utils.c
 index 74e74c9..4ab68ac 100644
 --- a/test-path-utils.c
 +++ b/test-path-utils.c
 @@ -40,15 +40,15 @@ static void normalize_argv_string(const char **var, const char *input)
  }
  
  struct test_data {
 -	char *from;  /* input:  transform from this ... */
 -	char *to;    /* output: ... to this.            */
 +	const char *from;  /* input:  transform from this ... */
 +	const char *to;    /* output: ... to this.            */
  };
  
  static int test_function(struct test_data *data, char *(*func)(char *input),
  	const char *funcname)
  {
  	int failed = 0, i;
 -	static char buffer[1024];
 +	char buffer[1024];
  	char *to;
  
  	for (i = 0; data[i].to; i++) {
 @@ -61,10 +61,10 @@ static int test_function(struct test_data *data, char *(*func)(char *input),
  		if (strcmp(to, data[i].to)) {
  			error("FAIL: %s(%s) => '%s' != '%s'\n",
  				funcname, data[i].from, to, data[i].to);
 -			failed++;
 +			failed = 1;
  		}
  	}
 -	return !!failed;
 +	return failed;
  }
  
  static struct test_data basename_data[] = {
 @@ -132,7 +132,6 @@ static struct test_data basename_data[] = {
  
  #endif
  #endif
 -	{ NULL,              "."    },
  	{ NULL,              NULL   }
  };
  
 @@ -197,7 +196,6 @@ static struct test_data dirname_data[] = {
  
  #endif
  #endif
 -	{ NULL,              "."      },
  	{ NULL,              NULL     }
  };
  

-- 
2.6.3.windows.1.300.g1c25e49

[PATCH v3 1/4] Refactor skipping DOS drive prefixes

[Johannes Schindelin]

Junio Hamano pointed out that there is an implicit assumption in pretty
much all the code calling has_dos_drive_prefix(): it assumes that the
DOS drive prefix is always two bytes long.

While this assumption is pretty safe, we can still make the code more
readable and less error-prone by introducing a function that skips the
DOS drive prefix safely.

While at it, we change the has_dos_drive_prefix() return value: it now
returns the number of bytes to be skipped if there is a DOS drive prefix.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c |  4 +---
 compat/mingw.c    | 14 +++++---------
 compat/mingw.h    | 10 +++++++++-
 git-compat-util.h |  8 ++++++++
 path.c            | 14 +++++---------
 5 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..9f00421 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -4,9 +4,7 @@
 char *gitbasename (char *path)
 {
 	const char *base;
-	/* Skip over the disk name in MSDOS pathnames. */
-	if (has_dos_drive_prefix(path))
-		path += 2;
+	skip_dos_drive_prefix(&path);
 	for (base = path; *path; path++) {
 		if (is_dir_sep(*path))
 			base = path + 1;
diff --git a/compat/mingw.c b/compat/mingw.c
index 5edea29..1b3530a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1934,26 +1934,22 @@ pid_t waitpid(pid_t pid, int *status, int options)
 
 int mingw_offset_1st_component(const char *path)
 {
-	int offset = 0;
-	if (has_dos_drive_prefix(path))
-		offset = 2;
+	char *pos = (char *)path;
 
 	/* unc paths */
-	else if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
-
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
 		/* skip server name */
-		char *pos = strpbrk(path + 2, "\\/");
+		pos = strpbrk(pos + 2, "\\/");
 		if (!pos)
 			return 0; /* Error: malformed unc path */
 
 		do {
 			pos++;
 		} while (*pos && !is_dir_sep(*pos));
-
-		offset = pos - path;
 	}
 
-	return offset + is_dir_sep(path[offset]);
+	return pos + is_dir_sep(*pos) - path;
 }
 
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
diff --git a/compat/mingw.h b/compat/mingw.h
index 57ca477..b3e5044 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -361,7 +361,15 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+static inline int mingw_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 2da0a75..0d66f3a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -335,6 +335,14 @@ static inline int git_has_dos_drive_prefix(const char *path)
 #define has_dos_drive_prefix git_has_dos_drive_prefix
 #endif
 
+#ifndef skip_dos_drive_prefix
+static inline int git_skip_dos_drive_prefix(const char **path)
+{
+	return 0;
+}
+#define skip_dos_drive_prefix git_skip_dos_drive_prefix
+#endif
+
 #ifndef is_dir_sep
 static inline int git_is_dir_sep(int c)
 {
diff --git a/path.c b/path.c
index 3cd155e..8b7e168 100644
--- a/path.c
+++ b/path.c
@@ -782,13 +782,10 @@ const char *relative_path(const char *in, const char *prefix,
 	else if (!prefix_len)
 		return in;
 
-	if (have_same_root(in, prefix)) {
+	if (have_same_root(in, prefix))
 		/* bypass dos_drive, for "c:" is identical to "C:" */
-		if (has_dos_drive_prefix(in)) {
-			i = 2;
-			j = 2;
-		}
-	} else {
+		i = j = has_dos_drive_prefix(in);
+	else {
 		return in;
 	}
 
@@ -943,11 +940,10 @@ const char *remove_leading_path(const char *in, const char *prefix)
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 {
 	char *dst0;
+	int i;
 
-	if (has_dos_drive_prefix(src)) {
+	for (i = has_dos_drive_prefix(src); i > 0; i--)
 		*dst++ = *src++;
-		*dst++ = *src++;
-	}
 	dst0 = dst;
 
 	if (is_dir_sep(*src)) {
-- 
2.6.3.windows.1.300.g1c25e49

[PATCH v3 2/4] compat/basename: make basename() conform to POSIX

[Johannes Schindelin]

According to POSIX, basename("/path/") should return "path", not
"path/". Likewise, basename(NULL) and basename("") should both
return "." to conform.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

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;
 }
-- 
2.6.3.windows.1.300.g1c25e49

[PATCH v3 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Johannes Schindelin]

When there is no `libgen.h` to our disposal, we miss the `dirname()`
function.

So far, we only had one user of that function: credential-cache--daemon
(which was only compiled when Unix sockets are available, anyway). But
now we also have `builtin/am.c` as user, so we need it.

Since `dirname()` is a sibling of `basename()`, we simply put our very
own `gitdirname()` implementation next to `gitbasename()` and use it
if `NO_LIBGEN_H` has been set.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/compat/basename.c b/compat/basename.c
index 0f1b0b0..0a2ed25 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -1,4 +1,5 @@
 #include "../git-compat-util.h"
+#include "../strbuf.h"
 
 /* Adapted from libiberty's basename.c.  */
 char *gitbasename (char *path)
@@ -25,3 +26,46 @@ char *gitbasename (char *path)
 	}
 	return (char *)base;
 }
+
+char *gitdirname(char *path)
+{
+	char *p = path, *slash = NULL, c;
+	int dos_drive_prefix;
+
+	if (!p)
+		return ".";
+
+	if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p) {
+		static struct strbuf buf = STRBUF_INIT;
+
+dot:
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
+		return buf.buf;
+	}
+
+	/*
+	 * POSIX.1-2001 says dirname("/") should return "/", and dirname("//")
+	 * should return "//", but dirname("///") should return "/" again.
+	 */
+	if (is_dir_sep(*p)) {
+		if (!p[1] || (is_dir_sep(p[1]) && !p[2]))
+			return path;
+		slash = ++p;
+	}
+	while ((c = *(p++)))
+		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;
+		}
+
+	if (!slash)
+		goto dot;
+	*slash = '\0';
+	return path;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 0d66f3a..94f311a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -253,6 +253,8 @@ struct itimerval {
 #else
 #define basename gitbasename
 extern char *gitbasename(char *);
+#define dirname gitdirname
+extern char *gitdirname(char *);
 #endif
 
 #ifndef NO_ICONV
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v3 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Eric Sunshine]

On Mon, Jan 11, 2016 at 1:30 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When there is no `libgen.h` to our disposal, we miss the `dirname()`
> function.
>
> So far, we only had one user of that function: credential-cache--daemon
> (which was only compiled when Unix sockets are available, anyway). But
> now we also have `builtin/am.c` as user, so we need it.
>
> Since `dirname()` is a sibling of `basename()`, we simply put our very
> own `gitdirname()` implementation next to `gitbasename()` and use it
> if `NO_LIBGEN_H` has been set.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/compat/basename.c b/compat/basename.c
> @@ -25,3 +26,46 @@ char *gitbasename (char *path)
> +char *gitdirname(char *path)
> +{
> +       char *p = path, *slash = NULL, c;
> +       int dos_drive_prefix;
> +
> +       if (!p)
> +               return ".";
> +
> +       if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p) {
> +               static struct strbuf buf = STRBUF_INIT;
> +
> +dot:
> +               strbuf_reset(&buf);
> +               strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
> +               return buf.buf;
> +       }
> +
> +       /*
> +        * POSIX.1-2001 says dirname("/") should return "/", and dirname("//")
> +        * should return "//", but dirname("///") should return "/" again.
> +        */
> +       if (is_dir_sep(*p)) {
> +               if (!p[1] || (is_dir_sep(p[1]) && !p[2]))
> +                       return path;
> +               slash = ++p;
> +       }
> +       while ((c = *(p++)))
> +               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;
> +               }
> +
> +       if (!slash)
> +               goto dot;
> +       *slash = '\0';
> +       return path;
> +}

I wonder if this would be a bit easier to follow if it was structured
something like this:

    static struct strbuf buf = STRBUF_INIT;

    if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p)
        goto dot;

    ...
    if (is_dir_sep(*p)) {
        ...
    }
    ...
    while ((c = *(p++)))
        ...

    if (slash) {
        *slash = '\0';
        return path;
    }

    dot:
    strbuf_reset(&buf);
    strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
    return buf.buf;

Re: [PATCH v3 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I wonder if this would be a bit easier to follow if it was structured
> something like this:
>
>     static struct strbuf buf = STRBUF_INIT;
>
>     if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p)
>         goto dot;
>
>     ...
>     if (is_dir_sep(*p)) {
>         ...
>     }
>     ...
>     while ((c = *(p++)))
>         ...
>
>     if (slash) {
>         *slash = '\0';
>         return path;
>     }
>
>     dot:
>     strbuf_reset(&buf);
>     strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
>     return buf.buf;

I'll queue the one from Dscho as-is for today, but avoiding the
"jump back to a place where it happens to have an identical clean-up
that need to happen" and defining the clean-up path at the end like
this would probably be easier to follow.  It certainly would have
saved one comment in the previous review cycle from me.

Thanks.

Re: [PATCH v3 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Johannes Schindelin]

Hi Eric & Junio,

On Mon, 11 Jan 2016, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > I wonder if this would be a bit easier to follow if it was structured
> > something like this:
> >
> >     static struct strbuf buf = STRBUF_INIT;
> >
> >     if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p)
> >         goto dot;
> >
> >     ...
> >     if (is_dir_sep(*p)) {
> >         ...
> >     }
> >     ...
> >     while ((c = *(p++)))
> >         ...
> >
> >     if (slash) {
> >         *slash = '\0';
> >         return path;
> >     }
> >
> >     dot:
> >     strbuf_reset(&buf);
> >     strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
> >     return buf.buf;
> 
> I'll queue the one from Dscho as-is for today, but avoiding the
> "jump back to a place where it happens to have an identical clean-up
> that need to happen" and defining the clean-up path at the end like
> this would probably be easier to follow.  It certainly would have
> saved one comment in the previous review cycle from me.

Thanks! I changed it and will mail out v4 shortly.

Ciao,
Dscho

[PATCH v3 4/4] t0060: verify that basename() and dirname() work as expected

[Johannes Schindelin]

Unfortunately, some libgen implementations yield outcomes different
from what Git expects. For example, mingw-w64-crt provides a basename()
function, that shortens `path0/` to `path`!

So let's verify that the basename() and dirname() functions we use
conform to what Git expects.

Derived-from-code-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0060-path-utils.sh |   3 +
 test-path-utils.c     | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 627ef85..f0152a7 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -59,6 +59,9 @@ case $(uname -s) in
 	;;
 esac
 
+test_expect_success basename 'test-path-utils basename'
+test_expect_success dirname 'test-path-utils dirname'
+
 norm_path "" ""
 norm_path . ""
 norm_path ./ ""
diff --git a/test-path-utils.c b/test-path-utils.c
index c67bf65..4ab68ac 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -39,6 +39,166 @@ static void normalize_argv_string(const char **var, const char *input)
 		die("Bad value: %s\n", input);
 }
 
+struct test_data {
+	const char *from;  /* input:  transform from this ... */
+	const char *to;    /* output: ... to this.            */
+};
+
+static int test_function(struct test_data *data, char *(*func)(char *input),
+	const char *funcname)
+{
+	int failed = 0, i;
+	char buffer[1024];
+	char *to;
+
+	for (i = 0; data[i].to; i++) {
+		if (!data[i].from)
+			to = func(NULL);
+		else {
+			strcpy(buffer, data[i].from);
+			to = func(buffer);
+		}
+		if (strcmp(to, data[i].to)) {
+			error("FAIL: %s(%s) => '%s' != '%s'\n",
+				funcname, data[i].from, to, data[i].to);
+			failed = 1;
+		}
+	}
+	return failed;
+}
+
+static struct test_data basename_data[] = {
+	/* --- POSIX type paths --- */
+	{ NULL,              "."    },
+	{ "",                "."    },
+	{ ".",               "."    },
+	{ "..",              ".."   },
+	{ "/",               "/"    },
+#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
+	{ "//",              "//"   },
+	{ "///",             "//"   },
+	{ "////",            "//"   },
+#else
+	{ "//",              "/"    },
+	{ "///",             "/"    },
+	{ "////",            "/"    },
+#endif
+	{ "usr",             "usr"  },
+	{ "/usr",            "usr"  },
+	{ "/usr/",           "usr"  },
+	{ "/usr//",          "usr"  },
+	{ "/usr/lib",        "lib"  },
+	{ "usr/lib",         "lib"  },
+	{ "usr/lib///",      "lib"  },
+
+#if defined(__MINGW32__) || defined(_MSC_VER)
+
+	/* --- win32 type paths --- */
+	{ "\\usr",           "usr"  },
+	{ "\\usr\\",         "usr"  },
+	{ "\\usr\\\\",       "usr"  },
+	{ "\\usr\\lib",      "lib"  },
+	{ "usr\\lib",        "lib"  },
+	{ "usr\\lib\\\\\\",  "lib"  },
+	{ "C:/usr",          "usr"  },
+	{ "C:/usr",          "usr"  },
+	{ "C:/usr/",         "usr"  },
+	{ "C:/usr//",        "usr"  },
+	{ "C:/usr/lib",      "lib"  },
+	{ "C:usr/lib",       "lib"  },
+	{ "C:usr/lib///",    "lib"  },
+	{ "C:",              "."    },
+	{ "C:a",             "a"    },
+	{ "C:/",             "/"    },
+	{ "C:///",           "/"    },
+#if defined(NO_LIBGEN_H)
+	{ "\\",              "\\"   },
+	{ "\\\\",            "\\"   },
+	{ "\\\\\\",          "\\"   },
+#else
+
+	/* win32 platform variations: */
+#if defined(__MINGW32__)
+	{ "\\",              "/"    },
+	{ "\\\\",            "/"    },
+	{ "\\\\\\",          "/"    },
+#endif
+
+#if defined(_MSC_VER)
+	{ "\\",              "\\"   },
+	{ "\\\\",            "\\"   },
+	{ "\\\\\\",          "\\"   },
+#endif
+
+#endif
+#endif
+	{ NULL,              NULL   }
+};
+
+static struct test_data dirname_data[] = {
+	/* --- POSIX type paths --- */
+	{ NULL,              "."      },
+	{ "",                "."      },
+	{ ".",               "."      },
+	{ "..",              "."      },
+	{ "/",               "/"      },
+	{ "//",              "//"     },
+#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
+	{ "///",             "//"     },
+	{ "////",            "//"     },
+#else
+	{ "///",             "/"      },
+	{ "////",            "/"      },
+#endif
+	{ "usr",             "."      },
+	{ "/usr",            "/"      },
+	{ "/usr/",           "/"      },
+	{ "/usr//",          "/"      },
+	{ "/usr/lib",        "/usr"   },
+	{ "usr/lib",         "usr"    },
+	{ "usr/lib///",      "usr"    },
+
+#if defined(__MINGW32__) || defined(_MSC_VER)
+
+	/* --- win32 type paths --- */
+	{ "\\",              "\\"     },
+	{ "\\\\",            "\\\\"   },
+	{ "\\usr",           "\\"     },
+	{ "\\usr\\",         "\\"     },
+	{ "\\usr\\\\",       "\\"     },
+	{ "\\usr\\lib",      "\\usr"  },
+	{ "usr\\lib",        "usr"    },
+	{ "usr\\lib\\\\\\",  "usr"    },
+	{ "C:a",             "C:."    },
+	{ "C:/",             "C:/"    },
+	{ "C:///",           "C:/"    },
+	{ "C:/usr",          "C:/"    },
+	{ "C:/usr/",         "C:/"    },
+	{ "C:/usr//",        "C:/"    },
+	{ "C:/usr/lib",      "C:/usr" },
+	{ "C:usr/lib",       "C:usr"  },
+	{ "C:usr/lib///",    "C:usr"  },
+	{ "\\\\\\",          "\\"     },
+	{ "\\\\\\\\",        "\\"     },
+#if defined(NO_LIBGEN_H)
+	{ "C:",              "C:."    },
+#else
+
+	/* win32 platform variations: */
+#if defined(__MINGW32__)
+	/* the following is clearly wrong ... */
+	{ "C:",              "."      },
+#endif
+
+#if defined(_MSC_VER)
+	{ "C:",              "C:."    },
+#endif
+
+#endif
+#endif
+	{ NULL,              NULL     }
+};
+
 int main(int argc, char **argv)
 {
 	if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
@@ -133,6 +293,12 @@ int main(int argc, char **argv)
 		return 0;
 	}
 
+	if (argc == 2 && !strcmp(argv[1], "basename"))
+		return test_function(basename_data, basename, argv[1]);
+
+	if (argc == 2 && !strcmp(argv[1], "dirname"))
+		return test_function(dirname_data, dirname, argv[1]);
+
 	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
 		argv[1] ? argv[1] : "(there was none)");
 	return 1;
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v3 0/4] Ensure that we can build without libgen.h

[Junio C Hamano]

Thanks.  Queued but I am OK if you agree it is better to replace 3/4
with Eric's.

[PATCH v4 0/4] Ensure that we can build without libgen.h

[Johannes Schindelin]

This mini series adds a fall-back for the `dirname()` function that we use
e.g. in git-am. This is necessary because not all platforms have a working
libgen.h.

While at it, we ensure that our basename() drop-in conforms to the POSIX
specifications.

In addition to Eric's style improvement, v4 also fixes the signature
of skip_dos_drive_prefix() in the non-Windows case.


Johannes Schindelin (4):
  Refactor skipping DOS drive prefixes
  compat/basename: make basename() conform to POSIX
  Provide a dirname() function when NO_LIBGEN_H=YesPlease
  t0060: verify that basename() and dirname() work as expected

 compat/basename.c     |  66 ++++++++++++++++++--
 compat/mingw.c        |  14 ++---
 compat/mingw.h        |  10 ++-
 git-compat-util.h     |  10 +++
 path.c                |  14 ++---
 t/t0060-path-utils.sh |   3 +
 test-path-utils.c     | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 259 insertions(+), 24 deletions(-)

Interdiff vs v3:

 diff --git a/compat/basename.c b/compat/basename.c
 index 0a2ed25..96bd953 100644
 --- a/compat/basename.c
 +++ b/compat/basename.c
 @@ -29,20 +29,15 @@ char *gitbasename (char *path)
  
  char *gitdirname(char *path)
  {
 +	static struct strbuf buf = STRBUF_INIT;
  	char *p = path, *slash = NULL, c;
  	int dos_drive_prefix;
  
  	if (!p)
  		return ".";
  
 -	if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p) {
 -		static struct strbuf buf = STRBUF_INIT;
 -
 -dot:
 -		strbuf_reset(&buf);
 -		strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
 -		return buf.buf;
 -	}
 +	if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p)
 +		goto dot;
  
  	/*
  	 * POSIX.1-2001 says dirname("/") should return "/", and dirname("//")
 @@ -64,8 +59,13 @@ dot:
  				slash = tentative;
  		}
  
 -	if (!slash)
 -		goto dot;
 -	*slash = '\0';
 -	return path;
 +	if (slash) {
 +		*slash = '\0';
 +		return path;
 +	}
 +
 +dot:
 +	strbuf_reset(&buf);
 +	strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
 +	return buf.buf;
  }
 diff --git a/git-compat-util.h b/git-compat-util.h
 index 94f311a..5f72f1c 100644
 --- a/git-compat-util.h
 +++ b/git-compat-util.h
 @@ -338,7 +338,7 @@ static inline int git_has_dos_drive_prefix(const char *path)
  #endif
  
  #ifndef skip_dos_drive_prefix
 -static inline int git_skip_dos_drive_prefix(const char **path)
 +static inline int git_skip_dos_drive_prefix(char **path)
  {
  	return 0;
  }

-- 
2.6.3.windows.1.300.g1c25e49

[PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Schindelin]

Junio Hamano pointed out that there is an implicit assumption in pretty
much all the code calling has_dos_drive_prefix(): it assumes that the
DOS drive prefix is always two bytes long.

While this assumption is pretty safe, we can still make the code more
readable and less error-prone by introducing a function that skips the
DOS drive prefix safely.

While at it, we change the has_dos_drive_prefix() return value: it now
returns the number of bytes to be skipped if there is a DOS drive prefix.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c |  4 +---
 compat/mingw.c    | 14 +++++---------
 compat/mingw.h    | 10 +++++++++-
 git-compat-util.h |  8 ++++++++
 path.c            | 14 +++++---------
 5 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..9f00421 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -4,9 +4,7 @@
 char *gitbasename (char *path)
 {
 	const char *base;
-	/* Skip over the disk name in MSDOS pathnames. */
-	if (has_dos_drive_prefix(path))
-		path += 2;
+	skip_dos_drive_prefix(&path);
 	for (base = path; *path; path++) {
 		if (is_dir_sep(*path))
 			base = path + 1;
diff --git a/compat/mingw.c b/compat/mingw.c
index 5edea29..1b3530a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1934,26 +1934,22 @@ pid_t waitpid(pid_t pid, int *status, int options)
 
 int mingw_offset_1st_component(const char *path)
 {
-	int offset = 0;
-	if (has_dos_drive_prefix(path))
-		offset = 2;
+	char *pos = (char *)path;
 
 	/* unc paths */
-	else if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
-
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
 		/* skip server name */
-		char *pos = strpbrk(path + 2, "\\/");
+		pos = strpbrk(pos + 2, "\\/");
 		if (!pos)
 			return 0; /* Error: malformed unc path */
 
 		do {
 			pos++;
 		} while (*pos && !is_dir_sep(*pos));
-
-		offset = pos - path;
 	}
 
-	return offset + is_dir_sep(path[offset]);
+	return pos + is_dir_sep(*pos) - path;
 }
 
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
diff --git a/compat/mingw.h b/compat/mingw.h
index 57ca477..b3e5044 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -361,7 +361,15 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+static inline int mingw_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 2da0a75..fbb11bb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -335,6 +335,14 @@ static inline int git_has_dos_drive_prefix(const char *path)
 #define has_dos_drive_prefix git_has_dos_drive_prefix
 #endif
 
+#ifndef skip_dos_drive_prefix
+static inline int git_skip_dos_drive_prefix(char **path)
+{
+	return 0;
+}
+#define skip_dos_drive_prefix git_skip_dos_drive_prefix
+#endif
+
 #ifndef is_dir_sep
 static inline int git_is_dir_sep(int c)
 {
diff --git a/path.c b/path.c
index 3cd155e..8b7e168 100644
--- a/path.c
+++ b/path.c
@@ -782,13 +782,10 @@ const char *relative_path(const char *in, const char *prefix,
 	else if (!prefix_len)
 		return in;
 
-	if (have_same_root(in, prefix)) {
+	if (have_same_root(in, prefix))
 		/* bypass dos_drive, for "c:" is identical to "C:" */
-		if (has_dos_drive_prefix(in)) {
-			i = 2;
-			j = 2;
-		}
-	} else {
+		i = j = has_dos_drive_prefix(in);
+	else {
 		return in;
 	}
 
@@ -943,11 +940,10 @@ const char *remove_leading_path(const char *in, const char *prefix)
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 {
 	char *dst0;
+	int i;
 
-	if (has_dos_drive_prefix(src)) {
+	for (i = has_dos_drive_prefix(src); i > 0; i--)
 		*dst++ = *src++;
-		*dst++ = *src++;
-	}
 	dst0 = dst;
 
 	if (is_dir_sep(*src)) {
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Sixt]

Am 12.01.2016 um 08:57 schrieb Johannes Schindelin:
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 57ca477..b3e5044 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -361,7 +361,15 @@ HANDLE winansi_get_osfhandle(int fd);
>    * git specific compatibility
>    */
>   
> -#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
> +#define has_dos_drive_prefix(path) \
> +	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> +static inline int mingw_skip_dos_drive_prefix(char **path)
> +{
> +	int ret = has_dos_drive_prefix(*path);
> +	*path += ret;
> +	return ret;
> +}
> +#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix

This triggers

    CC alloc.o
In file included from git-compat-util.h:186,
                 from cache.h:4,
                 from alloc.c:12:
compat/mingw.h: In function 'mingw_skip_dos_drive_prefix':
compat/mingw.h:365: warning: implicit declaration of function 'isalpha'

when I build under the old MSYS environment. While I would understand
that the old MSYS environment is end-of-lifed and not worth your time
catering to, the error is still an indication of a problem.

Notice that mingw.h is #included in line 186 of git-compat-util.h,
isalpha is only (re-)defined much later in line 790. That would explain
the warning. What I do not understand is that you do not observe the
same warning in your MSYS2/MINGWxx environment. It would mean that
<ctype.h> is included somewhere.

At any rate, the resulting binary sometimes uses an isalpha
implementation other than the one provided in git-compat-util.h. The
result is most likely correct, but it is certainly not the intent,
is it?

I did not attempt to build with MSVC, but it is not unlikely that it
shows the same error.

I suggest to move the function definition out of line:

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
 	return -1;
 }
 
+int mingw_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+
 int mingw_offset_1st_component(const char *path)
 {
 	char *pos = (char *)path;
diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 
 #define has_dos_drive_prefix(path) \
 	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
 #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> I suggest to move the function definition out of line:
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 10a51c0..0cebb61 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
>  	return -1;
>  }
>  
> +int mingw_skip_dos_drive_prefix(char **path)
> +{
> +	int ret = has_dos_drive_prefix(*path);
> +	*path += ret;
> +	return ret;
> +}
> +
>  int mingw_offset_1st_component(const char *path)
>  {
>  	char *pos = (char *)path;
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 9b5db4e..2099b79 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
>  
>  #define has_dos_drive_prefix(path) \
>  	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> -static inline int mingw_skip_dos_drive_prefix(char **path)
> -{
> -	int ret = has_dos_drive_prefix(*path);
> -	*path += ret;
> -	return ret;
> -}
> +int mingw_skip_dos_drive_prefix(char **path);
>  #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
>  #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
>  static inline char *mingw_find_last_dir_sep(const char *path)

This sounds good to me.  Dscho?

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Jan 2016, Junio C Hamano wrote:

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > I suggest to move the function definition out of line:
> >
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index 10a51c0..0cebb61 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
> >  	return -1;
> >  }
> >  
> > +int mingw_skip_dos_drive_prefix(char **path)
> > +{
> > +	int ret = has_dos_drive_prefix(*path);
> > +	*path += ret;
> > +	return ret;
> > +}
> > +
> >  int mingw_offset_1st_component(const char *path)
> >  {
> >  	char *pos = (char *)path;
> > diff --git a/compat/mingw.h b/compat/mingw.h
> > index 9b5db4e..2099b79 100644
> > --- a/compat/mingw.h
> > +++ b/compat/mingw.h
> > @@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
> >  
> >  #define has_dos_drive_prefix(path) \
> >  	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> > -static inline int mingw_skip_dos_drive_prefix(char **path)
> > -{
> > -	int ret = has_dos_drive_prefix(*path);
> > -	*path += ret;
> > -	return ret;
> > -}
> > +int mingw_skip_dos_drive_prefix(char **path);
> >  #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
> >  #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
> >  static inline char *mingw_find_last_dir_sep(const char *path)
> 
> This sounds good to me.  Dscho?

Yep, sounds good to me, too.

Personally, I have no inclination to add compatibility with the
now-safely-obsolete MSys to my responsibilities, but if Hannes wants to do
it, who am I to stand in his way? Especially when the fix is as trivial as
here.

Ciao,
Dscho

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Sixt]

Am 23.01.2016 um 09:25 schrieb Johannes Schindelin:
> Hi Junio,
>
> On Fri, 22 Jan 2016, Junio C Hamano wrote:
>
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> I suggest to move the function definition out of line:
>>>
>>> diff --git a/compat/mingw.c b/compat/mingw.c
>>> index 10a51c0..0cebb61 100644
>>> --- a/compat/mingw.c
>>> +++ b/compat/mingw.c
>>> @@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
>>>   	return -1;
>>>   }
>>>
>>> +int mingw_skip_dos_drive_prefix(char **path)
>>> +{
>>> +	int ret = has_dos_drive_prefix(*path);
>>> +	*path += ret;
>>> +	return ret;
>>> +}
>>> +
>>>   int mingw_offset_1st_component(const char *path)
>>>   {
>>>   	char *pos = (char *)path;
>>> diff --git a/compat/mingw.h b/compat/mingw.h
>>> index 9b5db4e..2099b79 100644
>>> --- a/compat/mingw.h
>>> +++ b/compat/mingw.h
>>> @@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
>>>
>>>   #define has_dos_drive_prefix(path) \
>>>   	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
>>> -static inline int mingw_skip_dos_drive_prefix(char **path)
>>> -{
>>> -	int ret = has_dos_drive_prefix(*path);
>>> -	*path += ret;
>>> -	return ret;
>>> -}
>>> +int mingw_skip_dos_drive_prefix(char **path);
>>>   #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
>>>   #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
>>>   static inline char *mingw_find_last_dir_sep(const char *path)
>>
>> This sounds good to me.  Dscho?
>
> Yep, sounds good to me, too.
>
> Personally, I have no inclination to add compatibility with the
> now-safely-obsolete MSys to my responsibilities, but if Hannes wants to do
> it, who am I to stand in his way? Especially when the fix is as trivial as
> here.

This is not a matter of compatibility. I am VERY curious why you do not 
see an error (or warning) without my proposed fixup. As I mentioned, 
isalpha() is defined much later than the definition of 
mingw_skip_dos_drive_prefix(). Where does your build get a declaration 
of isalpha() from?

-- Hannes

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Schindelin]

Hi Hannes,

On Sat, 23 Jan 2016, Johannes Sixt wrote:

> Am 23.01.2016 um 09:25 schrieb Johannes Schindelin:
>
> > On Fri, 22 Jan 2016, Junio C Hamano wrote:
> >
> > > Johannes Sixt <j6t@kdbg.org> writes:
> > >
> > > > I suggest to move the function definition out of line:
> > > >
> > > > diff --git a/compat/mingw.c b/compat/mingw.c
> > > > index 10a51c0..0cebb61 100644
> > > > --- a/compat/mingw.c
> > > > +++ b/compat/mingw.c
> > > > @@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int
> > > > options)
> > > >   	return -1;
> > > >   }
> > > >
> > > > +int mingw_skip_dos_drive_prefix(char **path)
> > > > +{
> > > > +	int ret = has_dos_drive_prefix(*path);
> > > > +	*path += ret;
> > > > +	return ret;
> > > > +}
> > > > +
> > > >   int mingw_offset_1st_component(const char *path)
> > > >   {
> > > >   	char *pos = (char *)path;
> > > > diff --git a/compat/mingw.h b/compat/mingw.h
> > > > index 9b5db4e..2099b79 100644
> > > > --- a/compat/mingw.h
> > > > +++ b/compat/mingw.h
> > > > @@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
> > > >
> > > >   #define has_dos_drive_prefix(path) \
> > > >   	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> > > > -static inline int mingw_skip_dos_drive_prefix(char **path)
> > > > -{
> > > > -	int ret = has_dos_drive_prefix(*path);
> > > > -	*path += ret;
> > > > -	return ret;
> > > > -}
> > > > +int mingw_skip_dos_drive_prefix(char **path);
> > > >   #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
> > > >   #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
> > > >   static inline char *mingw_find_last_dir_sep(const char *path)
> > >
> > > This sounds good to me.  Dscho?
> >
> > Yep, sounds good to me, too.
> >
> > Personally, I have no inclination to add compatibility with the
> > now-safely-obsolete MSys to my responsibilities, but if Hannes wants to do
> > it, who am I to stand in his way? Especially when the fix is as trivial as
> > here.
> 
> This is not a matter of compatibility. I am VERY curious why you do not see
> an error (or warning) without my proposed fixup. As I mentioned, isalpha() is
> defined much later than the definition of mingw_skip_dos_drive_prefix().
> Where does your build get a declaration of isalpha() from?

$ grep -w isalpha /mingw32/i686-w64-mingw32/include/*.h
/mingw32/i686-w64-mingw32/include/ctype.h:  _CRTIMP int __cdecl isalpha(int _C);
/mingw32/i686-w64-mingw32/include/ctype.h:#define __iscsymf(_c) (isalpha(_c) || ((_c)=='_'))

I guess that definition gets pulled in somehow.

Ciao,
Dscho

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Sixt]

Am 24.01.2016 um 11:56 schrieb Johannes Schindelin:
> $ grep -w isalpha /mingw32/i686-w64-mingw32/include/*.h
> /mingw32/i686-w64-mingw32/include/ctype.h:  _CRTIMP int __cdecl isalpha(int _C);
> /mingw32/i686-w64-mingw32/include/ctype.h:#define __iscsymf(_c) (isalpha(_c) || ((_c)=='_'))
> 
> I guess that definition gets pulled in somehow.

Ok, then, Junio, kindly replace js/dirname-basename~4 with this patch.
(I hope I get the patch headers right for git-am.)

---- 8< ----
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH] Refactor skipping DOS drive prefixes

Junio noticed that there is an implicit assumption in pretty much
all the code calling has_dos_drive_prefix(): it forces all of its
callsites to hardcode the knowledge that the DOS drive prefix is
always two bytes long.

While this assumption is pretty safe, we can still make the code
more readable and less error-prone by introducing a function that
skips the DOS drive prefix safely.

While at it, we change the has_dos_drive_prefix() return value: it
now returns the number of bytes to be skipped if there is a DOS
drive prefix.

[j6t: moved definition of mingw_skip_dos_drive_prefix() out of line]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 compat/basename.c |  4 +---
 compat/mingw.c    | 21 ++++++++++++---------
 compat/mingw.h    |  5 ++++-
 git-compat-util.h |  8 ++++++++
 path.c            | 14 +++++---------
 5 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..9f00421 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -4,9 +4,7 @@
 char *gitbasename (char *path)
 {
 	const char *base;
-	/* Skip over the disk name in MSDOS pathnames. */
-	if (has_dos_drive_prefix(path))
-		path += 2;
+	skip_dos_drive_prefix(&path);
 	for (base = path; *path; path++) {
 		if (is_dir_sep(*path))
 			base = path + 1;
diff --git a/compat/mingw.c b/compat/mingw.c
index f74da23..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1915,28 +1915,31 @@ pid_t waitpid(pid_t pid, int *status, int options)
 	return -1;
 }
 
+int mingw_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+
 int mingw_offset_1st_component(const char *path)
 {
-	int offset = 0;
-	if (has_dos_drive_prefix(path))
-		offset = 2;
+	char *pos = (char *)path;
 
 	/* unc paths */
-	else if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
-
+	if (!skip_dos_drive_prefix(&pos) &&
+			is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
 		/* skip server name */
-		char *pos = strpbrk(path + 2, "\\/");
+		pos = strpbrk(pos + 2, "\\/");
 		if (!pos)
 			return 0; /* Error: malformed unc path */
 
 		do {
 			pos++;
 		} while (*pos && !is_dir_sep(*pos));
-
-		offset = pos - path;
 	}
 
-	return offset + is_dir_sep(path[offset]);
+	return pos + is_dir_sep(*pos) - path;
 }
 
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
diff --git a/compat/mingw.h b/compat/mingw.h
index 738865c..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -358,7 +358,10 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#define has_dos_drive_prefix(path) \
+	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+int mingw_skip_dos_drive_prefix(char **path);
+#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 0feeae2..38397d7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -335,6 +335,14 @@ static inline int git_has_dos_drive_prefix(const char *path)
 #define has_dos_drive_prefix git_has_dos_drive_prefix
 #endif
 
+#ifndef skip_dos_drive_prefix
+static inline int git_skip_dos_drive_prefix(char **path)
+{
+	return 0;
+}
+#define skip_dos_drive_prefix git_skip_dos_drive_prefix
+#endif
+
 #ifndef is_dir_sep
 static inline int git_is_dir_sep(int c)
 {
diff --git a/path.c b/path.c
index 38f2ebd..747d6da 100644
--- a/path.c
+++ b/path.c
@@ -544,13 +544,10 @@ const char *relative_path(const char *in, const char *prefix,
 	else if (!prefix_len)
 		return in;
 
-	if (have_same_root(in, prefix)) {
+	if (have_same_root(in, prefix))
 		/* bypass dos_drive, for "c:" is identical to "C:" */
-		if (has_dos_drive_prefix(in)) {
-			i = 2;
-			j = 2;
-		}
-	} else {
+		i = j = has_dos_drive_prefix(in);
+	else {
 		return in;
 	}
 
@@ -703,11 +700,10 @@ const char *remove_leading_path(const char *in, const char *prefix)
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 {
 	char *dst0;
+	int i;
 
-	if (has_dos_drive_prefix(src)) {
+	for (i = has_dos_drive_prefix(src); i > 0; i--)
 		*dst++ = *src++;
-		*dst++ = *src++;
-	}
 	dst0 = dst;
 
 	if (is_dir_sep(*src)) {
-- 
2.7.0.118.g90056ae

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 24.01.2016 um 11:56 schrieb Johannes Schindelin:
>> $ grep -w isalpha /mingw32/i686-w64-mingw32/include/*.h
>> /mingw32/i686-w64-mingw32/include/ctype.h:  _CRTIMP int __cdecl isalpha(int _C);
>> /mingw32/i686-w64-mingw32/include/ctype.h:#define __iscsymf(_c) (isalpha(_c) || ((_c)=='_'))
>> 
>> I guess that definition gets pulled in somehow.
>
> Ok, then, Junio, kindly replace js/dirname-basename~4 with this patch.
> (I hope I get the patch headers right for git-am.)

Thanks for following it through.

If they are already in 'next', we'd want an incremental fixup, though.

    ... goes and looks ...

Yeah, unfortunately that is already in 'next'; could you make this
into an incremental, which would come with its own log message that
explains why the inlining was wrong?

Thanks.

[PATCH] mingw: avoid linking to the C library's isalpha()

[Johannes Sixt]

The implementation of mingw_skip_dos_drive_prefix() calls isalpha() via
has_dos_drive_prefix(). Since the definition occurs long before isalpha()
is defined in git-compat-util.h, my build environment reports:

    CC alloc.o
In file included from git-compat-util.h:186,
                 from cache.h:4,
                 from alloc.c:12:
compat/mingw.h: In function 'mingw_skip_dos_drive_prefix':
compat/mingw.h:365: warning: implicit declaration of function 'isalpha'

Dscho does not see a similar warning in his build and suspects that
ctype.h is included somehow behind the scenes. This implies that his build
links to the C library's isalpha() and does not use git's isalpha().

To fix both the warning in my build and the inconsistency in Dscho's
build, move the function definition to mingw.c. Then it picks up git's
isalpha() because git-compat-util.h is included at the top of the file.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 compat/mingw.c | 7 +++++++
 compat/mingw.h | 7 +------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
 	return -1;
 }
 
+int mingw_skip_dos_drive_prefix(char **path)
+{
+	int ret = has_dos_drive_prefix(*path);
+	*path += ret;
+	return ret;
+}
+
 int mingw_offset_1st_component(const char *path)
 {
 	char *pos = (char *)path;
diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 
 #define has_dos_drive_prefix(path) \
 	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-	int ret = has_dos_drive_prefix(*path);
-	*path += ret;
-	return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
 #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
-- 
2.7.0.118.g90056ae

Re: [PATCH] mingw: avoid linking to the C library's isalpha()

[Junio C Hamano]

Thanks.

[PATCH v4 2/4] compat/basename: make basename() conform to POSIX

[Johannes Schindelin]

According to POSIX, basename("/path/") should return "path", not
"path/". Likewise, basename(NULL) and basename("") should both
return "." to conform.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

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;
 }
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v4 2/4] compat/basename: make basename() conform to POSIX

[Ramsay Jones]

On 12/01/16 07:57, Johannes Schindelin wrote:
> According to POSIX, basename("/path/") should return "path", not
> "path/". Likewise, basename(NULL) and basename("") should both
> return "." to conform.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/basename.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> 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:

char *gitbasename (char *path)
{
	char *p;

	if (!path || !*path)
		return ".";
	/* skip drive designator, if any */
	if (has_dos_drive_prefix(path))
		path += 2;
	if (!*path)
		return ".";
	/* trim trailing directory separators */
	p = path + strlen(path) - 1;
	while (is_dir_sep(*p)) {
		if (p == path)
			return path;
		*p-- = '\0';
	}
	/* find begining of last path component */
	while (p > path && !is_dir_sep(*p))
		p--;
	if (is_dir_sep(*p))
		p++;
	return p;
}

ATB,
Ramsay Jones

Re: [PATCH v4 2/4] compat/basename: make basename() conform to POSIX

[Johannes Schindelin]

Hi Ramsay,

On Wed, 13 Jan 2016, Ramsay Jones wrote:

> 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

[PATCH v4 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Johannes Schindelin]

When there is no `libgen.h` to our disposal, we miss the `dirname()`
function.

So far, we only had one user of that function: credential-cache--daemon
(which was only compiled when Unix sockets are available, anyway). But
now we also have `builtin/am.c` as user, so we need it.

Since `dirname()` is a sibling of `basename()`, we simply put our very
own `gitdirname()` implementation next to `gitbasename()` and use it
if `NO_LIBGEN_H` has been set.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/basename.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/compat/basename.c b/compat/basename.c
index 0f1b0b0..96bd953 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -1,4 +1,5 @@
 #include "../git-compat-util.h"
+#include "../strbuf.h"
 
 /* Adapted from libiberty's basename.c.  */
 char *gitbasename (char *path)
@@ -25,3 +26,46 @@ char *gitbasename (char *path)
 	}
 	return (char *)base;
 }
+
+char *gitdirname(char *path)
+{
+	static struct strbuf buf = STRBUF_INIT;
+	char *p = path, *slash = NULL, c;
+	int dos_drive_prefix;
+
+	if (!p)
+		return ".";
+
+	if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p)
+		goto dot;
+
+	/*
+	 * POSIX.1-2001 says dirname("/") should return "/", and dirname("//")
+	 * should return "//", but dirname("///") should return "/" again.
+	 */
+	if (is_dir_sep(*p)) {
+		if (!p[1] || (is_dir_sep(p[1]) && !p[2]))
+			return path;
+		slash = ++p;
+	}
+	while ((c = *(p++)))
+		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;
+		}
+
+	if (slash) {
+		*slash = '\0';
+		return path;
+	}
+
+dot:
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
+	return buf.buf;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index fbb11bb..5f72f1c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -253,6 +253,8 @@ struct itimerval {
 #else
 #define basename gitbasename
 extern char *gitbasename(char *);
+#define dirname gitdirname
+extern char *gitdirname(char *);
 #endif
 
 #ifndef NO_ICONV
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v4 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Ramsay Jones]

On 12/01/16 07:57, Johannes Schindelin wrote:
> When there is no `libgen.h` to our disposal, we miss the `dirname()`
> function.
> 
> So far, we only had one user of that function: credential-cache--daemon
> (which was only compiled when Unix sockets are available, anyway). But
> now we also have `builtin/am.c` as user, so we need it.
> 
> Since `dirname()` is a sibling of `basename()`, we simply put our very
> own `gitdirname()` implementation next to `gitbasename()` and use it
> if `NO_LIBGEN_H` has been set.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/basename.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  git-compat-util.h |  2 ++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/compat/basename.c b/compat/basename.c
> index 0f1b0b0..96bd953 100644
> --- a/compat/basename.c
> +++ b/compat/basename.c
> @@ -1,4 +1,5 @@
>  #include "../git-compat-util.h"
> +#include "../strbuf.h"
>  
>  /* Adapted from libiberty's basename.c.  */
>  char *gitbasename (char *path)
> @@ -25,3 +26,46 @@ char *gitbasename (char *path)
>  	}
>  	return (char *)base;
>  }
> +
> +char *gitdirname(char *path)
> +{
> +	static struct strbuf buf = STRBUF_INIT;
> +	char *p = path, *slash = NULL, c;
> +	int dos_drive_prefix;
> +
> +	if (!p)
> +		return ".";
> +
> +	if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p)
> +		goto dot;
> +
> +	/*
> +	 * POSIX.1-2001 says dirname("/") should return "/", and dirname("//")
> +	 * should return "//", but dirname("///") should return "/" again.
> +	 */
> +	if (is_dir_sep(*p)) {
> +		if (!p[1] || (is_dir_sep(p[1]) && !p[2]))
> +			return path;
> +		slash = ++p;
> +	}
> +	while ((c = *(p++)))
> +		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;
> +		}
> +
> +	if (slash) {
> +		*slash = '\0';
> +		return path;
> +	}
> +
> +dot:
> +	strbuf_reset(&buf);
> +	strbuf_addf(&buf, "%.*s.", dos_drive_prefix, path);
> +	return buf.buf;
> +}

Again, I find my version much easier to read:

char *gitdirname(char *path)
{
	char *p, *start;

	if (!path || !*path)
		return ".";
	start = path;
	/* skip drive designator, if any */
	if (has_dos_drive_prefix(path))
		start += 2;
	/* check for // */
	if (strcmp(start, "//") == 0)
		return path;
	/* check for \\ */
	if (is_dir_sep('\\') && strcmp(start, "\\\\") == 0)
		return path;
	/* trim trailing directory separators */
	p = path + strlen(path) - 1;
	while (is_dir_sep(*p)) {
		if (p == start)
			return path;
		*p-- = '\0';
	}
	/* find begining of last path component */
	while (p > start && !is_dir_sep(*p))
		p--;
	/* terminate dirname */
	if (p == start && !is_dir_sep(*p))
		*p++ = '.';
	else if (p == start)
		p++;
	*p = '\0';
	return path;
}

> diff --git a/git-compat-util.h b/git-compat-util.h
> index fbb11bb..5f72f1c 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -253,6 +253,8 @@ struct itimerval {
>  #else

Also, when compiling on Cygwin with NO_LIBGEN_H, I need to
include the following here:

#undef basename

in order to suppress approx 230 warnings about the redefinition
of the basename macro.

(I suppose that should go in the previous commit. dunno)

>  #define basename gitbasename
>  extern char *gitbasename(char *);
> +#define dirname gitdirname
> +extern char *gitdirname(char *);
>  #endif
>  
>  #ifndef NO_ICONV
> 

ATB,
Ramsay Jones

Re: [PATCH v4 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Johannes Schindelin]

Hi Ramsay,

On Wed, 13 Jan 2016, Ramsay Jones wrote:

> Also, when compiling on Cygwin with NO_LIBGEN_H, I need to
> include the following here:
> 
> #undef basename
> 
> in order to suppress approx 230 warnings about the redefinition
> of the basename macro.
> 
> (I suppose that should go in the previous commit. dunno)

I think this is an incorrect use of NO_LIBGEN_H (because Cygwin obviously
has it), but in any case, it is a completely independent issue from
fixing/testing basename()/dirname(), so your #undef basename should be in
a completely separate commit, methinks.

Ciao,
Dscho

Re: [PATCH v4 3/4] Provide a dirname() function when NO_LIBGEN_H=YesPlease

[Ramsay Jones]

On 13/01/16 07:40, Johannes Schindelin wrote:
> Hi Ramsay,
> 
> On Wed, 13 Jan 2016, Ramsay Jones wrote:
> 
>> Also, when compiling on Cygwin with NO_LIBGEN_H, I need to
>> include the following here:
>>
>> #undef basename
>>
>> in order to suppress approx 230 warnings about the redefinition
>> of the basename macro.
>>
>> (I suppose that should go in the previous commit. dunno)
> 
> I think this is an incorrect use of NO_LIBGEN_H (because Cygwin obviously
> has it), but in any case, it is a completely independent issue from
> fixing/testing basename()/dirname(), so your #undef basename should be in
> a completely separate commit, methinks.

OK. I think this worked fine on 32-bit cygwin, but the system headers
have changed quite a bit on 64-bit cygwin and I only tried it for the
first time yesterday. (It was helpful in the debugging process at one
point to be able to build with NO_LIBGEN_H on all platforms ...)

ATB,
Ramsay Jones

[PATCH v4 4/4] t0060: verify that basename() and dirname() work as expected

[Johannes Schindelin]

Unfortunately, some libgen implementations yield outcomes different
from what Git expects. For example, mingw-w64-crt provides a basename()
function, that shortens `path0/` to `path`!

So let's verify that the basename() and dirname() functions we use
conform to what Git expects.

Derived-from-code-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0060-path-utils.sh |   3 +
 test-path-utils.c     | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 627ef85..f0152a7 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -59,6 +59,9 @@ case $(uname -s) in
 	;;
 esac
 
+test_expect_success basename 'test-path-utils basename'
+test_expect_success dirname 'test-path-utils dirname'
+
 norm_path "" ""
 norm_path . ""
 norm_path ./ ""
diff --git a/test-path-utils.c b/test-path-utils.c
index c67bf65..4ab68ac 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -39,6 +39,166 @@ static void normalize_argv_string(const char **var, const char *input)
 		die("Bad value: %s\n", input);
 }
 
+struct test_data {
+	const char *from;  /* input:  transform from this ... */
+	const char *to;    /* output: ... to this.            */
+};
+
+static int test_function(struct test_data *data, char *(*func)(char *input),
+	const char *funcname)
+{
+	int failed = 0, i;
+	char buffer[1024];
+	char *to;
+
+	for (i = 0; data[i].to; i++) {
+		if (!data[i].from)
+			to = func(NULL);
+		else {
+			strcpy(buffer, data[i].from);
+			to = func(buffer);
+		}
+		if (strcmp(to, data[i].to)) {
+			error("FAIL: %s(%s) => '%s' != '%s'\n",
+				funcname, data[i].from, to, data[i].to);
+			failed = 1;
+		}
+	}
+	return failed;
+}
+
+static struct test_data basename_data[] = {
+	/* --- POSIX type paths --- */
+	{ NULL,              "."    },
+	{ "",                "."    },
+	{ ".",               "."    },
+	{ "..",              ".."   },
+	{ "/",               "/"    },
+#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
+	{ "//",              "//"   },
+	{ "///",             "//"   },
+	{ "////",            "//"   },
+#else
+	{ "//",              "/"    },
+	{ "///",             "/"    },
+	{ "////",            "/"    },
+#endif
+	{ "usr",             "usr"  },
+	{ "/usr",            "usr"  },
+	{ "/usr/",           "usr"  },
+	{ "/usr//",          "usr"  },
+	{ "/usr/lib",        "lib"  },
+	{ "usr/lib",         "lib"  },
+	{ "usr/lib///",      "lib"  },
+
+#if defined(__MINGW32__) || defined(_MSC_VER)
+
+	/* --- win32 type paths --- */
+	{ "\\usr",           "usr"  },
+	{ "\\usr\\",         "usr"  },
+	{ "\\usr\\\\",       "usr"  },
+	{ "\\usr\\lib",      "lib"  },
+	{ "usr\\lib",        "lib"  },
+	{ "usr\\lib\\\\\\",  "lib"  },
+	{ "C:/usr",          "usr"  },
+	{ "C:/usr",          "usr"  },
+	{ "C:/usr/",         "usr"  },
+	{ "C:/usr//",        "usr"  },
+	{ "C:/usr/lib",      "lib"  },
+	{ "C:usr/lib",       "lib"  },
+	{ "C:usr/lib///",    "lib"  },
+	{ "C:",              "."    },
+	{ "C:a",             "a"    },
+	{ "C:/",             "/"    },
+	{ "C:///",           "/"    },
+#if defined(NO_LIBGEN_H)
+	{ "\\",              "\\"   },
+	{ "\\\\",            "\\"   },
+	{ "\\\\\\",          "\\"   },
+#else
+
+	/* win32 platform variations: */
+#if defined(__MINGW32__)
+	{ "\\",              "/"    },
+	{ "\\\\",            "/"    },
+	{ "\\\\\\",          "/"    },
+#endif
+
+#if defined(_MSC_VER)
+	{ "\\",              "\\"   },
+	{ "\\\\",            "\\"   },
+	{ "\\\\\\",          "\\"   },
+#endif
+
+#endif
+#endif
+	{ NULL,              NULL   }
+};
+
+static struct test_data dirname_data[] = {
+	/* --- POSIX type paths --- */
+	{ NULL,              "."      },
+	{ "",                "."      },
+	{ ".",               "."      },
+	{ "..",              "."      },
+	{ "/",               "/"      },
+	{ "//",              "//"     },
+#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
+	{ "///",             "//"     },
+	{ "////",            "//"     },
+#else
+	{ "///",             "/"      },
+	{ "////",            "/"      },
+#endif
+	{ "usr",             "."      },
+	{ "/usr",            "/"      },
+	{ "/usr/",           "/"      },
+	{ "/usr//",          "/"      },
+	{ "/usr/lib",        "/usr"   },
+	{ "usr/lib",         "usr"    },
+	{ "usr/lib///",      "usr"    },
+
+#if defined(__MINGW32__) || defined(_MSC_VER)
+
+	/* --- win32 type paths --- */
+	{ "\\",              "\\"     },
+	{ "\\\\",            "\\\\"   },
+	{ "\\usr",           "\\"     },
+	{ "\\usr\\",         "\\"     },
+	{ "\\usr\\\\",       "\\"     },
+	{ "\\usr\\lib",      "\\usr"  },
+	{ "usr\\lib",        "usr"    },
+	{ "usr\\lib\\\\\\",  "usr"    },
+	{ "C:a",             "C:."    },
+	{ "C:/",             "C:/"    },
+	{ "C:///",           "C:/"    },
+	{ "C:/usr",          "C:/"    },
+	{ "C:/usr/",         "C:/"    },
+	{ "C:/usr//",        "C:/"    },
+	{ "C:/usr/lib",      "C:/usr" },
+	{ "C:usr/lib",       "C:usr"  },
+	{ "C:usr/lib///",    "C:usr"  },
+	{ "\\\\\\",          "\\"     },
+	{ "\\\\\\\\",        "\\"     },
+#if defined(NO_LIBGEN_H)
+	{ "C:",              "C:."    },
+#else
+
+	/* win32 platform variations: */
+#if defined(__MINGW32__)
+	/* the following is clearly wrong ... */
+	{ "C:",              "."      },
+#endif
+
+#if defined(_MSC_VER)
+	{ "C:",              "C:."    },
+#endif
+
+#endif
+#endif
+	{ NULL,              NULL     }
+};
+
 int main(int argc, char **argv)
 {
 	if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
@@ -133,6 +293,12 @@ int main(int argc, char **argv)
 		return 0;
 	}
 
+	if (argc == 2 && !strcmp(argv[1], "basename"))
+		return test_function(basename_data, basename, argv[1]);
+
+	if (argc == 2 && !strcmp(argv[1], "dirname"))
+		return test_function(dirname_data, dirname, argv[1]);
+
 	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
 		argv[1] ? argv[1] : "(there was none)");
 	return 1;
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v4 4/4] t0060: verify that basename() and dirname() work as expected

[Ramsay Jones]

On 12/01/16 07:57, Johannes Schindelin wrote:
> Unfortunately, some libgen implementations yield outcomes different
> from what Git expects. For example, mingw-w64-crt provides a basename()
> function, that shortens `path0/` to `path`!
> 
> So let's verify that the basename() and dirname() functions we use
> conform to what Git expects.
> 
> Derived-from-code-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t0060-path-utils.sh |   3 +
>  test-path-utils.c     | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 169 insertions(+)
> 
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index 627ef85..f0152a7 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -59,6 +59,9 @@ case $(uname -s) in
>  	;;
>  esac
>  
> +test_expect_success basename 'test-path-utils basename'
> +test_expect_success dirname 'test-path-utils dirname'
> +
>  norm_path "" ""
>  norm_path . ""
>  norm_path ./ ""
> diff --git a/test-path-utils.c b/test-path-utils.c
> index c67bf65..4ab68ac 100644
> --- a/test-path-utils.c
> +++ b/test-path-utils.c
> @@ -39,6 +39,166 @@ static void normalize_argv_string(const char **var, const char *input)
>  		die("Bad value: %s\n", input);
>  }
>  
> +struct test_data {
> +	const char *from;  /* input:  transform from this ... */
> +	const char *to;    /* output: ... to this.            */
> +};
> +
> +static int test_function(struct test_data *data, char *(*func)(char *input),
> +	const char *funcname)
> +{
> +	int failed = 0, i;
> +	char buffer[1024];
> +	char *to;
> +
> +	for (i = 0; data[i].to; i++) {
> +		if (!data[i].from)
> +			to = func(NULL);
> +		else {
> +			strcpy(buffer, data[i].from);
> +			to = func(buffer);
> +		}
> +		if (strcmp(to, data[i].to)) {
> +			error("FAIL: %s(%s) => '%s' != '%s'\n",
> +				funcname, data[i].from, to, data[i].to);
> +			failed = 1;
> +		}
> +	}
> +	return failed;
> +}
> +
> +static struct test_data basename_data[] = {
> +	/* --- POSIX type paths --- */
> +	{ NULL,              "."    },
> +	{ "",                "."    },
> +	{ ".",               "."    },
> +	{ "..",              ".."   },
> +	{ "/",               "/"    },
> +#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
> +	{ "//",              "//"   },
> +	{ "///",             "//"   },
> +	{ "////",            "//"   },
> +#else
> +	{ "//",              "/"    },
> +	{ "///",             "/"    },
> +	{ "////",            "/"    },
> +#endif
> +	{ "usr",             "usr"  },
> +	{ "/usr",            "usr"  },
> +	{ "/usr/",           "usr"  },
> +	{ "/usr//",          "usr"  },
> +	{ "/usr/lib",        "lib"  },
> +	{ "usr/lib",         "lib"  },
> +	{ "usr/lib///",      "lib"  },
> +
> +#if defined(__MINGW32__) || defined(_MSC_VER)
> +
> +	/* --- win32 type paths --- */
> +	{ "\\usr",           "usr"  },
> +	{ "\\usr\\",         "usr"  },
> +	{ "\\usr\\\\",       "usr"  },
> +	{ "\\usr\\lib",      "lib"  },
> +	{ "usr\\lib",        "lib"  },
> +	{ "usr\\lib\\\\\\",  "lib"  },
> +	{ "C:/usr",          "usr"  },
> +	{ "C:/usr",          "usr"  },

This duplication was in the test-libgen.c file I sent
you ... so, my bad. ;-)

Did you not have more tests to add?

ATB,
Ramsay Jones

Re: [PATCH v4 4/4] t0060: verify that basename() and dirname() work as expected

[Johannes Schindelin]

Hi Ramsay,

On Wed, 13 Jan 2016, Ramsay Jones wrote:

> Did you not have more tests to add?

No, all my testing was manual so far, and well covered by your test cases.

Thanks,
Dscho

Re: [PATCH v4 4/4] t0060: verify that basename() and dirname() work as expected

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

Hi Torsten,

On Wed, 13 Jan 2016, Torsten Bögershausen wrote:

> On 01/12/2016 08:57 AM, Johannes Schindelin wrote:
> 
> > +static struct test_data basename_data[] = {
> > +	/* --- POSIX type paths --- */
> > +	{ NULL,              "."    },
> > +	{ "",                "."    },
> > +	{ ".",               "."    },
> > +	{ "..",              ".."   },
> > +	{ "/",               "/"    },
> > +#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
> Why the !defined(NO_LIBGEN_H)
> 
> Shouldn't CYGWIN always behave the same ?

One would assume... Alas, it does not.

I inherited the code in question and wondered the same. I opted for
keeping the code as a documentation of the differing behavior.

> The main problem is, that t0060 fails under Mac OS (with mac ports
> installed):
> expecting success: test-path-utils dirname
> error: FAIL: dirname(//) => '/' != '//'

See the patch I sent this morning.

Ciao,
Dscho

Re: [PATCH v4 4/4] t0060: verify that basename() and dirname() work as expected

[Ramsay Jones]

On 13/01/16 09:27, Johannes Schindelin wrote:
> Hi Torsten,
> 
> On Wed, 13 Jan 2016, Torsten Bögershausen wrote:
> 
>> On 01/12/2016 08:57 AM, Johannes Schindelin wrote:
>>
>>> +static struct test_data basename_data[] = {
>>> +	/* --- POSIX type paths --- */
>>> +	{ NULL,              "."    },
>>> +	{ "",                "."    },
>>> +	{ ".",               "."    },
>>> +	{ "..",              ".."   },
>>> +	{ "/",               "/"    },
>>> +#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
>> Why the !defined(NO_LIBGEN_H)
>>
>> Shouldn't CYGWIN always behave the same ?
> 
> One would assume... Alas, it does not.

Err, ... yes it does! :-P

> 
> I inherited the code in question and wondered the same. I opted for
> keeping the code as a documentation of the differing behavior.

Exactly.

ATB,
Ramsay Jones

Re: [PATCH v4 4/4] t0060: verify that basename() and dirname() work as expected

[Ramsay Jones]

On 13/01/16 05:47, Torsten Bögershausen wrote:
> On 01/12/2016 08:57 AM, Johannes Schindelin wrote:
> 
[snip]

>> +
>> +static struct test_data basename_data[] = {
>> +	/* --- POSIX type paths --- */
>> +	{ NULL,              "."    },
>> +	{ "",                "."    },
>> +	{ ".",               "."    },
>> +	{ "..",              ".."   },
>> +	{ "/",               "/"    },
>> +#if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
> Why the !defined(NO_LIBGEN_H)

These tests were derived from a standalone test program which
I was using to test my implementation of gitbasename() *and*
to document the differences between it and the system versions
of the <libgen.h> basename(). (I didn't bother with the GNU version
of basename, which is somewhat strange).

This particular section documents what is almost certainly a bug
in the cygwin basename() and also documents my choice of 'fix'.
(ie. in my implementation I chose to return '/' for '//', which
is one of the possible options that POSIX allows.)

> 
> Shouldn't CYGWIN always behave the same ?
> And, in general, shouldn't all Windows version behave the same ?

Hmm, cygwin is not really a 'Windows version', so ... (We have been
caught out before by cygwin 'supporting' UNC paths, so support for
'//' is open to question. Also, some git programs on cygwin kinda
sorta support dos paths ...)

> (This would mean, that we always use ../compat/basename.c for all kind
> of Windows Git implementattion. Would there be a drawback ?)

And maybe not just Windows ...

> 
> 
> The main problem is, that t0060 fails under Mac OS (with mac ports installed):
> expecting success: test-path-utils dirname
> error: FAIL: dirname(//) => '/' != '//'

Yep, not surprised. Again that test file was developed and tested on
only the five platforms available to me at the time, namely: Linux (both
32 and 64bit), Windows XP 32-bit (MSVC), MinGW 32-bit and Cygwin 32-bit.

POSIX says, in part [1]:

    If the string pointed to by path consists entirely of the '/' character,
    basename() shall return a pointer to the string "/". If the string pointed
    to by path is exactly "//", it is implementation-defined whether '/' or "//"
    is returned.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/basename.html

So we should expect other systems to differ, even if they support POSIX. (and maybe
not just this test case.)

> 
> not ok 2 - dirname
> #       test-path-utils dirname
> 
> To my understanding the treatment of a path name like "//"
> is defined as "undefined":
> If /string/ is "//", it is implementation-defined whether steps 3 to 6 are skipped or processed.
> http://pubs.opengroup.org/onlinepubs/009695399/utilities/basename.html
> 
> What I understand is that a path like "//XX/YY/ZZ" can be handled in 3 different ways:
> a) Same as "/XX/YY/ZZ", silently turning "//" into "/"
> b) Same as "\\XX\YY\ZZ", using UNC names under Windows. # Note: this reads as "\\\\XX\\YY\\ZZ" in a C program
> c) As invalid
> 
> 
> Does it make sense to use the compat/basename.c for all Git implementations ?
> (Or at least for CYGWIN, Mac OS, MSYS, MSVC)
> 

Maybe, I hadn't got that far yet ... :-D

ATB,
Ramsay Jones

Re: [PATCH v4 4/4] t0060: verify that basename() and dirname() work as expected

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> This particular section documents what is almost certainly a bug
> in the cygwin basename() and also documents my choice of 'fix'.
> (ie. in my implementation I chose to return '/' for '//', which
> is one of the possible options that POSIX allows.)
> ...
> POSIX says, in part [1]:
>
>     If the string pointed to by path consists entirely of the '/' character,
>     basename() shall return a pointer to the string "/". If the string pointed
>     to by path is exactly "//", it is implementation-defined whether '/' or "//"
>     is returned.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/basename.html
>
> So we should expect other systems to differ, even if they support POSIX. (and maybe
> not just this test case.)

Doesn't that mean the test shouldn't be insisting on the output
being one that you arbitrarily pick?  It feels to me that it is
wrong to say "We require // to become / unless we know we are on
such and such systems".  Instead, shouldn't it be doing "We feed //
to the function.  Either / or // is acceptable; any other value is a
bug"?

It is tempting to have a "check" feature to the test program for the
curious which one of the two acceptable results a particular platform
(or more precisely, implementation of basename/dirname) produces, but
I do not think "test" feature should insist on / and reject //.

Re: [PATCH v4 4/4] t0060: verify that basename() and dirname() work as expected

[Johannes Schindelin]

Hi Junio,

On Wed, 13 Jan 2016, Junio C Hamano wrote:

> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
> > This particular section documents what is almost certainly a bug
> > in the cygwin basename() and also documents my choice of 'fix'.  (ie.
> > in my implementation I chose to return '/' for '//', which is one of
> > the possible options that POSIX allows.)
> > ...
> > POSIX says, in part [1]:
> >
> >     If the string pointed to by path consists entirely of the '/'
> >     character, basename() shall return a pointer to the string "/". If
> >     the string pointed to by path is exactly "//", it is
> >     implementation-defined whether '/' or "//" is returned.
> >
> > [1]
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/basename.html
> >
> > So we should expect other systems to differ, even if they support POSIX. (and maybe
> > not just this test case.)
> 
> Doesn't that mean the test shouldn't be insisting on the output
> being one that you arbitrarily pick?  It feels to me that it is
> wrong to say "We require // to become / unless we know we are on
> such and such systems".  Instead, shouldn't it be doing "We feed //
> to the function.  Either / or // is acceptable; any other value is a
> bug"?

I guess that is the best solution of all. I'll try to modify
test-path-utils.c accordingly tomorrow.

Ciao,
Dscho

Re: [PATCH v4 0/4] Ensure that we can build without libgen.h

[Ramsay Jones]

Hi Johannes,

Sorry for not commenting sooner, I've been away from email for
a few days. Also, I have only just looked at what is currently
in pu (@1a05310), which I'm pretty sure is v3 of this series.

On 12/01/16 07:57, Johannes Schindelin wrote:
> This mini series adds a fall-back for the `dirname()` function that we use
> e.g. in git-am. This is necessary because not all platforms have a working
> libgen.h.
> 
> While at it, we ensure that our basename() drop-in conforms to the POSIX
> specifications.

I was somewhat disappointed that you ignored the implementation of
gitbasename() and gitdirname() that was included in the test-libgen.c
file that I sent you. I had hoped they would be (at worst) a good starting
point if you found them to be lacking for your use case (ie. for the
64-bit versions of MSVC/MinGW).

Did you have any test cases that failed? (If so, could you please add
them to the tests).

Hmm, I just had another look at them and recalled one of my TODO items.
Ahem, yes, ... err, replace code which provoked undefined behaviour. :-P

Actually, that took just ten minutes to fix. (patch below)

> 
> In addition to Eric's style improvement, v4 also fixes the signature
> of skip_dos_drive_prefix() in the non-Windows case.

Yes, this fixes one of my comments about v3.

ATB,
Ramsay Jones
-- >8 --
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Tue, 12 Jan 2016 23:28:09 +0000
Subject: [PATCH] test-libgen.c: don't provoke undefined behaviour

---
 test-libgen.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/test-libgen.c b/test-libgen.c
index aa3bd18..3024bf1 100644
--- a/test-libgen.c
+++ b/test-libgen.c
@@ -42,9 +42,11 @@ char *gitbasename (char *path)
 		*p-- = '\0';
 	}
 	/* find begining of last path component */
-	while (p >= path && !is_dir_sep(*p))
+	while (p > path && !is_dir_sep(*p))
 		p--;
-	return p + 1;
+	if (is_dir_sep(*p))
+		p++;
+	return p;
 }
 
 char *gitdirname(char *path)
@@ -71,13 +73,12 @@ char *gitdirname(char *path)
 		*p-- = '\0';
 	}
 	/* find begining of last path component */
-	while (p >= start && !is_dir_sep(*p))
+	while (p > start && !is_dir_sep(*p))
 		p--;
 	/* terminate dirname */
-	if (p < start) {
-		p = start;
+	if (p == start && !is_dir_sep(*p))
 		*p++ = '.';
-	} else if (p == start)
+	else if (p == start)
 		p++;
 	*p = '\0';
 	return path;
-- 
2.7.0

Re: [PATCH v4 0/4] Ensure that we can build without libgen.h

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Hi Johannes,
> ...
> I was somewhat disappointed that you ignored the implementation of
> gitbasename() and gitdirname() that was included in the test-libgen.c
> file that I sent you. I had hoped they would be (at worst) a good starting
> point if you found them to be lacking for your use case (ie. for the
> 64-bit versions of MSVC/MinGW).

Sorry to hear that, but the 'next' branch has just been rewound and
rebuilt with this series, so it is too late to replace them with
another round of reroll.  It however is never too late to improve
with incremental updates, though, so please work together and send
in a follow-up patch series as/if needed.

Thanks.

Re: [PATCH v4 0/4] Ensure that we can build without libgen.h

[Torsten Bögershausen]

On 01/13/2016 03:56 AM, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> Hi Johannes,
>> ...
>> I was somewhat disappointed that you ignored the implementation of
>> gitbasename() and gitdirname() that was included in the test-libgen.c
>> file that I sent you. I had hoped they would be (at worst) a good starting
>> point if you found them to be lacking for your use case (ie. for the
>> 64-bit versions of MSVC/MinGW).
> Sorry to hear that, but the 'next' branch has just been rewound and
> rebuilt with this series, so it is too late to replace them with
> another round of reroll.  It however is never too late to improve
> with incremental updates, though, so please work together and send
> in a follow-up patch series as/if needed.
>
Is there a chance to keep it in next (and not merge to master) until
we have found a solution for the broken t0060 test under Mac OS X ?

Re: [PATCH v4 0/4] Ensure that we can build without libgen.h

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2558 bytes --]

Hi Torsten,

On Wed, 13 Jan 2016, Torsten Bögershausen wrote:

> On 01/13/2016 03:56 AM, Junio C Hamano wrote:
> > Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> >
> > > Hi Johannes,
> > > ...
> > > I was somewhat disappointed that you ignored the implementation of
> > > gitbasename() and gitdirname() that was included in the test-libgen.c
> > > file that I sent you. I had hoped they would be (at worst) a good
> > > starting
> > > point if you found them to be lacking for your use case (ie. for the
> > > 64-bit versions of MSVC/MinGW).
> > Sorry to hear that, but the 'next' branch has just been rewound and
> > rebuilt with this series, so it is too late to replace them with
> > another round of reroll.  It however is never too late to improve
> > with incremental updates, though, so please work together and send
> > in a follow-up patch series as/if needed.
> >
> Is there a chance to keep it in next (and not merge to master) until
> we have found a solution for the broken t0060 test under Mac OS X ?

This is actually independent of follow-up patches that might replace the
gitbasename()/gitdirname() functions wholesale, as the problem you
encountered is *differing* behavior: no matter what implementation of
gitdirname() we use, it will either agree with dirname() on Linux or with
dirname() on MacOSX, it cannot do both.

So here is a hot-fix (Junio, would you kindly apply it?):

-- snipsnap --
Subject: [PATCH] t0060: fix dirname test on MacOSX

Contrary to this developer's assumptions, the behavior of dirname("//") is
not consistent between platforms. Let's document this by not removing this
test case, but showing that it behaves differently on MacOSX (unless
NO_LIBGEN_H is defined, that is).

Pointed-out-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 test-path-utils.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/test-path-utils.c b/test-path-utils.c
index 4ab68ac..a3a69ce 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -142,7 +142,11 @@ static struct test_data dirname_data[] = {
 	{ ".",               "."      },
 	{ "..",              "."      },
 	{ "/",               "/"      },
+#if defined(__APPLE__) && !defined(NO_LIBGEN_H)
+	{ "//",              "/"     },
+#else
 	{ "//",              "//"     },
+#endif
 #if defined(__CYGWIN__) && !defined(NO_LIBGEN_H)
 	{ "///",             "//"     },
 	{ "////",            "//"     },
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v4 0/4] Ensure that we can build without libgen.h

[Johannes Schindelin]

Hi Ramsay,

On Wed, 13 Jan 2016, Ramsay Jones wrote:

> On 12/01/16 07:57, Johannes Schindelin wrote:
> > This mini series adds a fall-back for the `dirname()` function that we use
> > e.g. in git-am. This is necessary because not all platforms have a working
> > libgen.h.
> > 
> > While at it, we ensure that our basename() drop-in conforms to the POSIX
> > specifications.
> 
> I was somewhat disappointed that you ignored the implementation of
> gitbasename() and gitdirname() that was included in the test-libgen.c
> file that I sent you.

I am sorry you feel that I ignored your work!

My line of reasoning, however, was to go with the existing gitbasename()
and with the gitdirname() I had come up with, because I was already
familiar with them.

Your tests included a couple of corner cases that neither handled
correctly, and I was able to fix that, so I was happy.

To be quite honest, I blindly deleted everything but the tests, noticed
that the remaining code looked eerily similar to test-path-utils, and
merged it there.

Ciao,
Dscho