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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> > - 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.

-Peff



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]