On 7/26/2022 1:43 PM, Taylor Blau wrote: > On Tue, Jul 26, 2022 at 10:40:18AM -0700, Glen Choo wrote: >>> I wonder: should it become a BUG() to call git_configset_add_file() with >>> a NULL filename? That would have elevated the test failure outside of >>> just the ASAn builds, I'd think. >>> >>> There's certainty a risk of being too defensive, but elevating this >>> error beyond just the ASan builds indicates that this would be an >>> appropriate layer of defense IMHO. >> >> Hm, if we're going in this direction, what if we made it a BUG() to call >> fopen_or_warn() with a NULL filename? Then we wouldn't have to >> reimplement this BUG() check in all of its callers. > > That may be too low-level of a place to put this check, but I don't have > a strong opinion about it either way (including whether we should have > such a BUG() *anywhere* in this series, including > git_configset_add_file()). Since git_configset_add_file() returns an 'int', could we return -1 if the supplied 'filename' was null? (The correct place to check would be down in git_config_from_file_with_options().) It would save all these checks here. (Also: do we care that we are ignoring the return values in read_protected_config()? Thanks, -Stolee
