Re: [PATCH 1/2] contrib/subtree: Add -S/--gpg-sign option
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=
fiafter 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").
Patrik Weiskircher