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

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

[Patrik Weiskircher]

Hi!

We use git subtree a lot to manage our dependencies, but recently started
requiring signed commits. This patch adds support for signing commits to
git subtree.

This is my first submission to the Git project - you don't have to be gentle,
but please let me know if I can improve anything.

Thanks!
Patrik

Patrik Weiskircher (2):
  contrib/subtree: Add -S/--gpg-sign option
  contrib/subtree: Add tests for -S/--gpg-sign

 contrib/subtree/git-subtree.adoc   |  20 +++--
 contrib/subtree/git-subtree.sh     |  50 +++++++++----
 contrib/subtree/t/t7900-subtree.sh | 113 +++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+), 19 deletions(-)

-- 
2.49.0

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

[Patrik Weiskircher]

If set, forwards the options to commit-tree and merge.

Signed-off-by: Patrik Weiskircher <patrik@pspdfkit.com>
---
 contrib/subtree/git-subtree.adoc | 20 +++++++++----
 contrib/subtree/git-subtree.sh   | 50 +++++++++++++++++++++++---------
 2 files changed, 51 insertions(+), 19 deletions(-)

diff --git a/contrib/subtree/git-subtree.adoc b/contrib/subtree/git-subtree.adoc
index 004abf415b..f550be1a86 100644
--- a/contrib/subtree/git-subtree.adoc
+++ b/contrib/subtree/git-subtree.adoc
@@ -9,14 +9,14 @@ git-subtree - Merge subtrees together and split repository into subtrees
 SYNOPSIS
 --------
 [verse]
-'git subtree' [<options>] -P <prefix> add <local-commit>
-'git subtree' [<options>] -P <prefix> add <repository> <remote-ref>
-'git subtree' [<options>] -P <prefix> merge <local-commit> [<repository>]
-'git subtree' [<options>] -P <prefix> split [<local-commit>]
+'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>]
 
 [verse]
-'git subtree' [<options>] -P <prefix> pull <repository> <remote-ref>
-'git subtree' [<options>] -P <prefix> push <repository> <refspec>
+'git subtree' [<options>] -P <prefix> [-S[<keyid>]] pull <repository> <remote-ref>
+'git subtree' [<options>] -P <prefix> [-S[<keyid>]] push <repository> <refspec>
 
 DESCRIPTION
 -----------
@@ -149,6 +149,14 @@ OPTIONS FOR ALL COMMANDS
 	want to manipulate.  This option is mandatory
 	for all commands.
 
+-S[<keyid>]::
+--gpg-sign[=<keyid>]::
+--no-gpg-sign::
+	GPG-sign commits. The `keyid` argument is optional and
+	defaults to the committer identity; if specified, it must be
+	stuck to the option without a space. `--no-gpg-sign` is useful to
+	countermand a `--gpg-sign` option given earlier on the command line.
+
 OPTIONS FOR 'add' AND 'merge' (ALSO: 'pull', 'split --rejoin', AND 'push --rejoin')
 -----------------------------------------------------------------------------------
 These options for 'add' and 'merge' may also be given to 'pull' (which
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 15ae86db1b..b98a708c10 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -26,12 +26,12 @@ then
 fi
 
 OPTS_SPEC="\
-git subtree add   --prefix=<prefix> <commit>
-git subtree add   --prefix=<prefix> <repository> <ref>
-git subtree merge --prefix=<prefix> <commit>
-git subtree split --prefix=<prefix> [<commit>]
-git subtree pull  --prefix=<prefix> <repository> <ref>
-git subtree push  --prefix=<prefix> <repository> <refspec>
+git subtree add   --prefix=<prefix> [-S[<keyid>]] <commit>
+git subtree add   --prefix=<prefix> [-S[<keyid>]] <repository> <ref>
+git subtree merge --prefix=<prefix> [-S[<keyid>]] <commit>
+git subtree split --prefix=<prefix> [-S[<keyid>]] [<commit>]
+git subtree pull  --prefix=<prefix> [-S[<keyid>]] <repository> <ref>
+git subtree push  --prefix=<prefix> [-S[<keyid>]] <repository> <refspec>
 --
 h,help!       show the help
 q,quiet!      quiet
@@ -46,6 +46,7 @@ rejoin        merge the new branch back into HEAD
  options for 'add' and 'merge' (also: 'pull', 'split --rejoin', and 'push --rejoin')
 squash        merge subtree changes as a single commit
 m,message!=   use the given message as the commit message for the merge commit
+S,gpg-sign?key-id   GPG-sign commits. The keyid argument is optional and defaults to the committer identity; if specified, it must be stuck to the option without a space.
 "
 
 indent=0
@@ -102,6 +103,20 @@ assert () {
 	fi
 }
 
+# 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
+}
+
 # Usage: die_incompatible_opt OPTION COMMAND
 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
+			;;
 		--)
 			break
 			;;
@@ -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
 	) || die "fatal: can't copy commit $1"
 }
 
@@ -683,10 +707,10 @@ new_squash_commit () {
 	if test -n "$old"
 	then
 		squash_msg "$dir" "$oldsub" "$newsub" |
-		git commit-tree "$tree" -p "$old" || exit $?
+		git commit-tree "$tree" $(gpg_sign_opt) -p "$old" || exit $?
 	else
 		squash_msg "$dir" "" "$newsub" |
-		git commit-tree "$tree" || exit $?
+		git commit-tree "$tree" $(gpg_sign_opt) || exit $?
 	fi
 }
 
@@ -925,11 +949,11 @@ cmd_add_commit () {
 	then
 		rev=$(new_squash_commit "" "" "$rev") || exit $?
 		commit=$(add_squashed_msg "$rev" "$dir" |
-			git commit-tree "$tree" $headp -p "$rev") || exit $?
+			git commit-tree "$tree" $(gpg_sign_opt) $headp -p "$rev") || exit $?
 	else
 		revp=$(peel_committish "$rev") || exit $?
 		commit=$(add_msg "$dir" $headrev "$rev" |
-			git commit-tree "$tree" $headp -p "$revp") || exit $?
+			git commit-tree "$tree" $(gpg_sign_opt) $headp -p "$revp") || exit $?
 	fi
 	git reset "$commit" || exit $?
 
@@ -1080,9 +1104,9 @@ cmd_merge () {
 	if test -n "$arg_addmerge_message"
 	then
 		git merge --no-ff -Xsubtree="$arg_prefix" \
-			--message="$arg_addmerge_message" "$rev"
+			--message="$arg_addmerge_message" $(gpg_sign_opt) "$rev"
 	else
-		git merge --no-ff -Xsubtree="$arg_prefix" $rev
+		git merge --no-ff -Xsubtree="$arg_prefix" $(gpg_sign_opt) "$rev"
 	fi
 }
 
-- 
2.49.0

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

[Junio C Hamano]

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

[PATCH 2/2] contrib/subtree: Add tests for -S/--gpg-sign

[Patrik Weiskircher]

Make sure it works correctly.

Signed-off-by: Patrik Weiskircher <patrik@pspdfkit.com>
---
 contrib/subtree/t/t7900-subtree.sh | 113 +++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 3c6103f6d2..3edbb33af4 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -11,6 +11,7 @@ and push subcommands of git subtree.
 
 TEST_DIRECTORY=$(pwd)/../../../t
 . "$TEST_DIRECTORY"/test-lib.sh
+. "$TEST_DIRECTORY"/lib-gpg.sh
 
 # Use our own wrapper around test-lib.sh's test_create_repo, in order
 # to set log.date=relative.  `git subtree` parses the output of `git
@@ -1563,4 +1564,116 @@ test_expect_success 'subtree descendant check' '
 	)
 '
 
+test_expect_success GPG 'add subproj with GPG signing using -S flag' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" -S FETCH_HEAD &&
+		git verify-commit HEAD &&
+		test "$(last_commit_subject)" = "Add '\''sub dir/'\'' from commit '\''$(git rev-parse FETCH_HEAD)'\''"
+	)
+'
+
+test_expect_success GPG 'add subproj with GPG signing using --gpg-sign flag' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" --gpg-sign FETCH_HEAD &&
+		git verify-commit HEAD &&
+		test "$(last_commit_subject)" = "Add '\''sub dir/'\'' from commit '\''$(git rev-parse FETCH_HEAD)'\''"
+	)
+'
+
+test_expect_success GPG 'add subproj with GPG signing using specific key ID' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" -S"$GIT_COMMITTER_EMAIL" FETCH_HEAD &&
+		git verify-commit HEAD &&
+		test "$(last_commit_subject)" = "Add '\''sub dir/'\'' from commit '\''$(git rev-parse FETCH_HEAD)'\''"
+	)
+'
+
+test_expect_success GPG 'merge with GPG signing' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" FETCH_HEAD
+	) &&
+	test_create_commit "$test_count/sub proj" sub2 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree merge --prefix="sub dir" -S FETCH_HEAD &&
+		git verify-commit HEAD
+	)
+'
+
+test_expect_success GPG 'split with GPG signing and --rejoin' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" FETCH_HEAD
+	) &&
+	test_create_commit "$test_count" "sub dir/main-sub1" &&
+	(
+		cd "$test_count" &&
+		git subtree split --prefix="sub dir" --rejoin -S &&
+		git verify-commit HEAD
+	)
+'
+
+test_expect_success GPG 'add with --squash and GPG signing' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" --squash -S FETCH_HEAD &&
+		git verify-commit HEAD &&
+		# With --squash, the commit subject should reference the squash commit (first parent of merge)
+		squash_commit=$(git rev-parse HEAD^2) &&
+		test "$(last_commit_subject)" = "Merge commit '\''$squash_commit'\'' as '\''sub dir'\''"
+	)
+'
+
+test_expect_success GPG 'pull with GPG signing' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git subtree add --prefix="sub dir" ./"sub proj" HEAD
+	) &&
+	test_create_commit "$test_count/sub proj" sub2 &&
+	(
+		cd "$test_count" &&
+		git subtree pull --prefix="sub dir" -S ./"sub proj" HEAD &&
+		git verify-commit HEAD
+	)
+'
+
 test_done
-- 
2.49.0

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

[Junio C Hamano]

Patrik Weiskircher <patrik@pspdfkit.com> writes:

> Hi!
>
> We use git subtree a lot to manage our dependencies, but recently started
> requiring signed commits. This patch adds support for signing commits to
> git subtree.
>
> This is my first submission to the Git project - you don't have to be gentle,
> but please let me know if I can improve anything.

A few things ;-) starting from the log message where you didn't
quite add why we would want to do these changes.

Also, I think these two should be combined into a single patch.  We
add a feature and make sure the feature works correctly and we also
make sure that the feature does not misfire when the user does not
ask it to trigger, all in the same commit.

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

[Patrik Weiskircher]

On Thu, May 29, 2025 at 7:15 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrik Weiskircher <patrik@pspdfkit.com> writes:
>
> > Hi!
> >
> > We use git subtree a lot to manage our dependencies, but recently started
> > requiring signed commits. This patch adds support for signing commits to
> > git subtree.
> >
> > This is my first submission to the Git project - you don't have to be gentle,
> > but please let me know if I can improve anything.

Apologies, Junio. I replied only to you and with HTML emails. I'm very
new here and I'm very much used to different workflows. I'll be
better. Please disregard my previous emails.

>
> A few things ;-) starting from the log message where you didn't
> quite add why we would want to do these changes.
>
> Also, I think these two should be combined into a single patch.  We
> add a feature and make sure the feature works correctly and we also
> make sure that the feature does not misfire when the user does not
> ask it to trigger, all in the same commit.

I looked at your review, and I agree with everything you said. My
patch wasn't very well thought out.

I'm also at a crossroads here. I'm not sure if the optional argument
parsing will work out nicely in bash. Let me explain:

git-subtree uses `git rev-parse --parseopt`. This *does* have support
for optional arguments, but highly recommends using `--stuck-long`,
otherwise unambiguously parsing arguments becomes impossible.

This is true. There's no way to differentiate between the optional
argument or the next positional parameter. Therefore, I would have to
use `--stuck-long`.

This in itself isn't overly difficult. There's only a couple of
parameters in the script that that would affect. The problem I have
with it is that I don't see a single user of `--stuck-long` in the
whole repo. I don't necessarily want to become the first one. Seems
like something destined to be cleaned up at some point.

This means my options become:
1. use --stuck-long or
2. make -S not take any parameter or
3. make -S require a parameter

I'm not happy with any of these options, but considering I really
would like to make this work, I would probably go with 1.

Any opinions on this?

Thanks!
Patrik

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

[Junio C Hamano]

Patrik Weiskircher <patrik.weiskircher@nutrient.io> writes:

> This means my options become:
> 1. use --stuck-long or
> 2. make -S not take any parameter or
> 3. make -S require a parameter

4. accept "-S[<key-id>]" in stuck form, and --gpg-sign[=<key-id>]

which is a variant of #1 may be an option?

Thanks.

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

[Patrik Weiskircher]

On Fri, May 30, 2025 at 4:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrik Weiskircher <patrik.weiskircher@nutrient.io> writes:
>
> > This means my options become:
> > 1. use --stuck-long or
> > 2. make -S not take any parameter or
> > 3. make -S require a parameter
>
> 4. accept "-S[<key-id>]" in stuck form, and --gpg-sign[=<key-id>]
>
> which is a variant of #1 may be an option?

Not with git rev-parse, unfortunately. Either `-Sabc` or
`--gpg-sign=abc` would get transformed into `-S 'abc'` after
rev-parse, where we run into the ambiguity of what is an optional
argument again. I *could* parse this manually without git rev-parse
beforehand, but that seems like just adding yet-another difficult to
maintain thing.

```
$ echo $OPTS_SPEC | git rev-parse --parseopt -- "--gpg-sign=abcd"
set -- -S 'abcd' --

$ echo $OPTS_SPEC | git rev-parse --parseopt -- "-Sabcd"
set -- -S 'abcd' --
```