Jeff King <peff@xxxxxxxx> writes: > This is actually a bit overengineered. Of the hundreds of calls to > git_config, there are exactly _two_ which check the return value. And > neither of them cares whether we parsed files or not; they really only > care if there was an error. So we could simply return 0 as long as there > is no error. > > This also makes me wonder, though. Git can do wildly different things > (including hard-to-reverse things) based on config (e.g., just consider > gc.pruneExpire). Yet we call git_config() without ever checking for > errors. In the actual parsing routines, we die() if there is an error. > But if we fail to open the file due to some transient error, we will > silently ignore the situation. > > Granted, such transient errors are unlikely. The biggest reasons for > failing to open a file are that it doesn't exist, or that we have no > permission to read it, both of which are treated explicitly in > git_config as "silently ok". But I wonder if we should simply be dying > on such an error, and git_config() should just have a void return. Thanks. ENOENT should be the same as having an empty file, which is the main point of the patch and at the same time why you feel this is overengineered. I agree with you on both counts. While I also agree that EACCESS and other failures should be treated as fatal in principle for safety, e.g. when running prune without being able to read gc.pruneExpire as you mentioned, we would need a tradeoff between safety and convenience. When asked to help looking at a complex merge in a colleages' repository, do you want your "git diff" to refuse to run only because her .git/config cannot be read by you? Of course, you _can_ work this particular one around by various means (e.g. prefixing GIT_CONFIG=... to force ignoring the file; telling the colleage that she'd better make her .git/config readable to you if she wants your help), if either one of the owner of the .git/config file or the party that wants to access the repository is a non-person such a workaround would be harder to arrange. Also there was a move going in the opposite direction to allow a config file that is syntactically broken to be handled without the parser dying, primarily to help "git config -e". In the longer term, our direction shouldn't be adding more die() in the git_config() callchain, but instead allowing it to report different kind of failures to let the caller act based on what it does and what the nature of failure is. For example, "gc" may say "I won't prune because I had to skip some of the lines in your .git/config because you have syntax errors in them, and I may have missed the definition of gc.pruneExpire you may wanted to place on them", while "diff" may ignore that kind of errors. Having said all that, my preference *for now* would be to ignore "there is no $HOME/.gitconfig (or /etc/gitconfig) file", but catch all other errors and die(). There are some other glitches in the current git_config() callchain. - No config file anywhere gives an error. I agree with you that this is a bug. - Having GIT_CONFIG=/no/such/file in the environment gives an error, which is good. - config.c::git_parse_file() [*1*] dies when it detects certain file format errors itself. This is not good for "git config -e", as it needs to learn core.editor before it can be used to fix such an error. This function then calls config.c::get_value() and it dies when config.c::get_value() reports any error. - config.c::get_value() is called by config.c::git_parse_file() to finish parsing out the <name, value> pair, and stores the "value" in a form usable in the code, e.g. a variable defined in environment.c. The function returns an error on some file format errors (e.g. a variable name is too long, string quoting unterminated) that signals the calling config.c::git_parse_file() to die(). These error returns are good (the caller however may need to be fixed for "config -e" issue not to die). It then calls the parse callbacck routines. They return error when they detect semantic errors (e.g. "branch.autosetupmerge = alwys" is not one of the valid values). The last one is not the topic of this patch, but it is quite problematic. When you are interested in finding out what value gc.pruneExpire is set, you do not care (as long as the configuration file was syntactically correct and you did not have to skip any file you were supposed to read due to EACCESS) if "branch.autosetupmerge" has an invalid value. A possible longer term solution would be to: - Change the signature of callbacks (e.g. git_default_branch_config()) so that they return void. They are not allowed to report any semantic errors while parsing. - Instead, they use special "INVALID" value and store that when they see a semantically incorrect value. They may also want to store what the actual string the config file gave them for later reporting, together with the name of and the line number in the config file for diagnostic purposes. - The user of the internalized value (i.e. "git grep git_branch_track" shows there are only two, cmd_branch() and cmd_checkout()) must check for the above INVALID value before they use the variable, and die at the point of the use. I'll send an illustration patch separately. [Footnote] *1* What a horrible name for this function! It is static so git_ prefix is unneeded, and if it anticipates it might get someday exported, parse_file is too generic and should be named git_parse_config_file(). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html