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

[Junio C Hamano <gitster@pobox.com>]

Patrik Weiskircher <patrik@pspdfkit.com> writes:

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

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

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