Patrik Weiskircher <patrik@xxxxxxxxxxxx> 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").
