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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 4, 2025, 16:21 UTC
Message-ID: <962654fc-02ea-47a9-a2ae-913101281240@gmail.com>
In-Reply-To: <xmqq5xbrwv4t.fsf@gitster.g>

On 03/11/2025 05:19, Junio C Hamano wrote:

Show 16 quoted lines

> "D. Ben Knoble" <ben.knoble+github@gmail.com> 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