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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 13, 2017, 17:06 UTC
Message-ID: <20170613170621.GG133952@aiede.mtv.corp.google.com>
In-Reply-To: <20170613025945.v54vrza2n23tk5pw@sigill.intra.peff.net>

Jeff King wrote:

> On Mon, Jun 12, 2017 at 06:38:17PM -0700, Jonathan Nieder wrote:
>> Brandon Williams wrote:
>>> On 06/12, Jonathan Nieder wrote:

Show 17 quoted lines

>>>> 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.
>> [...]
>>> And I don't know if I agree with renaming a function just to rename it.
>>
>> I forgot to say: another way to accomplish the same thing can be to
>> reorder the function's arguments.  The relevant thing is to make code
>> that calls the function without being aware of the new requirements
>> fail to compile.
>
> If the parameter is now required, then it might make sense for it to
> become an actual function parameter instead of being stuffed into the
> config_options struct. That would give you your breaking change, plus
> make it more obvious to the reader that it is not optional.

I like it. I also like that in your proposed patch the no longer necessary local git_dir goes away.

What's the next step? The discussion of get_git_common_dir() reveals that switching from git_path() to mkpathdup() loses the adjust_git_path() step and everything becomes complicated. So perhaps adding opts.git_common_dir and using that instead of git_path should happen as a preliminary patch, making this patch afterward a more straightforward refactoring.

Thanks, Jonathan