Re: [PATCH 4/4] config: don't implicitly use gitdir

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 13, 2017, 01:33 UTC
Message-ID: <20170613013350.GD133952@aiede.mtv.corp.google.com>
In-Reply-To: <20170613012337.GH154599@google.com>

Brandon Williams wrote:

> On 06/12, Jonathan Nieder wrote:
>> Brandon Williams wrote:

Show 8 quoted lines

>>> Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
>>> not set up) added a 'git_dir' field to the config_options struct.  Let's
>>> use this option field explicitly all the time instead of occasionally
>>> falling back to calling 'git_pathdup("config")' to get the path to the
>>> local repository configuration.  This allows 'do_git_config_sequence()'
>>> to not implicitly rely on global repository state.
>>>
>>> Signed-off-by: Brandon Williams <bmwill@google.com>

[...]

Show 6 quoted lines

>> Unlike the previous 3, this one makes me pause for a moment: it means
>> that "gitdir:" includes and .git/config discovery would stop working
>> if the caller does not remember to set git_dir in their
>> config_options.
>>
>> So we have to inspect callers.

[...]

>> nit: because of the context, this 'if' can be "if (!nongit)".
>
> Will do.

Thanks.

[...]

Show 24 quoted lines

>>> +++ b/config.c
>>> @@ -219,8 +219,6 @@ static int include_by_gitdir(const struct config_options *opts,
>>>  
>>>  	if (opts->git_dir)
>>>  		git_dir = opts->git_dir;
>>> -	else if (have_git_dir())
>>> -		git_dir = get_git_dir();
>>>  	else
>>>  		goto done;
>>
>> I wonder if this should have a sanity-check:
>>
>> 	else if (have_git_dir())
>> 		BUG("caller forgot to set opts->git_dir");
>>
>> Alternatively, could this patch rename git_config_with_options?  That
>> way any other patch in flight that calls git_config_with_options would
>> conflict with this patch, giving us an opportunity to make sure it
>> also sets git_dir.  As another nice side benefit it would make it easy
>> for someone reading the patch to verify it didn't miss any callers.
>
> That kind of defeats the purpose of the patch.  The point is to not rely
> on global repository state and with the BUG statement we still are
> relying on global state.

By "that" are you referring to the BUG()? I agree --- I'd prefer renaming the function over adding a BUG() to catch mistaken old callers, so I shouldn't have mentioned the idea. I mentioned it to give another example of how to make this kind of change safely.

> And I don't know if I agree with renaming a function just to rename it.

It is not renaming just to rename: this is approximately the only safe way that the Git project has to make a significant breaking change to the API of a function. In a project where people push directly to the repository in a fast-forward way, it may be possible to make such a change by announcing it loudly and relying on patch authors keeping track of what is happening in the repository. But in a project like Git that juggles multiple patches in flight, some more explicit coordination, for example by renaming the identifier, is needed.

Sorry.

Do you have another method in mind?

Thanks, Jonathan