Re: [PATCH v2] config.c: NULL check when reading protected config

From: Glen Choo <chooglen@google.com>
Date: Jul 27, 2022, 16:52 UTC
Message-ID: <kl6lczdqsdkz.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq1qu6txb0.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

> It however makes me wonder if it is simpler to allow passing NULL to
> git_config_from_file_with_options() and make it silently turn into a
> no-op.  I.e. instead of ...
>
>> @@ -1979,6 +1979,8 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
>>  	int ret = -1;
>>  	FILE *f;
>>  
>> +	if (!filename)
>> +		BUG("filename cannot be NULL");
>
> ... we could do
>
> 	if (!filename)
> 		return 0; /* successful no-op */
>
> Even if there are codepaths that feed arbitrary pathnames given by
> the end user, they wouldn't be passing NULL (they may pass an empty
> string, or a filename that causes fopen() to fail), would they?

Yeah, that's worth considering. I'm not sure how I feel about it yet, but hopefully I find some time to dig around and form an opinion.

Show 5 quoted lines

>
> But that is something we should leave to a follow-up series, not
> "oops, we need to fix it now" fix.
>
> Thanks, will queue.

Thanks :)

>
>>  	f = fopen_or_warn(filename, "r");
>>  	if (f) {
>>  		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,