Provide 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 <[email protected]>

---

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

compat/basename.c +26 -0

index d8f8a3c..10dba38 100644
@@ -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;
+}

git-compat-util.h +0 -0

index f649e81..8b01aa5 100644

Quoted 7 lines · 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.

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

compat/basename.c +0 -0

index d8f8a3c..10dba38 100644
@@ -13,3 +13,29 @@ char *gitbasename (char *path)
 	}

compat/basename.c +5 -0

+	if (!slash)
+		return ".";
+	*slash = '\0';
+	return path;
+}

git-compat-util.h +2 -0

index f649e81..8b01aa5 100644
@@ -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

Quoted 14 lines · Johannes Schindelin

compat/basename.c +6 -0

index d8f8a3c..10dba38 100644
@@ -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. */

compat/basename.c char *gitbasename (char *path) · line 23

Quoted 2 lines

compat/basename.c +2 -0

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

compat/basename.c char *gitbasename (char *path) · line 26

Quoted 3 lines

compat/basename.c +3 -0

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

compat/basename.c char *gitbasename (char *path) · line 35

Quoted 9 lines

compat/basename.c +9 -0

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

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 <[email protected]>

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

Quoted 9 lines · Eric Sunshine

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.

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

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 compat/basename.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

compat/basename.c +17 -3

index 9f00421..0f1b0b0 100644
@@ -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;
 }

Quoted 3 lines · Johannes Schindelin

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?

Hi Junio,

Quoted 2 lines · Junio C Hamano

Johannes Schindelin <[email protected]> writes:

Quoted 5 lines

Quoted 3 lines

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

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 <[email protected]>

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

compat/basename.c +6 -0

index 0f1b0b0..0a2ed25 100644
@@ -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;

compat/basename.c +12 -0

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

compat/basename.c +3 -0

+	*slash = '\0';
+	return path;
+}

git-compat-util.h +2 -0

index 0d66f3a..94f311a 100644
@@ -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

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 <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---

test-path-utils.c +2 -0

+       { NULL,              NULL     }
+};

Quoted 7 lines · 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.

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 41

Quoted 8 lines

Derived-from-code-by Ramsay Jones <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---

test-path-utils.c +3 -0

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

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 51

Quoted 7 lines

test-path-utils.c +7 -0

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

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 64

Quoted 13 lines

test-path-utils.c +13 -0

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

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 67

Quoted 3 lines

test-path-utils.c +3 -0

+               }
+       }
+       return !!failed;

And then simply:

    return failed;

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 72

Quoted 5 lines

test-path-utils.c +5 -0

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

NULL is tested here.

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 135

Quoted 5 lines

test-path-utils.c +4 -0

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

And also here. Is that intentional?

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 <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t0060-path-utils.sh |   3 +
 test-path-utils.c     | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 171 insertions(+)

t/t0060-path-utils.sh +3 -0

index 627ef85..f0152a7 100755
@@ -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 ./ ""

test-path-utils.c +168 -1

index c67bf65..74e74c9 100644
@@ -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 [email protected]
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

Quoted 7 lines · Michael Blume

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.

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 <[email protected]>

---

compat/basename.c +32 -0

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

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;

Quoted 17 lines · Eric Sunshine

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

Quoted 8 lines

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

compat/mingw.h +0 -0

index 57ca477..b3e5044 100644
@@ -361,7 +361,15 @@ HANDLE winansi_get_osfhandle(int fd);
   * git specific compatibility
   */
 

I suggest to move the function definition out of line:

compat/mingw.c +7 -0

index 10a51c0..0cebb61 100644
@@ -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;

compat/mingw.h +0 -0

index 9b5db4e..2099b79 100644
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 
 #define has_dos_drive_prefix(path) \
 	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)

Quoted 26 lines · Johannes Sixt

I suggest to move the function definition out of line:

compat/mingw.c +7 -0

index 10a51c0..0cebb61 100644
@@ -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;

compat/mingw.h +0 -0

index 9b5db4e..2099b79 100644
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 

compat/mingw.h HANDLE winansi_get_osfhandle(int fd); · line 366

Quoted 12 lines

compat/mingw.h +1 -6

 #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?

Hi Junio,

Quoted 28 lines · Junio C Hamano

Quoted 26 lines · Johannes Sixt

I suggest to move the function definition out of line:

compat/mingw.c +7 -0

index 10a51c0..0cebb61 100644
@@ -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;

compat/mingw.h +0 -0

index 9b5db4e..2099b79 100644
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 

Quoted 14 lines

compat/mingw.h HANDLE winansi_get_osfhandle(int fd); · line 366

Quoted 12 lines

compat/mingw.h +1 -6

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

Am 23.01.2016 um 09:25 schrieb Johannes Schindelin:

Quoted 46 lines · Junio C Hamano

Quoted 30 lines

Quoted 27 lines · Johannes Sixt

I suggest to move the function definition out of line:

compat/mingw.c +7 -0

index 10a51c0..0cebb61 100644
@@ -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;

compat/mingw.h +0 -0

index 9b5db4e..2099b79 100644
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 

Quoted 14 lines

compat/mingw.h HANDLE winansi_get_osfhandle(int fd); · line 366

Quoted 12 lines

compat/mingw.h +1 -6

  #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?

Quoted 6 lines

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.

Am 24.01.2016 um 11:56 schrieb Johannes Schindelin:

Quoted 5 lines

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

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 <[email protected]>

---
 compat/basename.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

compat/basename.c +10 -3

index 9f00421..0f1b0b0 100644
@@ -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 {

Quoted 28 lines · Johannes Schindelin

compat/basename.c +16 -3

index 9f00421..0f1b0b0 100644
@@ -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))

compat/basename.c line 27

Quoted 5 lines

compat/basename.c +1 -0

+				*path = '\0';
 	}
 	return (char *)base;
 }

I don't suppose it makes much difference, but I find my version slightly easier to read:

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 <[email protected]>

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

compat/basename.c +34 -0

index 0f1b0b0..96bd953 100644
@@ -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;
+		}
+

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)

Hi Ramsay,

Quoted 8 lines · Ramsay Jones

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.

Quoted 1 line

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

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 <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t0060-path-utils.sh |   3 +
 test-path-utils.c     | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)

t/t0060-path-utils.sh +3 -0

index 627ef85..f0152a7 100755
@@ -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 ./ ""

test-path-utils.c +53 -0

index c67bf65..4ab68ac 100644
@@ -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)

Did you not have more tests to add?

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 77

Quoted 8 lines · Johannes Schindelin

test-path-utils.c +8 -0

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

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

Hi Torsten,

Quoted 12 lines · Torsten Bögershausen

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 77

Quoted 8 lines · Johannes Schindelin

test-path-utils.c +8 -0

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

Why the !defined(NO_LIBGEN_H)

Quoted 1 line

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.

On 01/12/2016 08:57 AM, Johannes Schindelin wrote:

test-path-utils.c static void normalize_argv_string(const char **var, const char *input) · line 77

Quoted 9 lines

test-path-utils.c +9 -0

+
+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 ?
And, in general, shouldn't all Windows version behave the same ?

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

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

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)

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

Quoted 12 lines · Ramsay Jones

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.

Quoted 5 lines

[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"?

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.

Quoted 6 lines · 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.

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.

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

Quoted 7 lines · Ramsay Jones

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.

Quoted 10 lines · Junio C Hamano

Quoted 8 lines · Ramsay Jones

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

Quoted 6 lines

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 ?