On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote: > > Yes, though I don't think it's too bad. We already have a "die_on_error" > > flag in the config code. I think it just needs to become a tristate: > > die/error/silent (and probably get passed in via config_options, since I > > think we tie it right now to the file/blob source). > > Yes, but this code is already very crufty - and I'm just > waiting for someone to want to have per-repo/submodule > config parsing i... ;-) It is crufty, but I think we actually handle that part OK; the flag gets attached to the stack. > > Hmm, if we end up doing the config thing above, then this patch would > > become unnecessary. > > I was thinking of timing - the current patch could go > in quickly to solve the immediate problem (eg. in maint). > > Also, it does not hurt to do this _as well as_ suppress > the config errors. Yes, it can go in quickly. But I'd prefer not to keep it in the long term if it's literally doing nothing. I have some patches which I think solve your problem. They apply on v2.18.0, but not on v2.17.1 (because they rely on Dscho's increased passing of config_options in v2.18). Is that good enough? > > And I think doing that would help _all_ cases, even ones without a > > skiplist. They don't need to see random config error messages either, > > even if we do eventually report an fsck error. > > Oh, yes, I agree. You will have noticed that it was my > first suggested solution. (I have started writing that > patch a few times, but it just makes me want to throw > the current code away and start again)! > > Hmm, BTW, the 'rejected push' problem would include *any* > '.gitmodules' blob that contained a syntax error, right? > > Maybe it won't be as rare as all that! (Imagine not being > able to push due to a compiler error/warning in source files. > How irritating would that be! :-P ). Yes, it would include any syntax error. I also have a slight worry about that, but nobody seems to have screamed _yet_. :) > (if we fix this, you could hide some nefarious settings > after an obvious syntax error - then get the victim to > fix the syntax error ...) You can also usually get the victim to type "make", which is even simpler. :) Here are the patches I came up with. Note that the config_options struct has a bit of a dual-nature. Some options are respected only via config_from_options(), and some only from git_config_from_file(). I think this should probably be remedied in the long run, but I stopped here in the interest of keeping this maint-worthy. [1/4]: config: turn die_on_error into caller-facing enum [2/4]: config: add CONFIG_ERROR_SILENT handler [3/4]: config: add options parameter to git_config_from_mem [4/4]: fsck: silence stderr when parsing .gitmodules config.c | 32 +++++++++++++++++++++++--------- config.h | 13 +++++++++++-- fsck.c | 4 +++- submodule-config.c | 2 +- t/t7415-submodule-names.sh | 15 +++++++++++++++ 5 files changed, 53 insertions(+), 13 deletions(-) -Peff
