Re: [PATCH v3 02/25] Convert git_snpath() to strbuf_git_path()

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 20, 2014, 03:55 UTC
Message-ID: <CACsJy8DB+OP6_vwZKWXjq4YbtiKDg2NE2zRsAuz1ER6oTmyGag@mail.gmail.com>
In-Reply-To: <xmqqa9dm78n3.fsf@gitster.dls.corp.google.com>

On Thu, Feb 20, 2014 at 10:41 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>>>> -             }
>>>> +             if (old->path && old->name &&
>>>> +                 !file_exists(git_path("%s", old->path)) &&
>>>> +                  file_exists(git_path("logs/%s", old->path)))
>>>> +                     remove_path(git_path("logs/%s", old->path));
>>>
>>> Hmph.  Is this conversion safe?
>>>
>>> This adds three uses of the round-robin path buffer; if a caller of
>>> this function used two or more path buffers obtained from
>>> get_pathname() and expected their contents to remain stable across
>>> the call to this, it will silently break.
>>
>> three round-robin buffers but not all required at the same time, once
>> the first file_exists() returns the first round-robin buffer could be
>> free, and remove_path() calls git_path again, not reusing the result
>> from the second file_exists, so the second buffer is free to go too.
>
> I know these three callers to git_path() will not step on each
> other's toes.  But that is not the question I asked.

OK so your question was if there was a git_path() or mkpath() call earlier in update_refs_for_switch() and the result was expected to remain stable till the end of update_refs_for_switch(), then this conversion could ruin it, correct? I didn't think about that, but I have checked and the only mkpath() call in this function is not supposed to last long. If it's about a git_pathname() call outside update_refs_...() that still expects to be stable, we should fix that code instead.

-- 
Duy