On Mon, Jun 12, 2017 at 02:53:52PM -0700, Brandon Williams wrote: > > These all seem reasonable to me. Patch 3 made me shrug a little, because > > it seems like the majority of C files end up including it anyway. I > > suspect you could break config.h down into two files: the few things > > that everybody needs (git_config() and the few parsing functions needed > > in callbacks) and the ones for commands that actually manipulate the > > config. > > > > That would reduce the surface area of the module that most callers look > > at, but I don't think there's a huge benefit to doing so (mostly it just > > makes re-compiling faster by decreasing the chance that a dependent > > header has changed for each file). > > Yes, ultimately I think it would be a good thing to break config.c down > into at least 2 more files (the file parsing logic and the config_set > logic) but that can be done at a later point. I started looking at > doing that now but that logic is a little more entangled than I thought > it was. To be clear, I don't mind that sort of module refactoring and like the results. But it almost certainly isn't the biggest bang-for-buck in terms of the time it takes versus the benefit it brings. So take my comment as "we could also do..." but not "we should not take your patch because it does not go far enough". -Peff