Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 30, 2025, 15:26 UTC
- Message-ID
- <e8755a04-bd44-4ead-ba44-c603bffcc75e@gmail.com>
- In-Reply-To
- <5f7057c236c9af3152bd531eed2e4ad0ac35e291.1759094936.git.ben.knoble+github@gmail.com>
Hi Ben
On 28/09/2025 22:29, D. Ben Knoble wrote:
Show 8 quoted lines
> From: Junio C Hamano <gitster@pobox.com> > > In the previous step, we introduced an optional filename that can be > given to a configuration variable, and nullify the fact that such a > configuration setting even existed if the named path is missing or > empty. > > Let's do the same for command line options that name a pathname.
Sounds sensible
> +Magic filename options
I assume we're calling these "magic" to match to pathspec "magic" options? I wonder if that is a good idea but I don't have a better suggestion.
Show 6 quoted lines
> +~~~~~~~~~~~~~~~~~~~~~~ > +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
This doesn't quite scan for me, maybe s/, /, it is/ ?
> +git commit > +---------------------------- > + > +Like with configuration values, if the named file is missing Git behaves as if
I'd drop "with" here
> +the option was not given at all. See "Values" in linkgit:git-config[1]. > +
Show 9 quoted lines
> @@ -209,21 +208,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
> case OPTION_FILENAME:
> {
> const char *value;
> -
> - FREE_AND_NULL(*(char **)opt->value);
> -
> - err = 0;
> + int is_optional;This can be a bool as in the last patch.
Show 7 quoted lines
> if (unset) > value = NULL; > else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) > - value = (const char *) opt->defval; > - else > - err = get_arg(p, opt, flags, &value); > + value = (char *)opt->defval;
I'm not sure why we're changing the cast here (or why we need one in the first place assuming opt->defval is "void*")
Show 14 quoted lines
> + else {
> + int err = get_arg(p, opt, flags, &value);
> + if (err)
> + return err;
> + }
> + if (!value)
> + return 0;
>
> - if (!err)
> - *(char **)opt->value = fix_filename(p->prefix, value);
> - return err;
> + is_optional = skip_prefix(value, ":(optional)", &value);
> + if (!value)
> + is_optional = 0;I'm struggling to see how value can be NULL here as we return early if it NULL before calling skip_prefix()
> + value = fix_filename(p->prefix, value);
> + if (is_optional && is_empty_or_missing_file(value)) {
> + free((char *)value);I think we want to call is_missing_file() here. If the file is missing then we do nothing which matches the documentation above - Good.
> + } else {
> + FREE_AND_NULL(*(char **)opt->value);
> + *(const char **)opt->value = value;If the file isn't optional or it is optional and exists then we behave as before - Good.
Thanks
Phillip
Show 26 quoted lines
> + }
> + return 0;
> }
> case OPTION_CALLBACK:
> {
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 366f7f23b3..c065f12baf 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -37,6 +37,16 @@ commit_msg_is ()
> )
> '
>
> +test_expect_success 'nonexistent optional template file on command line' '
> + echo changes >> foo &&
> + git add foo &&
> + (
> + GIT_EDITOR="echo hello >\"\$1\"" &&
> + export GIT_EDITOR &&
> + git commit --template ":(optional)$PWD/notexist"
> + )
> +'
> +
> test_expect_success 'nonexistent template file in config should return error' '
> test_config commit.template "$PWD"/notexist &&
> (