Re: [PATCH 3/5] parseopt: use boolean type for a simple flag
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 3, 2025, 05:19 UTC
- Message-ID
- <xmqq5xbrwv4t.fsf@gitster.g>
- In-Reply-To
- <10d531daf2c90d1bb53c07f1d72b087ebc1dd9c8.1762100242.git.ben.knoble+github@gmail.com>
"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
Show 25 quoted lines
> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
> 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);