Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 28, 2018, 18:45 UTC
Message-ID: <CACsJy8COj-usWbJzb_CRF95an94QhpTcc+uv-VDxmPbDfH+o6g@mail.gmail.com>
In-Reply-To: <20180328183011.GA16931@sigill.intra.peff.net>

On Wed, Mar 28, 2018 at 8:30 PM, Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

> On Wed, Mar 28, 2018 at 07:55:35PM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> From: Duy Nguyen <pclouds@gmail.com>
>>
>> As noted in the previous patch, when $CWD is moved, we recognize the
>> problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR
>> with new ones.
>>
>> We have plenty more environment variables that can contain paths
>> though. If they are read and cached before setup_work_tree() is
>> called, nobody will update them and they become bad paths.
>
> Hmm, yeah, I missed these. It would be interesting to know if there are
> easy-to-run test cases that show off these bugs, or if they're
> hypothetical. (Even if they are hypothetical, I'm not opposed to fixing
> them in the name of maintainability).

It's kinda hard to show off these. But the GIT_ALTERNATE_OBJ.. variable could be one example in favor of this fix. Before, we lazily read the env var which is most likely after setup_work_tree() has run. After a bunch of code reorganization and stuff, GIT_ALTERNATE_OBJ is now read very early, which should be safe (why not?) but it actually breaks. Alternate db is only queried as the last resort if I'm not mistaken, so 90% of time you just hit an object in odb and never find out that your GIT_ALTERNATE_OBJ... points to nowhere.

Show 24 quoted lines

>> diff --git a/environment.c b/environment.c
>> index 39b3d906c8..f9dcc1b99e 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -128,6 +128,20 @@ const char * const local_repo_env[] = {
>>       NULL
>>  };
>>
>> +/* A subset of local_repo_env[] that contains path */
>> +const char * const local_repo_path_env[] = {
>> +     ALTERNATE_DB_ENVIRONMENT,
>> +     CONFIG_ENVIRONMENT,
>> +     DB_ENVIRONMENT,
>> +     GIT_COMMON_DIR_ENVIRONMENT,
>> +     GIT_DIR_ENVIRONMENT,
>> +     GIT_SHALLOW_FILE_ENVIRONMENT,
>> +     GIT_WORK_TREE_ENVIRONMENT,
>> +     GRAFT_ENVIRONMENT,
>> +     INDEX_ENVIRONMENT,
>> +     NULL
>> +};
>
> It might be nice to fold this list into local_repo_env automatically. I
> think you'd have to do it with a macro.

Aha! I did not like the split either and wanted to turn local_repo_env[] to an array of struct so we can add attributes to each variable, but the way local_repo_env[] is being used, that's impossible without more surgery.

> OTOH, it's possible that there could be a path-related environment
> variable that _isn't_ actually part of local_repo_env. E.g., I think
> GIT_CONFIG might classify there (though I don't know if it's worth
> worrying about).

I'd rather fix it now and forget about it than trying to troubleshoot a problem related to bad relative $GIT_CONFIG (even if the chance of that happening is probably 1%)

Show 15 quoted lines

>> +static void update_path_envs(const char *old_cwd, const char *new_cwd,
>> +                          void *cb)
>> +{
>> +     int i;
>> +
>> +     /*
>> +      * FIXME: special treatment needed for
>> +      * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain
>> +      * multiple paths.
>> +      */
>
> Yuck. It just keeps getting more complicated. :(
>
> I do wonder if relative paths in variables like this are worth worrying
> about. AFAIK, it's always been a "well, it kind of works" situation, but

Yeah. 99% of time $GIT_DIR is $GIT_WORK_TREE/.git and no path adjustment is needed.

Show 5 quoted lines

> not something we've tried to actively support. I think with the current
> code you'd potentially get inconsistent results between a command which
> sets up the work tree and one which doesn't. So this would be fixing
> that, but at the same time, I'm not sure how much we want to promise
> here.

I would be just as happy to die() when we find out we have relative paths that takes too much work to reparent. It keeps the amount of work down and will not bite us later (and will let us know if any user needs it because they would have to report back after hitting the said die()).

-- 
Duy