patchProvide a dirname() function when NO_LIBGEN_H=YesPlease

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

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

Thanks for being careful.

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.

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

I would have expected the function to scan from the end/right/tail.

Hi Junio,

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

In the upcoming v2, this is addressed.

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

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

Ciao, Dscho

Hi Johannes,

On 30/09/15 15:50, Johannes Schindelin wrote:

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

#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;
}

Hi Ramsay,

On Wed, 30 Sep 2015, Ramsay Jones wrote:

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

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;

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(-)

On Fri, Jan 8, 2016 at 11:21 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

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

Eric Sunshine <sunshine@sunshineco.com> writes:

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.

Hi Eric & Junio,

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

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

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

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

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(-)

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

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

Hi Junio,

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

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

Ciao, Dscho

Johannes Schindelin <schindelin@wisc.edu> writes:

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.

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

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

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

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.

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

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

On Fri, Jan 8, 2016 at 11:21 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

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.

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

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

    failed = 1;

And then simply:

    return failed;

NULL is tested here.

And also here. Is that intentional?

Ditto.

Hi Eric,

On Sat, 9 Jan 2016, Eric Sunshine wrote:

This, and ...

... this, and ...

... this and ...

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

Thanks, Dscho

On Fri, Jan 8, 2016 at 8:21 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Test fails on my Mac:

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

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

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

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.

Hi,

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

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

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

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     }
  };
  

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(-)

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(-)

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

On Mon, Jan 11, 2016 at 1:30 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

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;

Eric Sunshine <sunshine@sunshineco.com> writes:

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.

Hi Eric & Junio,

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

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

Ciao, Dscho

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

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

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;
  }

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(-)

Am 12.01.2016 um 08:57 schrieb Johannes Schindelin:

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:

Johannes Sixt <j6t@kdbg.org> writes:

This sounds good to me.  Dscho?

Hi Junio,

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

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

Am 23.01.2016 um 09:25 schrieb Johannes Schindelin:

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

Hi Hannes,

On Sat, 23 Jan 2016, Johannes Sixt wrote:

$ 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

Am 24.01.2016 um 11:56 schrieb Johannes Schindelin:

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(-)

Johannes Sixt <j6t@kdbg.org> writes:

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.

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(-)

Thanks.

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(-)

On 12/01/16 07:57, Johannes Schindelin wrote:

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

Hi Ramsay,

On Wed, 13 Jan 2016, Ramsay Jones wrote:

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

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

On 12/01/16 07:57, Johannes Schindelin wrote:

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;
}

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)

ATB, Ramsay Jones

Hi Ramsay,

On Wed, 13 Jan 2016, Ramsay Jones wrote:

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

On 13/01/16 07:40, Johannes Schindelin wrote:

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

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

On 12/01/16 07:57, Johannes Schindelin wrote:

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

Hi Ramsay,

On Wed, 13 Jan 2016, Ramsay Jones wrote:

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

Thanks, Dscho

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:

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)

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(-)

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

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.

On 01/13/2016 03:56 AM, Junio C Hamano wrote:

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 ?

Hi Torsten,

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

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

Hi Ramsay,

On Wed, 13 Jan 2016, Ramsay Jones wrote:

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

Hi Torsten,

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

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.

See the patch I sent this morning.

Ciao, Dscho

On 13/01/16 09:27, Johannes Schindelin wrote:

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

Exactly.

ATB, Ramsay Jones

On 13/01/16 05:47, Torsten Bögershausen wrote:

[snip]

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

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

And maybe not just Windows ...

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

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

ATB, Ramsay Jones

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

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

Hi Junio,

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

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

Ciao, Dscho