patch, 5 partsFixes for :(optional) path code

> Unlike the configuration option magic, the parseopt code also ignores
> empty files: compare implementations from ccfcaf399f (parseopt: values
> of pathname type can be prefixed with :(optional), 2025-09-28) and
> 749d6d166d (config: values of pathname type can be prefixed with
> :(optional), 2025-09-28).
> 
> Unify the 2 by not ignoring empty files, which is less surprising and
> the intended semantics from the first patch for config.
> 
> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
>   parse-options.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 5933468c19..6211b55a83 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -226,7 +226,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
>   		if (!value)
>   			is_optional = 0;
>   		value = fix_filename(p->prefix, value);
> -		if (is_optional && is_empty_or_missing_file(value)) {
> +		if (is_optional && is_missing_file(value)) {
>   			free((char *)value);
>   		} else {
>   			FREE_AND_NULL(*(char **)opt->value);

> Hi Ben
>
> These all look good to me though I agree with Junio's comments on patch 
> 3. It would be nice to get at least the fist patch merged in time for 
> 2.52.0.

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> Hi Ben
>>
>> These all look good to me though I agree with Junio's comments on patch 
>> 3. It would be nice to get at least the fist patch merged in time for 
>> 2.52.0.
>
> Yup, let me do exactly that ;-)
>
> Thanks, both.

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> >> Hi Ben
> >>
> >> These all look good to me though I agree with Junio's comments on patch
> >> 3. It would be nice to get at least the fist patch merged in time for
> >> 2.52.0.
> >
> > Yup, let me do exactly that ;-)
> >
> > Thanks, both.
>
> Let me have this on top of Ben's 5-patch series.
>
> ----- >8 -----
> Subject: [PATCH] parseopt: remove unreachable code
>
> At this point in the code after running skip_prefix() on the
> variable and receiving the result in the same variable, the contents
> of the variable can never be NULL.  The function either (1) updates
> the variable to point at a later part of the string it originally
> pointed at, or (2) leaves it intact if the string does not have the
> prefix.  (1) will never make the variable NULL, and (2) cannot be
> the source of NULL, because the variable cannot be NULL before
> calling skip_prefix(), which would die immediately by dereferencing
> the NULL pointer in that case.
>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  parse-options.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 27c1e75d53..97a55300e8 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
>                         return 0;
>
>                 is_optional = skip_prefix(value, ":(optional)", &value);
> -               if (!value)
> -                       is_optional = false;
>                 value = fix_filename(p->prefix, value);
>                 if (is_optional && is_missing_file(value)) {
>                         free((char *)value);
> --
> 2.52.0-rc0-28-g4cf919bd7b
>

> Junio C Hamano <gitster@pobox.com> writes:
> 
> Let me have this on top of Ben's 5-patch series.
> 
> ----- >8 -----
> Subject: [PATCH] parseopt: remove unreachable code
> 
> At this point in the code after running skip_prefix() on the
> variable and receiving the result in the same variable, the contents
> of the variable can never be NULL.  The function either (1) updates
> the variable to point at a later part of the string it originally
> pointed at, or (2) leaves it intact if the string does not have the
> prefix.  (1) will never make the variable NULL, and (2) cannot be
> the source of NULL, because the variable cannot be NULL before
> calling skip_prefix(), which would die immediately by dereferencing
> the NULL pointer in that case.

> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   parse-options.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 27c1e75d53..97a55300e8 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
>   			return 0;
>   
>   		is_optional = skip_prefix(value, ":(optional)", &value);
> -		if (!value)
> -			is_optional = false;
>   		value = fix_filename(p->prefix, value);
>   		if (is_optional && is_missing_file(value)) {
>   			free((char *)value);

> On 04/11/2025 17:34, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> Let me have this on top of Ben's 5-patch series.
>> 
>> ----- >8 -----
>> Subject: [PATCH] parseopt: remove unreachable code
>> 
>> At this point in the code after running skip_prefix() on the
>> variable and receiving the result in the same variable, the contents
>> of the variable can never be NULL.  The function either (1) updates
>> the variable to point at a later part of the string it originally
>> pointed at, or (2) leaves it intact if the string does not have the
>> prefix.  (1) will never make the variable NULL, and (2) cannot be
>> the source of NULL, because the variable cannot be NULL before
>> calling skip_prefix(), which would die immediately by dereferencing
>> the NULL pointer in that case.
>
> Nicely explained, the changes below look good
>
> Thanks
>
> Phillip

>
>> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>   parse-options.c | 2 --
>>   1 file changed, 2 deletions(-)
>> 
>> diff --git a/parse-options.c b/parse-options.c
>> index 27c1e75d53..97a55300e8 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
>>   			return 0;
>>   
>>   		is_optional = skip_prefix(value, ":(optional)", &value);
>> -		if (!value)
>> -			is_optional = false;
>>   		value = fix_filename(p->prefix, value);
>>   		if (is_optional && is_missing_file(value)) {
>>   			free((char *)value);

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

> "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.

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