Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 18, 2013, 15:58 UTC
Message-ID: <CAPig+cTLMKZ4MnG6YmKmqyFAZ0OgNLf83nOher_MqhPfDTDpLQ@mail.gmail.com>
In-Reply-To: <CALKQrgdMagUQMTGWX7RwBfBkA6+Q65aPfpk1W3m1BKMUNmjKFg@mail.gmail.com>

On Fri, Oct 18, 2013 at 10:52 AM, Johan Herland <johan@herland.net> wrote:

Show 20 quoted lines

> On Fri, Oct 18, 2013 at 3:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland <johan@herland.net> wrote:
>>> There are cases (e.g. when running concurrent fetches in a repo) where
>>> multiple Git processes concurrently attempt to create loose objects
>>> within the same objects/XX/ dir. The creation of the loose object files
>>> is (AFAICS) safe from races, but the creation of the objects/XX/ dir in
>>> which the loose objects reside is unsafe, for example:
>
> [...]
>
>>> The fix is to simply handle mkdir() setting errno = EEXIST as success.
>>
>> Would it make sense for the commit message to explain what happens if
>> EEXIST is returned but the pathname is not a directory? (For instance,
>> in the same race window, another process had created a plain file or
>> pipe there.)
>
> I'm pretty sure in that case, the following adjust_shared_perm() or
> git_mkstemp_mode() would fail, and we'd end up returning error from
> create_tmpfile() in any case.

>From a quick glance at the code, that was my impression as well.

> I can add the above to the commit message if you insist. :)

It was the only unanswered question which popped into my mind upon reading the commit message, and required code consultation to answer.

A benefit of mentioning it in the commit message is that future readers see that these other cases were taken into consideration (but I don't insist upon a re-roll).