{"thread":{"id":"64421","subject":"[PATCH 0/5] Fixes for :(optional) path code","startedAt":"2025-11-02T16:18:12Z","lastAt":"2025-11-06T17:47:39Z","messageCount":15,"participants":["D. Ben Knoble","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"530075","messageId":"cover.1762100242.git.ben.knoble+github@gmail.com","threadId":"64421","inReplyTo":null,"subject":"[PATCH 0/5] Fixes for :(optional) path code","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-02T16:17:43Z","receivedAt":"2025-11-02T16:18:12Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"This series has a few fixes from Phillip's review, set up as individual\npatches. Justification is light, as many are hopefully straightforward?\n\nThe most important patch comes first. The rest are probably\ntake-or-leave.\n\nD. Ben Knoble (5):\n parseopt: fix :(optional) at command line to only ignore missing files\n doc: clarify command equivalence comment\n parseopt: use boolean type for a simple flag\n config: use boolean type for a simple flag\n parseopt: restore const qualifier to parsed filename\n\n Documentation/gitcli.adoc | 2 +-\n config.c | 2 +-\n parse-options.c | 8 ++++----\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\n\nbase-commit: a99f379adf116d53eb11957af5bab5214915f91d\n-- \n2.48.1\n\n"},{"id":"530076","messageId":"9ec696eaac647aa01466b101129da2b12ef5dbd5.1762100242.git.ben.knoble+github@gmail.com","threadId":"64421","inReplyTo":"cover.1762100242.git.ben.knoble+github@gmail.com","subject":"[PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-02T16:17:44Z","receivedAt":"2025-11-02T16:18:15Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Unlike the configuration option magic, the parseopt code also ignores\nempty files: compare implementations from ccfcaf399f (parseopt: values\nof pathname type can be prefixed with :(optional), 2025-09-28) and\n749d6d166d (config: values of pathname type can be prefixed with\n:(optional), 2025-09-28).\n\nUnify the 2 by not ignoring empty files, which is less surprising and\nthe intended semantics from the first patch for config.\n\nSuggested-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n parse-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 5933468c19..6211b55a83 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -226,7 +226,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\tif (!value)\n \t\t\tis_optional = 0;\n \t\tvalue = fix_filename(p->prefix, value);\n-\t\tif (is_optional && is_empty_or_missing_file(value)) {\n+\t\tif (is_optional && is_missing_file(value)) {\n \t\t\tfree((char *)value);\n \t\t} else {\n \t\t\tFREE_AND_NULL(*(char **)opt->value);\n-- \n2.48.1\n\n"},{"id":"530077","messageId":"30908830025a3004fa34080793b610b5fe1eef9e.1762100242.git.ben.knoble+github@gmail.com","threadId":"64421","inReplyTo":"cover.1762100242.git.ben.knoble+github@gmail.com","subject":"[PATCH 2/5] doc: clarify command equivalence comment","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-02T16:17:45Z","receivedAt":"2025-11-02T16:18:16Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Documentation of command parsing for :(optional) includes a terse\ncomment; expand it to be clearer to readers.\n\nSuggested-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n Documentation/gitcli.adoc | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitcli.adoc b/Documentation/gitcli.adoc\nindex ef2a0a399d..6815d6bfb7 100644\n--- a/Documentation/gitcli.adoc\n+++ b/Documentation/gitcli.adoc\n@@ -223,7 +223,7 @@ Options that take a filename allow a prefix `:(optional)`. For example:\n \n ----------------------------\n git commit -F :(optional)COMMIT_EDITMSG\n-# if COMMIT_EDITMSG does not exist, equivalent to\n+# if COMMIT_EDITMSG does not exist, the above is equivalent to\n git commit\n ----------------------------\n \n-- \n2.48.1\n\n"},{"id":"530078","messageId":"10d531daf2c90d1bb53c07f1d72b087ebc1dd9c8.1762100242.git.ben.knoble+github@gmail.com","threadId":"64421","inReplyTo":"cover.1762100242.git.ben.knoble+github@gmail.com","subject":"[PATCH 3/5] parseopt: use boolean type for a simple flag","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-02T16:17:46Z","receivedAt":"2025-11-02T16:18:19Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Suggested-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n parse-options.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 6211b55a83..197c01987e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -208,7 +208,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \tcase OPTION_FILENAME:\n \t{\n \t\tconst char *value;\n-\t\tint is_optional;\n+\t\tbool is_optional;\n \n \t\tif (unset)\n \t\t\tvalue = NULL;\n@@ -224,7 +224,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \n \t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n \t\tif (!value)\n-\t\t\tis_optional = 0;\n+\t\t\tis_optional = false;\n \t\tvalue = fix_filename(p->prefix, value);\n \t\tif (is_optional && is_missing_file(value)) {\n \t\t\tfree((char *)value);\n-- \n2.48.1\n\n"},{"id":"530079","messageId":"dc4e2b9c65bc4fe29b4bebf9b0305b501bbfb9b1.1762100242.git.ben.knoble+github@gmail.com","threadId":"64421","inReplyTo":"cover.1762100242.git.ben.knoble+github@gmail.com","subject":"[PATCH 4/5] config: use boolean type for a simple flag","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-02T16:17:47Z","receivedAt":"2025-11-02T16:18:21Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Suggested-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 71b136bf7f..f1def0dcfb 100644\n--- a/config.c\n+++ b/config.c\n@@ -1278,7 +1278,7 @@ int git_config_string(char **dest, const char *var, const char *value)\n \n int git_config_pathname(char **dest, const char *var, const char *value)\n {\n-\tint is_optional;\n+\tbool is_optional;\n \tchar *path;\n \n \tif (!value)\n-- \n2.48.1\n\n"},{"id":"530080","messageId":"cedd4c3dbf448ae479bca109f74e1dd6f64cac72.1762100242.git.ben.knoble+github@gmail.com","threadId":"64421","inReplyTo":"cover.1762100242.git.ben.knoble+github@gmail.com","subject":"[PATCH 5/5] parseopt: restore const qualifier to parsed filename","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-02T16:17:48Z","receivedAt":"2025-11-02T16:18:22Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"This was unintentionally dropped in ccfcaf399f (parseopt: values of\npathname type can be prefixed with :(optional), 2025-09-28). Notably,\ncontinue dropping the const qualifier when free'ing value; see\n4049b9cfc0 (fix const issues with some functions, 2007-10-16) or\n83838d5c1b (cast variable in call to free() in builtin/diff.c and\nsubmodule.c, 2011-11-06) for more details on why.\n\nSuggested-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n parse-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 197c01987e..be3d8f6599 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -213,7 +213,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\tif (unset)\n \t\t\tvalue = NULL;\n \t\telse if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n-\t\t\tvalue = (char *)opt->defval;\n+\t\t\tvalue = (const char *)opt->defval;\n \t\telse {\n \t\t\tint err = get_arg(p, opt, flags, &value);\n \t\t\tif (err)\n-- \n2.48.1\n\n"},{"id":"530092","messageId":"xmqq5xbrwv4t.fsf@gitster.g","threadId":"64421","inReplyTo":"10d531daf2c90d1bb53c07f1d72b087ebc1dd9c8.1762100242.git.ben.knoble+github@gmail.com","subject":"Re: [PATCH 3/5] parseopt: use boolean type for a simple flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-03T05:19:30Z","receivedAt":"2025-11-03T05:19:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> Suggested-by: Phillip Wood \n> Signed-off-by: D. Ben Knoble \n> ---\n> parse-options.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index 6211b55a83..197c01987e 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -208,7 +208,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n> \tcase OPTION_FILENAME:\n> \t{\n> \t\tconst char *value;\n> -\t\tint is_optional;\n> +\t\tbool is_optional;\n> \n> \t\tif (unset)\n> \t\t\tvalue = NULL;\n> @@ -224,7 +224,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n> \n> \t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n> \t\tif (!value)\n> -\t\t\tis_optional = 0;\n> +\t\t\tis_optional = false;\n\nWhether it is spelled 0 or false, I do not think this makes any\nsense. skip_prefix() either touches &value to point at the\nsubstring in value that comes after \":(optional)\", or it does not\ntouch it at all, so there is no way value can be NULL here (and we\nknow value is not NULL before we call skip_prefix()).\n\nShouldn't you be removing the entire \"if value is NULL, it is not\noptional\" thing instead? That is exactly what Phillip pointed out\nin his review.\n\n> \t\tvalue = fix_filename(p->prefix, value);\n> \t\tif (is_optional && is_missing_file(value)) {\n> \t\t\tfree((char *)value);\n"},{"id":"530202","messageId":"ec8d1764-649d-4edf-b0ae-a19ead5f6f9a@gmail.com","threadId":"64421","inReplyTo":"9ec696eaac647aa01466b101129da2b12ef5dbd5.1762100242.git.ben.knoble+github@gmail.com","subject":"Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-04T16:19:31Z","receivedAt":"2025-11-04T16:19:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nThese all look good to me though I agree with Junio's comments on patch \n3. It would be nice to get at least the fist patch merged in time for \n2.52.0.\n\nThanks for following up on these\n\nPhillip\n\nOn 02/11/2025 16:17, D. Ben Knoble wrote:\n> Unlike the configuration option magic, the parseopt code also ignores\n> empty files: compare implementations from ccfcaf399f (parseopt: values\n> of pathname type can be prefixed with :(optional), 2025-09-28) and\n> 749d6d166d (config: values of pathname type can be prefixed with\n> :(optional), 2025-09-28).\n> \n> Unify the 2 by not ignoring empty files, which is less surprising and\n> the intended semantics from the first patch for config.\n> \n> Suggested-by: Phillip Wood \n> Signed-off-by: D. Ben Knoble \n> ---\n> parse-options.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 5933468c19..6211b55a83 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -226,7 +226,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n> \t\tif (!value)\n> \t\t\tis_optional = 0;\n> \t\tvalue = fix_filename(p->prefix, value);\n> -\t\tif (is_optional && is_empty_or_missing_file(value)) {\n> +\t\tif (is_optional && is_missing_file(value)) {\n> \t\t\tfree((char *)value);\n> \t\t} else {\n> \t\t\tFREE_AND_NULL(*(char **)opt->value);\n\n"},{"id":"530203","messageId":"962654fc-02ea-47a9-a2ae-913101281240@gmail.com","threadId":"64421","inReplyTo":"xmqq5xbrwv4t.fsf@gitster.g","subject":"Re: [PATCH 3/5] parseopt: use boolean type for a simple flag","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-04T16:21:31Z","receivedAt":"2025-11-04T16:21:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 03/11/2025 05:19, Junio C Hamano wrote:\n> \"D. Ben Knoble\" writes:\n> \n>> \t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n>> \t\tif (!value)\n>> -\t\t\tis_optional = 0;\n>> +\t\t\tis_optional = false;\n> \n> Whether it is spelled 0 or false, I do not think this makes any\n> sense. skip_prefix() either touches &value to point at the\n> substring in value that comes after \":(optional)\", or it does not\n> touch it at all, so there is no way value can be NULL here (and we\n> know value is not NULL before we call skip_prefix()).\n> \n> Shouldn't you be removing the entire \"if value is NULL, it is not\n> optional\" thing instead? That is exactly what Phillip pointed out\n> in his review.\n\nLooking at this again I wonder if the intention was to error out if \nthere wasn't a filename after the \":(optional)\" prefix which I think \nwould be a reasonable thing to do but that's not what this code actually \ndoes.\n\nThanks\n\nPhillip\n\n"},{"id":"530206","messageId":"xmqq1pmdr9qu.fsf@gitster.g","threadId":"64421","inReplyTo":"ec8d1764-649d-4edf-b0ae-a19ead5f6f9a@gmail.com","subject":"Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-04T17:24:57Z","receivedAt":"2025-11-04T17:25:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Ben\n>\n> These all look good to me though I agree with Junio's comments on patch \n> 3. It would be nice to get at least the fist patch merged in time for \n> 2.52.0.\n\nYup, let me do exactly that ;-)\n\nThanks, both.\n"},{"id":"530207","messageId":"xmqqwm45puqr.fsf@gitster.g","threadId":"64421","inReplyTo":"xmqq1pmdr9qu.fsf@gitster.g","subject":"Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-04T17:34:20Z","receivedAt":"2025-11-04T17:34:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Phillip Wood writes:\n>\n>> Hi Ben\n>>\n>> These all look good to me though I agree with Junio's comments on patch \n>> 3. It would be nice to get at least the fist patch merged in time for \n>> 2.52.0.\n>\n> Yup, let me do exactly that ;-)\n>\n> Thanks, both.\n\nLet me have this on top of Ben's 5-patch series.\n\n----- >8 -----\nSubject: [PATCH] parseopt: remove unreachable code\n\nAt this point in the code after running skip_prefix() on the\nvariable and receiving the result in the same variable, the contents\nof the variable can never be NULL. The function either (1) updates\nthe variable to point at a later part of the string it originally\npointed at, or (2) leaves it intact if the string does not have the\nprefix. (1) will never make the variable NULL, and (2) cannot be\nthe source of NULL, because the variable cannot be NULL before\ncalling skip_prefix(), which would die immediately by dereferencing\nthe NULL pointer in that case.\n\nHelped-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n parse-options.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 27c1e75d53..97a55300e8 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn 0;\n \n \t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n-\t\tif (!value)\n-\t\t\tis_optional = false;\n \t\tvalue = fix_filename(p->prefix, value);\n \t\tif (is_optional && is_missing_file(value)) {\n \t\t\tfree((char *)value);\n-- \n2.52.0-rc0-28-g4cf919bd7b\n\n"},{"id":"530210","messageId":"CALnO6CCLUfQyMb2DU5OXowvO6SLhdso4mOr4ukQe_A_kAL1t0Q@mail.gmail.com","threadId":"64421","inReplyTo":"962654fc-02ea-47a9-a2ae-913101281240@gmail.com","subject":"Re: [PATCH 3/5] parseopt: use boolean type for a simple flag","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-04T18:22:49Z","receivedAt":"2025-11-04T18:23:03Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Tue, Nov 4, 2025 at 11:21 AM Phillip Wood wrote:\n>\n> On 03/11/2025 05:19, Junio C Hamano wrote:\n> > \"D. Ben Knoble\" writes:\n> >\n> >> is_optional = skip_prefix(value, \":(optional)\", &value);\n> >> if (!value)\n> >> - is_optional = 0;\n> >> + is_optional = false;\n> >\n> > Whether it is spelled 0 or false, I do not think this makes any\n> > sense. skip_prefix() either touches &value to point at the\n> > substring in value that comes after \":(optional)\", or it does not\n> > touch it at all, so there is no way value can be NULL here (and we\n> > know value is not NULL before we call skip_prefix()).\n> >\n> > Shouldn't you be removing the entire \"if value is NULL, it is not\n> > optional\" thing instead? That is exactly what Phillip pointed out\n> > in his review.\n>\n> Looking at this again I wonder if the intention was to error out if\n> there wasn't a filename after the \":(optional)\" prefix which I think\n> would be a reasonable thing to do but that's not what this code actually\n> does.\n>\n> Thanks\n>\n> Phillip\n\nAgreed both, will reroll\n"},{"id":"530211","messageId":"CALnO6CBgTu4Qtr8xwPCerrpqfAf02SV-bONLOUgq0X70eDThfw@mail.gmail.com","threadId":"64421","inReplyTo":"xmqqwm45puqr.fsf@gitster.g","subject":"Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-04T18:24:52Z","receivedAt":"2025-11-04T18:25:05Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Tue, Nov 4, 2025 at 12:34 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Phillip Wood writes:\n> >\n> >> Hi Ben\n> >>\n> >> These all look good to me though I agree with Junio's comments on patch\n> >> 3. It would be nice to get at least the fist patch merged in time for\n> >> 2.52.0.\n> >\n> > Yup, let me do exactly that ;-)\n> >\n> > Thanks, both.\n>\n> Let me have this on top of Ben's 5-patch series.\n>\n> ----- >8 -----\n> Subject: [PATCH] parseopt: remove unreachable code\n>\n> At this point in the code after running skip_prefix() on the\n> variable and receiving the result in the same variable, the contents\n> of the variable can never be NULL. The function either (1) updates\n> the variable to point at a later part of the string it originally\n> pointed at, or (2) leaves it intact if the string does not have the\n> prefix. (1) will never make the variable NULL, and (2) cannot be\n> the source of NULL, because the variable cannot be NULL before\n> calling skip_prefix(), which would die immediately by dereferencing\n> the NULL pointer in that case.\n>\n> Helped-by: Phillip Wood \n> Signed-off-by: Junio C Hamano \n> ---\n> parse-options.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index 27c1e75d53..97a55300e8 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n> return 0;\n>\n> is_optional = skip_prefix(value, \":(optional)\", &value);\n> - if (!value)\n> - is_optional = false;\n> value = fix_filename(p->prefix, value);\n> if (is_optional && is_missing_file(value)) {\n> free((char *)value);\n> --\n> 2.52.0-rc0-28-g4cf919bd7b\n>\n\nAh, that's fine, and I won't send a new version unless we need it. Thanks!\n\nI do wonder about Phillip's comment: perhaps it should have been\n`!*value`? But then something else has to be fixed because value is no\nlonger \":(optional)\" which maybe was intended as a literal filename?\nIDK.\n"},{"id":"530255","messageId":"5951a930-0e57-4201-9b56-12a41cb44333@gmail.com","threadId":"64421","inReplyTo":"xmqqwm45puqr.fsf@gitster.g","subject":"Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-05T16:35:25Z","receivedAt":"2025-11-05T16:35:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/11/2025 17:34, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> Let me have this on top of Ben's 5-patch series.\n> \n> ----- >8 -----\n> Subject: [PATCH] parseopt: remove unreachable code\n> \n> At this point in the code after running skip_prefix() on the\n> variable and receiving the result in the same variable, the contents\n> of the variable can never be NULL. The function either (1) updates\n> the variable to point at a later part of the string it originally\n> pointed at, or (2) leaves it intact if the string does not have the\n> prefix. (1) will never make the variable NULL, and (2) cannot be\n> the source of NULL, because the variable cannot be NULL before\n> calling skip_prefix(), which would die immediately by dereferencing\n> the NULL pointer in that case.\n\nNicely explained, the changes below look good\n\nThanks\n\nPhillip\n\n> Helped-by: Phillip Wood \n> Signed-off-by: Junio C Hamano \n> ---\n> parse-options.c | 2 --\n> 1 file changed, 2 deletions(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 27c1e75d53..97a55300e8 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n> \t\t\treturn 0;\n> \n> \t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n> -\t\tif (!value)\n> -\t\t\tis_optional = false;\n> \t\tvalue = fix_filename(p->prefix, value);\n> \t\tif (is_optional && is_missing_file(value)) {\n> \t\t\tfree((char *)value);\n\n"},{"id":"530330","messageId":"xmqqecqbhx3b.fsf@gitster.g","threadId":"64421","inReplyTo":"5951a930-0e57-4201-9b56-12a41cb44333@gmail.com","subject":"Re: [PATCH 1/5] parseopt: fix :(optional) at command line to only ignore missing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-06T17:47:36Z","receivedAt":"2025-11-06T17:47:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 04/11/2025 17:34, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> \n>> Let me have this on top of Ben's 5-patch series.\n>> \n>> ----- >8 -----\n>> Subject: [PATCH] parseopt: remove unreachable code\n>> \n>> At this point in the code after running skip_prefix() on the\n>> variable and receiving the result in the same variable, the contents\n>> of the variable can never be NULL. The function either (1) updates\n>> the variable to point at a later part of the string it originally\n>> pointed at, or (2) leaves it intact if the string does not have the\n>> prefix. (1) will never make the variable NULL, and (2) cannot be\n>> the source of NULL, because the variable cannot be NULL before\n>> calling skip_prefix(), which would die immediately by dereferencing\n>> the NULL pointer in that case.\n>\n> Nicely explained, the changes below look good\n>\n> Thanks\n>\n> Phillip\n\nThanks for a quick review.\n\n>\n>> Helped-by: Phillip Wood \n>> Signed-off-by: Junio C Hamano \n>> ---\n>> parse-options.c | 2 --\n>> 1 file changed, 2 deletions(-)\n>> \n>> diff --git a/parse-options.c b/parse-options.c\n>> index 27c1e75d53..97a55300e8 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n>> \t\t\treturn 0;\n>> \n>> \t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n>> -\t\tif (!value)\n>> -\t\t\tis_optional = false;\n>> \t\tvalue = fix_filename(p->prefix, value);\n>> \t\tif (is_optional && is_missing_file(value)) {\n>> \t\t\tfree((char *)value);\n"}]}