Re: [PATCH v2 0/4] road to reentrant real_path

From: Brandon Williams <bmwill@google.com>
Date: Dec 9, 2016, 19:42 UTC
Message-ID: <20161209194232.GC88637@google.com>
In-Reply-To: <CACsJy8A2M_G34MeHh6vGsrf5ePOOduM6u=n17_EZLtu31uDAYg@mail.gmail.com>

On 12/09, Duy Nguyen wrote:

Show 13 quoted lines

> On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams <bmwill@google.com> wrote:
> > diff --git a/setup.c b/setup.c
> > index fe572b8..0d9fdd0 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> >                 if (!is_absolute_path(data.buf))
> >                         strbuf_addf(&path, "%s/", gitdir);
> >                 strbuf_addbuf(&path, &data);
> > -               strbuf_addstr(sb, real_path(path.buf));
> > +               strbuf_realpath(sb, path.buf, 1);
> 
> This is not the same because of this hunk in strbuf_realpath()

Then perhaps I shouldn't make this change (and just leave it as is) since the way real_path_internal/strbuf_realpath is written requires that the strbuf being used for the resolved path only contains the resolved path (see the lstat(resolved->buf &st) call). Sidenote it looks like strbuf_getcwd() also does a reset, though more subtlety, since it just passes its buffer to getcwd().

Show 19 quoted lines

> 
> > @@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, int die_on_error)
> >                         goto error_out;
> >         }
> >
> > -       strbuf_reset(&resolved);
> > +       strbuf_reset(resolved);
> >
> >         if (is_absolute_path(path)) {
> 
> But if you you remove that, then all (old) callers of
> strbuf_realpath() must do a strbuf_reset() in advance if needed
> (probably just real_path does) which sounds reasonable to me. You're
> probably want to be careful about the strbuf_reset() at the end of the
> function too.
> 
> Other than that, I think this diff looks nice.
> -- 
> Duy

-- 
Brandon Williams