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