Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)
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.
> +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
> +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 &&
> (
Noah Pendleton
