# [PATCH 0/5] Fixes for :(optional) path code 15 messages from 2025-11-02 to 2025-11-06. Participants: D. Ben Knoble, Junio C Hamano, Phillip Wood. Thread: https://gitlist.dev/t/64421 ## D. Ben Knoble, 2025-11-02 16:17 Subject: [PATCH 0/5] Fixes for :(optional) path code Message-ID: URL: https://gitlist.dev/e/cover.1762100242.git.ben.knoble%2Bgithub%40gmail.com ``` 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 ``` ## D. Ben Knoble, 2025-11-02 16:17 Subject: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files Message-ID: <9ec696eaac647aa01466b101129da2b12ef5dbd5.1762100242.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/9ec696eaac647aa01466b101129da2b12ef5dbd5.1762100242.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` 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 Signed-off-by: D. Ben Knoble --- 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 ``` ## D. Ben Knoble, 2025-11-02 16:17 Subject: [PATCH 2/5] doc: clarify command equivalence comment Message-ID: <30908830025a3004fa34080793b610b5fe1eef9e.1762100242.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/30908830025a3004fa34080793b610b5fe1eef9e.1762100242.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` Documentation of command parsing for :(optional) includes a terse comment; expand it to be clearer to readers. Suggested-by: Phillip Wood Signed-off-by: D. Ben Knoble --- 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 ``` ## D. Ben Knoble, 2025-11-02 16:17 Subject: [PATCH 3/5] parseopt: use boolean type for a simple flag Message-ID: <10d531daf2c90d1bb53c07f1d72b087ebc1dd9c8.1762100242.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/10d531daf2c90d1bb53c07f1d72b087ebc1dd9c8.1762100242.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` 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; value = fix_filename(p->prefix, value); if (is_optional && is_missing_file(value)) { free((char *)value); -- 2.48.1 ``` ## D. Ben Knoble, 2025-11-02 16:17 Subject: [PATCH 4/5] config: use boolean type for a simple flag Message-ID: URL: https://gitlist.dev/e/dc4e2b9c65bc4fe29b4bebf9b0305b501bbfb9b1.1762100242.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` Suggested-by: Phillip Wood Signed-off-by: D. Ben Knoble --- 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 ``` ## D. Ben Knoble, 2025-11-02 16:17 Subject: [PATCH 5/5] parseopt: restore const qualifier to parsed filename Message-ID: URL: https://gitlist.dev/e/cedd4c3dbf448ae479bca109f74e1dd6f64cac72.1762100242.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` 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 Signed-off-by: D. Ben Knoble --- 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 ``` ## Junio C Hamano, 2025-11-03 05:19 Subject: Re: [PATCH 3/5] parseopt: use boolean type for a simple flag Message-ID: URL: https://gitlist.dev/e/xmqq5xbrwv4t.fsf%40gitster.g 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); ``` ## Phillip Wood, 2025-11-04 16:19 Subject: Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files Message-ID: URL: https://gitlist.dev/e/ec8d1764-649d-4edf-b0ae-a19ead5f6f9a%40gmail.com In-Reply-To: <9ec696eaac647aa01466b101129da2b12ef5dbd5.1762100242.git.ben.knoble+github@gmail.com> ``` 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 > Signed-off-by: D. Ben Knoble > --- > 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); ``` ## Phillip Wood, 2025-11-04 16:21 Subject: Re: [PATCH 3/5] parseopt: use boolean type for a simple flag Message-ID: <962654fc-02ea-47a9-a2ae-913101281240@gmail.com> URL: https://gitlist.dev/e/962654fc-02ea-47a9-a2ae-913101281240%40gmail.com In-Reply-To: ``` On 03/11/2025 05:19, Junio C Hamano wrote: > "D. Ben Knoble" 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 ``` ## Junio C Hamano, 2025-11-04 17:24 Subject: Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files Message-ID: URL: https://gitlist.dev/e/xmqq1pmdr9qu.fsf%40gitster.g In-Reply-To: ``` Phillip Wood 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, 2025-11-04 17:34 Subject: Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files Message-ID: URL: https://gitlist.dev/e/xmqqwm45puqr.fsf%40gitster.g In-Reply-To: ``` Junio C Hamano writes: > Phillip Wood 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 Signed-off-by: Junio C Hamano --- 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 ``` ## D. Ben Knoble, 2025-11-04 18:22 Subject: Re: [PATCH 3/5] parseopt: use boolean type for a simple flag Message-ID: URL: https://gitlist.dev/e/CALnO6CCLUfQyMb2DU5OXowvO6SLhdso4mOr4ukQe_A_kAL1t0Q%40mail.gmail.com In-Reply-To: <962654fc-02ea-47a9-a2ae-913101281240@gmail.com> ``` On Tue, Nov 4, 2025 at 11:21 AM Phillip Wood wrote: > > On 03/11/2025 05:19, Junio C Hamano wrote: > > "D. Ben Knoble" 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 ``` ## D. Ben Knoble, 2025-11-04 18:24 Subject: Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files Message-ID: URL: https://gitlist.dev/e/CALnO6CBgTu4Qtr8xwPCerrpqfAf02SV-bONLOUgq0X70eDThfw%40mail.gmail.com In-Reply-To: ``` On Tue, Nov 4, 2025 at 12:34 PM Junio C Hamano wrote: > > Junio C Hamano writes: > > > Phillip Wood 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 > Signed-off-by: Junio C Hamano > --- > 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. ``` ## Phillip Wood, 2025-11-05 16:35 Subject: Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files Message-ID: <5951a930-0e57-4201-9b56-12a41cb44333@gmail.com> URL: https://gitlist.dev/e/5951a930-0e57-4201-9b56-12a41cb44333%40gmail.com In-Reply-To: ``` On 04/11/2025 17:34, Junio C Hamano wrote: > Junio C Hamano 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 > Signed-off-by: Junio C Hamano > --- > 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); ``` ## Junio C Hamano, 2025-11-06 17:47 Subject: Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files Message-ID: URL: https://gitlist.dev/e/xmqqecqbhx3b.fsf%40gitster.g In-Reply-To: <5951a930-0e57-4201-9b56-12a41cb44333@gmail.com> ``` Phillip Wood writes: > On 04/11/2025 17:34, Junio C Hamano wrote: >> Junio C Hamano 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 >> Signed-off-by: Junio C Hamano >> --- >> 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); ```