Re: [PATCH] real_path: make real_path thread-safe

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2016, 23:44 UTC
Message-ID: <xmqqtwagy65q.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1480964316-99305-2-git-send-email-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 8 quoted lines

> +/* removes the last path component from 'path' except if 'path' is root */
> +static void strip_last_component(struct strbuf *path)
> +{
> +	if (path->len > 1) {
> +		char *last_slash = find_last_dir_sep(path->buf);
> +		strbuf_setlen(path, last_slash - path->buf);
> +	}
> +}

You use find_last_dir_sep() which takes care of "Windows uses backslash" issue. Is this function expected to be fed something like "C:\My Files\foo.txt" and more importantly "C:\My Files"? Or is that handled by a lot higher level up in the callchain? I am reacting the comparison of path->len and 1 here.

Also is the input expected to be normalized? Are we expected to be fed something like "/a//b/./c/../d/e" and react sensibly, or is that handled by a lot higher level up in the callchain?

Show 12 quoted lines

> +/* gets the next component in 'remaining' and places it in 'next' */
> +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> +{
> +	char *start = NULL;
> +	char *end = NULL;
> +
> +	strbuf_reset(next);
> +
> +	/* look for the next component */
> +	/* Skip sequences of multiple path-separators */
> +	for (start = remaining->buf; is_dir_sep(*start); start++)
> +		/* nothing */;

Style:
		; /* nothing */

Show 5 quoted lines

> +	/* Find end of the path component */
> +	for (end = start; *end && !is_dir_sep(*end); end++)
> +		/* nothing */;
> +
> +	strbuf_add(next, start, end - start);

OK, so this was given "///foo/bar" in "remaining" and appended 'foo/' to "next". I.e. deduping of slashes is handled here.

POSIX cares about treating "//" at the very beginning of the path specially. Is that supposed to be handled here, or by a lot higher level up in the callchain?

Show 22 quoted lines

> +	/* remove the component from 'remaining' */
> +	strbuf_remove(remaining, 0, end - remaining->buf);
> +}
> +
>  /* We allow "recursive" symbolic links. Only within reason, though. */
> -#define MAXDEPTH 5
> +#define MAXSYMLINKS 5
>  
>  /*
>   * Return the real path (i.e., absolute path, with symlinks resolved
> @@ -21,7 +51,6 @@ int is_directory(const char *path)
>   * absolute_path().)  The return value is a pointer to a static
>   * buffer.
>   *
>   * The directory part of path (i.e., everything up to the last
>   * dir_sep) must denote a valid, existing directory, but the last
>   * component need not exist.  If die_on_error is set, then die with an
> @@ -33,22 +62,16 @@ int is_directory(const char *path)
>   */
>  static const char *real_path_internal(const char *path, int die_on_error)
>  {
> +	static struct strbuf resolved = STRBUF_INIT;

This being 'static' would probably mean that this is not reentrant, which goes against the title of the patch.