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

From: Brandon Williams <bmwill@google.com>
Date: Dec 7, 2016, 00:10 UTC
Message-ID: <20161207001018.GD103573@google.com>
In-Reply-To: <xmqqtwagy65q.fsf@gitster.mtv.corp.google.com>

On 12/06, Junio C Hamano wrote:

Show 16 quoted lines

> Brandon Williams <bmwill@google.com> writes:
> 
> > +/* 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.

This function should accept both absolute and relative paths, which means it should probably accept "C:\My Files". I wasn't thinking about windows 100% of the time while writing this so I'm hoping that a windows expert will point things like this out to me :). So in reality this check shouldn't be (path->len > 1) but rather some function is_only_root which would check if the strbuf only contains a string which looks like root.

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

Yes it should handle paths like that sensibly.

Show 15 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 */

k will fix.

Show 13 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?

What exactly does "//" mean in this context? (I'm just naive in this area)

Show 26 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.

Yes I mentioned in the cover letter that this was the one last thing that needed to be changed to make it reentrant. I wanted to get this out for review sooner since this is the biggest change (getting rid of the chdir() calls) and dropping the static here would just require making the callers own the memory which should hopefully be an easy refactor. It just would have taken a bit more time to actually do that refactor now. I'm slowly working on it while this is being reviewed and could be ready to send out when rerolling this patch. Sorry for having a slightly misleading patch title :)

-- 
Brandon Williams