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

[Stefan Beller <sbeller@google.com>]

On Mon, Dec 5, 2016 at 10:58 AM, Brandon Williams <bmwill@google.com> wrote:
> The current implementation of real_path uses chdir() in order to resolve
> symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
> process as a whole and not just an individual thread.  Instead perform
> the symlink resolution by hand so that the calls to chdir() can be
> removed, making real_path reentrant.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Thanks for working on this, some comments below:

>
> +/* 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);

What happens when there is no dir_sep?

> +/* gets the next component in 'remaining' and places it in 'next' */
> +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> +{

It's more than just getting it, it also chops it off of 'remaining' ?



> +       strbuf_reset(&resolved);
> +
> +       if (is_absolute_path(path)) {
> +               /* absolute path; start with only root as being resolved */
> +               strbuf_addch(&resolved, '/');
> +               strbuf_addstr(&remaining, path + 1);

This is where we would wait for input of Windows savy people.

> +       } else {
> +               /* relative path; can use CWD as the initial resolved path */
> +               if (strbuf_getcwd(&resolved)) {
> +                       if (die_on_error)
> +                               die_errno("Could not get current working directory");

I am looking at xgetcwd, which words it slightly differently.

    if (strbuf_getcwd(&sb))
        die_errno(_("unable to get current working directory"));

Not sure if aligning them would be a good idea?

Going by "git grep die_errno" as well as our Coding guidelines,
we don't want to see capitalized error messages.

> +
> +               if (next.len == 0) {
> +                       continue; /* empty component */

which means we resolve over path//with//double//slashes just fine,
as well as /./ parts. :)

>                 }
>
> -               if (sb.len) {
> -                       if (!cwd.len && strbuf_getcwd(&cwd)) {
> +               /* append the next component and resolve resultant path */

"resultant" indicates you have a math background. :)
But I had to look it up, I guess it is fine that way,
though "resulting" may cause less mental friction
for non native speakers.


> +                       if (!(errno == ENOENT && !remaining.len)) {
>                                 if (die_on_error)
> -                                       die_errno("Could not get current working directory");
> +                                       die_errno("Invalid path '%s'",
> +                                                 resolved.buf);
>                                 else
>                                         goto error_out;
>                         }
> +               } else if (S_ISLNK(st.st_mode)) {

As far as I can tell, we could keep the symlink strbuf
at a smaller scope here? (I was surprised how many strbufs
are declared at the beginning of the function)

> +       //strbuf_release(&resolved);

This is why the cover letter toned down expectations ?
(no // as comment, maybe remove that line?)

Thanks,
Stefan