On Tue, Dec 6, 2016 at 3:16 AM, Brandon Williams <bmwill@xxxxxxxxxx> 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