Re: [PATCH] Provide a dirname() function when NO_LIBGEN_H=YesPlease
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2015, 18:24 UTC
- Message-ID
- <xmqq8u7n934i.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <25a2598e756959f55f06ae6b4dc6f448e3b6b127.1443624188.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 5 quoted lines
> 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.
Show 24 quoted lines
>
> I tested this implementation a bit, but I still would appreciate
> a few eye-balls to go over it.
>
> compat/basename.c | 26 ++++++++++++++++++++++++++
> git-compat-util.h | 2 ++
> 2 files changed, 28 insertions(+)
>
> diff --git a/compat/basename.c b/compat/basename.c
> index d8f8a3c..10dba38 100644
> --- a/compat/basename.c
> +++ b/compat/basename.c
> @@ -13,3 +13,29 @@ char *gitbasename (char *path)
> }
> return (char *)base;
> }
> +
> +char *gitdirname(char *path)
> +{
> + char *p = path, *slash, c;
> +
> + /* Skip over the disk name in MSDOS pathnames. */
> + if (has_dos_drive_prefix(p))
> + p += 2;Not a new problem, but many callers of has_dos_drive_prefix() hardcodes that "2" in various forms. I wonder if this is something we should relieve callers of by tweaking the semantics of it, e.g. by returning 2 (or howmanyever bytes should be skipped) from the function, changing it to skip_dos_drive_prefix(&p), etc.
> + /* POSIX.1-2001 says dirname("/") should return "/" */
> + slash = is_dir_sep(*p) ? ++p : NULL;
> + while ((c = *(p++)))I am confused by this. What is the invariant on 'p' at the beginning of the body of this while loop in each iteration?
Inside the body, p skips over dir-sep characters, so p must point at the byte past the last run of slashes?
If that is the invariant, upon entry, shouldn't the initialization of "slash" be skipping over all slashes, not just the first one, when the input is "///foo", for example? Instead the above skips '/' and sets slash to the byte past the first '/' (which is OK because you want to NUL-terminate to remove "//foo" from the input) but does not move p to 'f', so the invariant is not "p must point at the byte past the last run of slashes".
Show 9 quoted lines
> + 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.
Show 18 quoted lines
> + 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