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
