git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
D. Ben Knoble <ben.knoble+github@gmail.com>
Date
Nov 4, 2025, 18:24 UTC
Message-ID
<CALnO6CBgTu4Qtr8xwPCerrpqfAf02SV-bONLOUgq0X70eDThfw@mail.gmail.com>
In-Reply-To
<xmqqwm45puqr.fsf@gitster.g>
On Tue, Nov 4, 2025 at 12:34 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 52 quoted lines
>
> 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.

Previous: Junio C HamanoNext: Phillip Wood
Message 6 of 15 in “Fixes for :(optional) path code”
  1. 0/5 Fixes for :(optional) path codeD. Ben Knoble, Nov 2, 2025
  2. 1/5 parseopt: fix :(optional) at command line to only ignore missing filesD. Ben Knoble, Nov 2, 2025
  3. Phillip WoodNov 4, 2025
  4. Junio C HamanoNov 4, 2025
  5. Junio C HamanoNov 4, 2025
  6. D. Ben KnobleNov 4, 2025
  7. Phillip WoodNov 5, 2025
  8. Junio C HamanoNov 6, 2025
  9. 2/5 doc: clarify command equivalence commentD. Ben Knoble, Nov 2, 2025
  10. 3/5 parseopt: use boolean type for a simple flagD. Ben Knoble, Nov 2, 2025
  11. Junio C HamanoNov 3, 2025
  12. Phillip WoodNov 4, 2025
  13. D. Ben KnobleNov 4, 2025
  14. 4/5 config: use boolean type for a simple flagD. Ben Knoble, Nov 2, 2025
  15. 5/5 parseopt: restore const qualifier to parsed filenameD. Ben Knoble, Nov 2, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.