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

[Junio C Hamano]

Johannes Schindelin  writes:

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

Thanks for being careful.

>
>   I tested this implementation a bit, but I still would appreciate
>   a few eye-balls to go over it.
>
>  compat/basename.c | 26 ++
>  git-compat-util.h |  2 ++
>  2 files changed, 28 insertions(+)
>
> diff --git a/compat/basename.c b/compat/basename.c
> index d8f8a3c..10dba38 100644
> --- a/compat/basename.c
> +++ b/compat/basename.c
> @@ -13,3 +13,29 @@ char *gitbasename (char *path)
>   }
>   return (char *)base;
>  }
> +
> +char *gitdirname(char *path)
> +{
> + char *p = path, *slash, c;
> +
> + /* Skip over the disk name in MSDOS pathnames. */
> + if (has_dos_drive_prefix(p))
> + p += 2;

Not a new problem, but many callers of has_dos_drive_prefix()
hardcodes that "2" in various forms.  I wonder if this is something
we should relieve callers of by tweaking the semantics of it, e.g.
by returning 2 (or howmanyever bytes should be skipped) from the
function, changing it to skip_dos_drive_prefix(), etc.

> + /* POSIX.1-2001 says dirname("/") should return "/" */
> + slash = is_dir_sep(*p) ? ++p : NULL;
> + while ((c = *(p++)))

I am confused by this.  What is the invariant on 'p' at the
beginning of the body of this while loop in each iteration?

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

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

> + if (is_dir_sep(c)) {
> + char *tentative = p - 1;
> +
> + /* POSIX.1-2001 says to ignore trailing slashes */
> + while (is_dir_sep(*p))
> + p++;
> + if (*p)
> + slash = tentative;
> + }

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

> + if (!slash)
> + return ".";
> + *slash = '\0';
> + return path;
> +}
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f649e81..8b01aa5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -253,6 +253,8 @@ struct itimerval {
>  #else
>  #define basename gitbasename
>  extern char *gitbasename(char *);
> +#define dirname gitdirname
> +extern char *gitdirname(char *);
>  #endif
>  
>  #ifndef NO_ICONV
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ramsay Jones]

Hi Johannes,

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

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

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

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

HTH

Ramsay Jones


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

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

[Johannes Schindelin]

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

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

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

Signed-off-by: Johannes Schindelin 
---

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

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

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

diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..10dba38 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -13,3 +13,29 @@ char *gitbasename (char *path)
}
return (char *)base;
 }
+
+char *gitdirname(char *path)
+{
+   char *p = path, *slash, c;
+
+   /* Skip over the disk name in MSDOS pathnames. */
+   if (has_dos_drive_prefix(p))
+   p += 2;
+   /* POSIX.1-2001 says dirname("/") should return "/" */
+   slash = is_dir_sep(*p) ? ++p : NULL;
+   while ((c = *(p++)))
+   if (is_dir_sep(c)) {
+   char *tentative = p - 1;
+
+   /* POSIX.1-2001 says to ignore trailing slashes */
+   while (is_dir_sep(*p))
+   p++;
+   if (*p)
+   slash = tentative;
+   }
+
+   if (!slash)
+   return ".";
+   *slash = '\0';
+   return path;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index f649e81..8b01aa5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -253,6 +253,8 @@ struct itimerval {
 #else
 #define basename gitbasename
 extern char *gitbasename(char *);
+#define dirname gitdirname
+extern char *gitdirname(char *);
 #endif
 
 #ifndef NO_ICONV
-- 
2.5.3.windows.1.3.gc322723


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html