Re: [PATCH] Make locked paths absolute when current directory is changed

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 19, 2014, 12:40 UTC
Message-ID: <CACsJy8CpoOCw+3Q5AZk+cuXsUcoqW3hoHS9mX_=VuYtu8638+w@mail.gmail.com>
In-Reply-To: <xmqqmwc6mueh.fsf@gitster.dls.corp.google.com>

On Sat, Jul 19, 2014 at 12:47 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

>> +             abspath = absolute_path(lk->filename);
>> +             if (strlen(abspath) >= sizeof(lk->filename))
>> +                     warning("locked path %s is relative when current directory "
>> +                             "is changed", lk->filename);
>
> Shouldn't this be a die() or an error return (which will kill the
> caller anyway)?

We don't know for sure there will be a die() or something to trigger the roll back (or commit). If the chdir() is temporary, absolute path not fitting in PATH_MAX chars is not fatal because cwd will be reverted before commit/rollback. A better solution is probably avoid PATH_MAX in lk->filename. But yeah, changing it to die() is safer (especially when cwd is moved permanently for some options in update-index and read-tree)

Show 8 quoted lines

>> @@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
>>               die_errno("Unable to read current working directory");
>>       offset = len = strlen(cwd);
>>
>> +     make_locked_paths_absolute();
>
> Just being curious, but this early in the start-up sequence, what
> files do we have locks on?

We don't know. For most builtin commands, the setup is done early and we can be sure of no locks. Some commands (especially non-builtin) can still delay calling setup_git_directory() until later and they might do something in between, so better be safe than sorry.

-- 
Duy