From: D. Ben Knoble <ben.knoble+github@gmail.com>
Date: Tue, 04 Nov 2025 18:22:49 GMT
Subject: Re: [PATCH 3/5] parseopt: use boolean type for a simple flag
Message-ID: <CALnO6CCLUfQyMb2DU5OXowvO6SLhdso4mOr4ukQe_A_kAL1t0Q@mail.gmail.com>
In-Reply-To: <962654fc-02ea-47a9-a2ae-913101281240@gmail.com>

On Tue, Nov 4, 2025 at 11:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 03/11/2025 05:19, Junio C Hamano wrote:
> > "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

Agreed both, will reroll