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

[Junio C Hamano <gitster@pobox.com>]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

> 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);