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

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Dec 6, 2016 at 3:16 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/05, Stefan Beller wrote:
>> >  static const char *real_path_internal(const char *path, int die_on_error)
>> >  {
>> > -       static struct strbuf sb = STRBUF_INIT;
>> > +       static struct strbuf resolved = STRBUF_INIT;
>>
>> Also by having this static here, it is not quite thread safe, yet.
>>
>> By removing the static here we cannot do the early cheap check as:
>>
>> >         /* We've already done it */
>> > -       if (path == sb.buf)
>> > +       if (path == resolved.buf)
>> >                 return path;
>>
>> I wonder how often we run into this case; are there some callers explicitly
>> relying on real_path_internal being cheap for repeated calls?
>> (Maybe run the test suite with this early return instrumented? Not sure how
>> to assess the impact of removing the cheap out return optimization)
>>
>> The long tail (i.e. the actual functionality) should actually be
>> faster, I'd imagine
>> as we do less than with using chdir.
>
> Depends on how expensive the chdir calls were.  And I'm working to get
> rid of the static buffer.  Just need have the callers own the memory
> first.

I suggest you turn this real_path_internal into strbuf_real_path. In
other words, it takes a strbuf and writes the result there, allocating
memory if needed.

This function can replace the two strbuf_addstr(..., real_path(..));
we have in setup.c and sha1_file.c. real_path() can own a static
strbuf buffer to retain current behavior. We could also have a new
wrapper real_pathdup() around strbuf_real_path(), which can replace 9
instances of xstrdup(real_path(...)) (and Stefan is adding a few more;
that's what led me back to these mails)
-- 
Duy