Re: [PATCH 1/2] contrib/subtree: Add -S/--gpg-sign option

From: Junio C Hamano <gitster@pobox.com>
Date: May 29, 2025, 23:45 UTC
Message-ID: <xmqqbjrbhtgw.fsf@gitster.g>
In-Reply-To: <20250528130116.21534-2-patrik@pspdfkit.com>

Patrik Weiskircher <patrik@pspdfkit.com> writes:

Show 13 quoted lines

> +# Usage: gpg_sign_opt
> +# Returns the GPG signing option for git commit-tree
> +gpg_sign_opt () {
> +	if test "${arg_gpg_sign+set}" = "set"
> +	then
> +		if test -n "$arg_gpg_sign"
> +		then
> +			printf " -S%s" "$arg_gpg_sign"
> +		else
> +			printf " -S"
> +		fi
> +	fi
> +}

I wonder if this misbehaves if the user has arg_gpg_sign defined in their environment. I guess that this contrib script is written loosely in that regard, as none of the other arg_* variables are cleared upon script start-up, so let's let it pass.

Also, why is this a separate function that needs to be used via $(command substitution)? Wouldn't it be more natural to have something like

	if test "${arg_gpg_sign+set}" = set
	then
		arg_gpg_sign=-S$arg_gpg_sign
	else
		arg_gpg_sign=
	fi

after the option parsing loop and have the caller just interpolate it as $arg_gpg_sign (without surrounding double quotes)?

Or you can define arg_gpg_sign to be

 * unset, if the user did not give any -S option
 * -S<string>, if the user gave <string> as its value
 * -S, if the user only gave -S without value

directly inside the option parsing loop, so there is no such clean-up needed, perhaps?

Near the start of "main" function, there is an early pre-parse loop that iterates over all the arguments and is aware of which option takes its own value, i.e.

		case "$opt" in
			--annotate|-b|-P|-m|--onto)
				shift
				;;
			--rejoin)
				arg_split_rejoin=1
				;;

If you are adding "-S" as taking its own value separately, don't you need to add it to that part of the code, next to -b, -P, and their friends?

Show 15 quoted lines

>  die_incompatible_opt () {
>  	assert test "$#" = 2
> @@ -240,6 +255,15 @@ main () {
>  			test -n "$allow_addmerge" || die_incompatible_opt "$opt" "$arg_command"
>  			arg_addmerge_squash=
>  			;;
> +		-S)
> +			if test $# -gt 0 && test "${1#-}" = "$1"
> +			then
> +				arg_gpg_sign="$1"
> +				shift
> +			else
> +				arg_gpg_sign=""
> +			fi
> +			;;

The comparison between "${1#-}" and "$1" is "does the next argument start with a dash?", iow, you attempt to parse this

	git subtree -S -P prefix add HEAD

as "-S" with empty arg_gpg_sign, instead of slurping "-P" as its argument. Which is nice, but is that enough to parse this

	git subtree -S add HEAD

correctly?

> +'git subtree' [<options>] -P <prefix> [-S[<keyid>]] add <local-commit>
> +'git subtree' [<options>] -P <prefix> [-S[<keyid>]] add <repository> <remote-ref>
> +'git subtree' [<options>] -P <prefix> [-S[<keyid>]] merge <local-commit> [<repository>]
> +'git subtree' [<options>] -P <prefix> [-S[<keyid>]] split [<local-commit>]

These synopsis elements suggest that <keyid> should be given in the "stuck" form as the value for the "-S" option, and if that were the case, your option parser would look more like

		-S*)
			arg_gpg_sign=${opt#-S}
			;;

which is much simpler and more robust. If you go that route, you would probably want to retrofit -P and other options to also allow their value in the "stuck" form as well as their traditional "separate" form, which would also be annoying, so I do not know if it is worth it.

Or just use "arg_gpg_sign=$opt" here, if you are following an earlier suggestion to get rid of the gpg_sign_opt helper function that does not seem to be doing anything useful. And then ...

Show 6 quoted lines

> @@ -537,7 +561,7 @@ copy_commit () {
>  			printf "%s" "$arg_split_annotate"
>  			cat
>  		) |
> -		git commit-tree "$2" $3  # reads the rest of stdin
> +		git commit-tree "$2" $(gpg_sign_opt) $3  # reads the rest of stdin

... here, you'd just say

		git commit-tree "$2" $gpg_sign_opt $3  # reads the rest of stdin

instead. If your <key-id> has $IFS whitespace, this will break, but your gpg_sign_out function does not help with that issue anyway (you'd need to rewrite this and other callers to use eval if you really wanted to treat $IFS whitespace in values properly, but looking at the existing code in this script, I do not see any careful string handling to do so, so it may be more appropriate and punt, saying "no, key IDs with whitespaces in them are not supported").