Subject: Re: [PATCH 3/5] parseopt: use boolean type for a simple flag

From: Phillip Wood <[email-masked]>
Date: Tue, 4 Nov 2025 16:21:31 +0000

On 03/11/2025 05:19, Junio C Hamano wrote:
> "D. Ben Knoble" <[email-masked]> writes:
> 
>>   		is_optional = skip_prefix(value, ":(optional)", &value);
>>   		if (!value)
>> -			is_optional = 0;
>> +			is_optional = false;
> 
> Whether it is spelled 0 or false, I do not think this makes any
> sense.  skip_prefix() either touches &value to point at the
> substring in value that comes after ":(optional)", or it does not
> touch it at all, so there is no way value can be NULL here (and we
> know value is not NULL before we call skip_prefix()).
> 
> Shouldn't you be removing the entire "if value is NULL, it is not
> optional" thing instead?  That is exactly what Phillip pointed out
> in his review.

Looking at this again I wonder if the intention was to error out if 
there wasn't a filename after the ":(optional)" prefix which I think 
would be a reasonable thing to do but that's not what this code actually 
does.

Thanks

Phillip