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

[Brandon Williams <bmwill@google.com>]

On 06/14, Jeff King wrote:
> On Tue, Jun 13, 2017 at 02:38:15PM -0700, Brandon Williams wrote:
> 
> > > The same comments as before still apply:
> > > 
> > > - this changes API to make opts->git_dir mandatory, which is error prone
> > >   and easily avoidable, e.g. by making git_dir an argument to
> > >   git_config_with_options
> > 
> > I still don't agree with this.  I have looked at all callers and ensured
> > that 'git_dir' will be set when appropriate in the 'config_options'
> > struct.  I find the notion ridiculous that I would need to change a
> > function's name or arguments every time the internals of the function
> > are adjusted or when an options struct obtains a new field.  Plus, there
> > is already an aptly named parameter of type 'config_options' with which
> > to hold options for the config machinery.  This struct is also added to
> > in a later patch to include commondir so that the gitdir vs commondir
> > issue can be resolved.
> 
> I've already said "I'm OK either way for this case", but let me clarify
> a bit.
> 
> It's not about changing a function's internals, or even the struct
> obtaining a new field. The key change here is that the _interface_
> changed. Callers used to be able to pass NULL for the git_dir and have
> the function behave one way, and now if they do so it behaves
> differently. That leads to spooky action at a distance. Code which you
> didn't know about still compiles but does something subtly different.
> 
> We can catch that at the compile stage, or we can catch it at the test
> stage, or we can decide it's somebody else's problem to deal with if
> they wrote code that the rest of the project hasn't seen.
> 
> But it is a real thing that comes up in a big, open project. There is no
> "looked at all the callers", because you can't see the whole universe of
> code. I do think it's a much bigger deal in a project like the kernel,
> which has hundreds of long-lasting forks. Git has only a handful, and we
> don't necessarily need to bend over backwards for people whose code
> hasn't been shared.

At this point I'm done arguing, I'll rename the function and drop the
'git' prefix as that's the only sensible name I can think of.

> 
> > > - the commit message doesn't say anything about to git dir vs common dir
> > >   change.  It needs to, or even better, the switch to use common dir
> > >   instead of git dir can happen as a separate patch.
> > 
> > There really isn't any switching in this patch.  One of the following
> > patches in this series addresses this problem in more detail though.
> 
> I would have expected that patch to actually come earlier. That fixes
> the bug there, and then this conversion patch becomes that much more
> straightforward.

True, I can reorder the patches in v3 so that this change flows better.

-- 
Brandon Williams