From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 30 Sep 2025 15:26:45 GMT
Subject: Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)
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:
> 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.

> +~~~~~~~~~~~~~~~~~~~~~~
> +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].
> +

> @@ -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.

>   		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*")

> +		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

> +		}
> +		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 &&
>   	(