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

[Brandon Williams <bmwill@google.com>]

On 12/05, Stefan Beller wrote:
> > +/* 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?

There should always be a dir_sep since that only gets run if the passed
in path at least contains root '/'

> 
> > +/* 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' ?

True, I can update the comment to reflect that.

> > +       } 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.

K I can use the other msg.

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

Yeah I can push it down in scope.  There will be a bit more allocation
churn with the smaller scope but multiple symlinks should be rare?
Alternatively the 'next' buffer can be reused...I decided against that
initially due to readability.  And yes, lots of string manipulation
requires lots of strbufs :)

> > +       //strbuf_release(&resolved);
> 
> This is why the cover letter toned down expectations ?
> (no // as comment, maybe remove that line?)

yep.  It will be added back in though once the callers to real_path take
ownership of the memory.

-- 
Brandon Williams