From aece3bc266d3edf3a2710799876ce538561b5fba Mon Sep 17 00:00:00 2001 From: "D. Ben Knoble" Date: Sun, 2 Nov 2025 11:17:44 -0500 Subject: [PATCH 1/6] parseopt: fix :(optional) at command line to only ignore missing files 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 Signed-off-by: Junio C Hamano --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 4faf66023a..5b7dc29b2c 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); From 2fd151af1393fb8b380820ef6d64a5241258a0b0 Mon Sep 17 00:00:00 2001 From: "D. Ben Knoble" Date: Sun, 2 Nov 2025 11:17:45 -0500 Subject: [PATCH 2/6] doc: clarify command equivalence comment 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 Signed-off-by: Junio C Hamano --- 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 ---------------------------- From 4da5bebc17518bace2a15484f345c3494c120135 Mon Sep 17 00:00:00 2001 From: "D. Ben Knoble" Date: Sun, 2 Nov 2025 11:17:46 -0500 Subject: [PATCH 3/6] parseopt: use boolean type for a simple flag Suggested-by: Phillip Wood Signed-off-by: D. Ben Knoble Signed-off-by: Junio C Hamano --- parse-options.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/parse-options.c b/parse-options.c index 5b7dc29b2c..b105e4b6cf 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); From 4dbb7f4f820ca392699bac884311e2c9204cddcb Mon Sep 17 00:00:00 2001 From: "D. Ben Knoble" Date: Sun, 2 Nov 2025 11:17:47 -0500 Subject: [PATCH 4/6] config: use boolean type for a simple flag Suggested-by: Phillip Wood Signed-off-by: D. Ben Knoble Signed-off-by: Junio C Hamano --- config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.c b/config.c index 73fc74c8fa..d105bbc86b 100644 --- a/config.c +++ b/config.c @@ -1279,7 +1279,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) From 383e5e1c4bfa604bcd479100258b4ff354dbaabb Mon Sep 17 00:00:00 2001 From: "D. Ben Knoble" Date: Sun, 2 Nov 2025 11:17:48 -0500 Subject: [PATCH 5/6] parseopt: restore const qualifier to parsed filename 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 Signed-off-by: Junio C Hamano --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index b105e4b6cf..27c1e75d53 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) From a2584d04344b93610ee9e958d477d743380fc8d7 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 4 Nov 2025 09:34:20 -0800 Subject: [PATCH 6/6] 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);