From: Junio C Hamano Date: Mon, 03 Nov 2025 05:19:30 GMT Subject: Re: [PATCH 3/5] parseopt: use boolean type for a simple flag Message-ID: In-Reply-To: <10d531daf2c90d1bb53c07f1d72b087ebc1dd9c8.1762100242.git.ben.knoble+github@gmail.com> "D. Ben Knoble" writes: > Suggested-by: Phillip Wood > Signed-off-by: D. Ben Knoble > --- > parse-options.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/parse-options.c b/parse-options.c > index 6211b55a83..197c01987e 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -208,7 +208,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, > case OPTION_FILENAME: > { > const char *value; > - int is_optional; > + bool is_optional; > > if (unset) > value = NULL; > @@ -224,7 +224,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, > > 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. > value = fix_filename(p->prefix, value); > if (is_optional && is_missing_file(value)) { > free((char *)value);