[PATCH 0/5] Fixes for :(optional) path code

[PATCH 0/5] Fixes for :(optional) path code

[D. Ben Knoble]

This series has a few fixes from Phillip's review, set up as individual
patches. Justification is light, as many are hopefully straightforward?

The most important patch comes first. The rest are probably
take-or-leave.

D. Ben Knoble (5):
  parseopt: fix :(optional) at command line to only ignore missing files
  doc: clarify command equivalence comment
  parseopt: use boolean type for a simple flag
  config: use boolean type for a simple flag
  parseopt: restore const qualifier to parsed filename

 Documentation/gitcli.adoc | 2 +-
 config.c                  | 2 +-
 parse-options.c           | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)


base-commit: a99f379adf116d53eb11957af5bab5214915f91d
-- 
2.48.1

[PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files

[D. Ben Knoble]

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

Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files

[Phillip Wood]

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.

Thanks for following up on these

Phillip

On 02/11/2025 16:17, D. Ben Knoble wrote:
> 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);

Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files

[Junio C Hamano]

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.

Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files

[Junio C Hamano]

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

Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files

[D. Ben Knoble]

On Tue, Nov 4, 2025 at 12:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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
>

Ah, that's fine, and I won't send a new version unless we need it. Thanks!

I do wonder about Phillip's comment: perhaps it should have been
`!*value`? But then something else has to be fixed because value is no
longer ":(optional)" which maybe was intended as a literal filename?
IDK.

Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files

[Phillip Wood]

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

Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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

Thanks for a quick review.

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

[PATCH 2/5] doc: clarify command equivalence comment

[D. Ben Knoble]

Documentation of command parsing for :(optional) includes a terse
comment; expand it to be clearer to readers.

Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 Documentation/gitcli.adoc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gitcli.adoc b/Documentation/gitcli.adoc
index ef2a0a399d..6815d6bfb7 100644
--- a/Documentation/gitcli.adoc
+++ b/Documentation/gitcli.adoc
@@ -223,7 +223,7 @@ Options that take a filename allow a prefix `:(optional)`. For example:
 
 ----------------------------
 git commit -F :(optional)COMMIT_EDITMSG
-# if COMMIT_EDITMSG does not exist, equivalent to
+# if COMMIT_EDITMSG does not exist, the above is equivalent to
 git commit
 ----------------------------
 
-- 
2.48.1

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

[D. Ben Knoble]

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;
 		value = fix_filename(p->prefix, value);
 		if (is_optional && is_missing_file(value)) {
 			free((char *)value);
-- 
2.48.1

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

[Junio C Hamano]

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

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

[Phillip Wood]

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

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

[D. Ben Knoble]

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

[PATCH 4/5] config: use boolean type for a simple flag

[D. Ben Knoble]

Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 71b136bf7f..f1def0dcfb 100644
--- a/config.c
+++ b/config.c
@@ -1278,7 +1278,7 @@ int git_config_string(char **dest, const char *var, const char *value)
 
 int git_config_pathname(char **dest, const char *var, const char *value)
 {
-	int is_optional;
+	bool is_optional;
 	char *path;
 
 	if (!value)
-- 
2.48.1

[PATCH 5/5] parseopt: restore const qualifier to parsed filename

[D. Ben Knoble]

This was unintentionally dropped in ccfcaf399f (parseopt: values of
pathname type can be prefixed with :(optional), 2025-09-28). Notably,
continue dropping the const qualifier when free'ing value; see
4049b9cfc0 (fix const issues with some functions, 2007-10-16) or
83838d5c1b (cast variable in call to free() in builtin/diff.c and
submodule.c, 2011-11-06) for more details on why.

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 197c01987e..be3d8f6599 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -213,7 +213,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 		if (unset)
 			value = NULL;
 		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			value = (char *)opt->defval;
+			value = (const char *)opt->defval;
 		else {
 			int err = get_arg(p, opt, flags, &value);
 			if (err)
-- 
2.48.1