{"thread":{"id":"63539","subject":"[PATCH 0/2] contrib/subtree: Add -S/--gpg-sign option","startedAt":"2025-05-28T13:02:14Z","lastAt":"2025-06-02T13:13:05Z","messageCount":8,"participants":["Patrik Weiskircher","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"519077","messageId":"20250528130116.21534-1-patrik@pspdfkit.com","threadId":"63539","inReplyTo":null,"subject":"[PATCH 0/2] contrib/subtree: Add -S/--gpg-sign option","fromName":"Patrik Weiskircher","fromEmail":"patrik@pspdfkit.com","sentAt":"2025-05-28T13:01:14Z","receivedAt":"2025-05-28T13:02:14Z","isPatch":true,"sender":{"key":"patrik@pspdfkit.com","avatar":"https://avatars.githubusercontent.com/u/1521642?v=4"},"body":"Hi!\n\nWe use git subtree a lot to manage our dependencies, but recently started\nrequiring signed commits. This patch adds support for signing commits to\ngit subtree.\n\nThis is my first submission to the Git project - you don't have to be gentle,\nbut please let me know if I can improve anything.\n\nThanks!\nPatrik\n\nPatrik Weiskircher (2):\n contrib/subtree: Add -S/--gpg-sign option\n contrib/subtree: Add tests for -S/--gpg-sign\n\n contrib/subtree/git-subtree.adoc | 20 +++--\n contrib/subtree/git-subtree.sh | 50 +++++++++----\n contrib/subtree/t/t7900-subtree.sh | 113 +++++++++++++++++++++++++++++\n 3 files changed, 164 insertions(+), 19 deletions(-)\n\n-- \n2.49.0\n\n"},{"id":"519078","messageId":"20250528130116.21534-2-patrik@pspdfkit.com","threadId":"63539","inReplyTo":"20250528130116.21534-1-patrik@pspdfkit.com","subject":"[PATCH 1/2] contrib/subtree: Add -S/--gpg-sign option","fromName":"Patrik Weiskircher","fromEmail":"patrik@pspdfkit.com","sentAt":"2025-05-28T13:01:15Z","receivedAt":"2025-05-28T13:02:23Z","isPatch":true,"sender":{"key":"patrik@pspdfkit.com","avatar":"https://avatars.githubusercontent.com/u/1521642?v=4"},"body":"If set, forwards the options to commit-tree and merge.\n\nSigned-off-by: Patrik Weiskircher \n---\n contrib/subtree/git-subtree.adoc | 20 +++++++++----\n contrib/subtree/git-subtree.sh | 50 +++++++++++++++++++++++---------\n 2 files changed, 51 insertions(+), 19 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.adoc b/contrib/subtree/git-subtree.adoc\nindex 004abf415b..f550be1a86 100644\n--- a/contrib/subtree/git-subtree.adoc\n+++ b/contrib/subtree/git-subtree.adoc\n@@ -9,14 +9,14 @@ git-subtree - Merge subtrees together and split repository into subtrees\n SYNOPSIS\n --------\n [verse]\n-'git subtree' [] -P add \n-'git subtree' [] -P add \n-'git subtree' [] -P merge []\n-'git subtree' [] -P split []\n+'git subtree' [] -P [-S[]] add \n+'git subtree' [] -P [-S[]] add \n+'git subtree' [] -P [-S[]] merge []\n+'git subtree' [] -P [-S[]] split []\n \n [verse]\n-'git subtree' [] -P pull \n-'git subtree' [] -P push \n+'git subtree' [] -P [-S[]] pull \n+'git subtree' [] -P [-S[]] push \n \n DESCRIPTION\n -----------\n@@ -149,6 +149,14 @@ OPTIONS FOR ALL COMMANDS\n \twant to manipulate. This option is mandatory\n \tfor all commands.\n \n+-S[]::\n+--gpg-sign[=]::\n+--no-gpg-sign::\n+\tGPG-sign commits. The `keyid` argument is optional and\n+\tdefaults to the committer identity; if specified, it must be\n+\tstuck to the option without a space. `--no-gpg-sign` is useful to\n+\tcountermand a `--gpg-sign` option given earlier on the command line.\n+\n OPTIONS FOR 'add' AND 'merge' (ALSO: 'pull', 'split --rejoin', AND 'push --rejoin')\n -----------------------------------------------------------------------------------\n These options for 'add' and 'merge' may also be given to 'pull' (which\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 15ae86db1b..b98a708c10 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -26,12 +26,12 @@ then\n fi\n \n OPTS_SPEC=\"\\\n-git subtree add --prefix= \n-git subtree add --prefix= \n-git subtree merge --prefix= \n-git subtree split --prefix= []\n-git subtree pull --prefix= \n-git subtree push --prefix= \n+git subtree add --prefix= [-S[]] \n+git subtree add --prefix= [-S[]] \n+git subtree merge --prefix= [-S[]] \n+git subtree split --prefix= [-S[]] []\n+git subtree pull --prefix= [-S[]] \n+git subtree push --prefix= [-S[]] \n --\n h,help! show the help\n q,quiet! quiet\n@@ -46,6 +46,7 @@ rejoin merge the new branch back into HEAD\n options for 'add' and 'merge' (also: 'pull', 'split --rejoin', and 'push --rejoin')\n squash merge subtree changes as a single commit\n m,message!= use the given message as the commit message for the merge commit\n+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.\n \"\n \n indent=0\n@@ -102,6 +103,20 @@ assert () {\n \tfi\n }\n \n+# Usage: gpg_sign_opt\n+# Returns the GPG signing option for git commit-tree\n+gpg_sign_opt () {\n+\tif test \"${arg_gpg_sign+set}\" = \"set\"\n+\tthen\n+\t\tif test -n \"$arg_gpg_sign\"\n+\t\tthen\n+\t\t\tprintf \" -S%s\" \"$arg_gpg_sign\"\n+\t\telse\n+\t\t\tprintf \" -S\"\n+\t\tfi\n+\tfi\n+}\n+\n # Usage: die_incompatible_opt OPTION COMMAND\n die_incompatible_opt () {\n \tassert test \"$#\" = 2\n@@ -240,6 +255,15 @@ main () {\n \t\t\ttest -n \"$allow_addmerge\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_addmerge_squash=\n \t\t\t;;\n+\t\t-S)\n+\t\t\tif test $# -gt 0 && test \"${1#-}\" = \"$1\"\n+\t\t\tthen\n+\t\t\t\targ_gpg_sign=\"$1\"\n+\t\t\t\tshift\n+\t\t\telse\n+\t\t\t\targ_gpg_sign=\"\"\n+\t\t\tfi\n+\t\t\t;;\n \t\t--)\n \t\t\tbreak\n \t\t\t;;\n@@ -537,7 +561,7 @@ copy_commit () {\n \t\t\tprintf \"%s\" \"$arg_split_annotate\"\n \t\t\tcat\n \t\t) |\n-\t\tgit commit-tree \"$2\" $3 # reads the rest of stdin\n+\t\tgit commit-tree \"$2\" $(gpg_sign_opt) $3 # reads the rest of stdin\n \t) || die \"fatal: can't copy commit $1\"\n }\n \n@@ -683,10 +707,10 @@ new_squash_commit () {\n \tif test -n \"$old\"\n \tthen\n \t\tsquash_msg \"$dir\" \"$oldsub\" \"$newsub\" |\n-\t\tgit commit-tree \"$tree\" -p \"$old\" || exit $?\n+\t\tgit commit-tree \"$tree\" $(gpg_sign_opt) -p \"$old\" || exit $?\n \telse\n \t\tsquash_msg \"$dir\" \"\" \"$newsub\" |\n-\t\tgit commit-tree \"$tree\" || exit $?\n+\t\tgit commit-tree \"$tree\" $(gpg_sign_opt) || exit $?\n \tfi\n }\n \n@@ -925,11 +949,11 @@ cmd_add_commit () {\n \tthen\n \t\trev=$(new_squash_commit \"\" \"\" \"$rev\") || exit $?\n \t\tcommit=$(add_squashed_msg \"$rev\" \"$dir\" |\n-\t\t\tgit commit-tree \"$tree\" $headp -p \"$rev\") || exit $?\n+\t\t\tgit commit-tree \"$tree\" $(gpg_sign_opt) $headp -p \"$rev\") || exit $?\n \telse\n \t\trevp=$(peel_committish \"$rev\") || exit $?\n \t\tcommit=$(add_msg \"$dir\" $headrev \"$rev\" |\n-\t\t\tgit commit-tree \"$tree\" $headp -p \"$revp\") || exit $?\n+\t\t\tgit commit-tree \"$tree\" $(gpg_sign_opt) $headp -p \"$revp\") || exit $?\n \tfi\n \tgit reset \"$commit\" || exit $?\n \n@@ -1080,9 +1104,9 @@ cmd_merge () {\n \tif test -n \"$arg_addmerge_message\"\n \tthen\n \t\tgit merge --no-ff -Xsubtree=\"$arg_prefix\" \\\n-\t\t\t--message=\"$arg_addmerge_message\" \"$rev\"\n+\t\t\t--message=\"$arg_addmerge_message\" $(gpg_sign_opt) \"$rev\"\n \telse\n-\t\tgit merge --no-ff -Xsubtree=\"$arg_prefix\" $rev\n+\t\tgit merge --no-ff -Xsubtree=\"$arg_prefix\" $(gpg_sign_opt) \"$rev\"\n \tfi\n }\n \n-- \n2.49.0\n\n"},{"id":"519079","messageId":"20250528130116.21534-3-patrik@pspdfkit.com","threadId":"63539","inReplyTo":"20250528130116.21534-1-patrik@pspdfkit.com","subject":"[PATCH 2/2] contrib/subtree: Add tests for -S/--gpg-sign","fromName":"Patrik Weiskircher","fromEmail":"patrik@pspdfkit.com","sentAt":"2025-05-28T13:01:16Z","receivedAt":"2025-05-28T13:02:33Z","isPatch":true,"sender":{"key":"patrik@pspdfkit.com","avatar":"https://avatars.githubusercontent.com/u/1521642?v=4"},"body":"Make sure it works correctly.\n\nSigned-off-by: Patrik Weiskircher \n---\n contrib/subtree/t/t7900-subtree.sh | 113 +++++++++++++++++++++++++++++\n 1 file changed, 113 insertions(+)\n\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 3c6103f6d2..3edbb33af4 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -11,6 +11,7 @@ and push subcommands of git subtree.\n \n TEST_DIRECTORY=$(pwd)/../../../t\n . \"$TEST_DIRECTORY\"/test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-gpg.sh\n \n # Use our own wrapper around test-lib.sh's test_create_repo, in order\n # to set log.date=relative. `git subtree` parses the output of `git\n@@ -1563,4 +1564,116 @@ test_expect_success 'subtree descendant check' '\n \t)\n '\n \n+test_expect_success GPG 'add subproj with GPG signing using -S flag' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/sub proj\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/sub proj\" sub1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./\"sub proj\" HEAD &&\n+\t\tgit subtree add --prefix=\"sub dir\" -S FETCH_HEAD &&\n+\t\tgit verify-commit HEAD &&\n+\t\ttest \"$(last_commit_subject)\" = \"Add '\\''sub dir/'\\'' from commit '\\''$(git rev-parse FETCH_HEAD)'\\''\"\n+\t)\n+'\n+\n+test_expect_success GPG 'add subproj with GPG signing using --gpg-sign flag' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/sub proj\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/sub proj\" sub1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./\"sub proj\" HEAD &&\n+\t\tgit subtree add --prefix=\"sub dir\" --gpg-sign FETCH_HEAD &&\n+\t\tgit verify-commit HEAD &&\n+\t\ttest \"$(last_commit_subject)\" = \"Add '\\''sub dir/'\\'' from commit '\\''$(git rev-parse FETCH_HEAD)'\\''\"\n+\t)\n+'\n+\n+test_expect_success GPG 'add subproj with GPG signing using specific key ID' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/sub proj\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/sub proj\" sub1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./\"sub proj\" HEAD &&\n+\t\tgit subtree add --prefix=\"sub dir\" -S\"$GIT_COMMITTER_EMAIL\" FETCH_HEAD &&\n+\t\tgit verify-commit HEAD &&\n+\t\ttest \"$(last_commit_subject)\" = \"Add '\\''sub dir/'\\'' from commit '\\''$(git rev-parse FETCH_HEAD)'\\''\"\n+\t)\n+'\n+\n+test_expect_success GPG 'merge with GPG signing' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/sub proj\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/sub proj\" sub1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./\"sub proj\" HEAD &&\n+\t\tgit subtree add --prefix=\"sub dir\" FETCH_HEAD\n+\t) &&\n+\ttest_create_commit \"$test_count/sub proj\" sub2 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./\"sub proj\" HEAD &&\n+\t\tgit subtree merge --prefix=\"sub dir\" -S FETCH_HEAD &&\n+\t\tgit verify-commit HEAD\n+\t)\n+'\n+\n+test_expect_success GPG 'split with GPG signing and --rejoin' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/sub proj\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/sub proj\" sub1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./\"sub proj\" HEAD &&\n+\t\tgit subtree add --prefix=\"sub dir\" FETCH_HEAD\n+\t) &&\n+\ttest_create_commit \"$test_count\" \"sub dir/main-sub1\" &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree split --prefix=\"sub dir\" --rejoin -S &&\n+\t\tgit verify-commit HEAD\n+\t)\n+'\n+\n+test_expect_success GPG 'add with --squash and GPG signing' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/sub proj\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/sub proj\" sub1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./\"sub proj\" HEAD &&\n+\t\tgit subtree add --prefix=\"sub dir\" --squash -S FETCH_HEAD &&\n+\t\tgit verify-commit HEAD &&\n+\t\t# With --squash, the commit subject should reference the squash commit (first parent of merge)\n+\t\tsquash_commit=$(git rev-parse HEAD^2) &&\n+\t\ttest \"$(last_commit_subject)\" = \"Merge commit '\\''$squash_commit'\\'' as '\\''sub dir'\\''\"\n+\t)\n+'\n+\n+test_expect_success GPG 'pull with GPG signing' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/sub proj\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/sub proj\" sub1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree add --prefix=\"sub dir\" ./\"sub proj\" HEAD\n+\t) &&\n+\ttest_create_commit \"$test_count/sub proj\" sub2 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree pull --prefix=\"sub dir\" -S ./\"sub proj\" HEAD &&\n+\t\tgit verify-commit HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.49.0\n\n"},{"id":"519187","messageId":"xmqqfrgnhuuw.fsf@gitster.g","threadId":"63539","inReplyTo":"20250528130116.21534-1-patrik@pspdfkit.com","subject":"Re: [PATCH 0/2] contrib/subtree: Add -S/--gpg-sign option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-29T23:15:19Z","receivedAt":"2025-05-29T23:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrik Weiskircher writes:\n\n> Hi!\n>\n> We use git subtree a lot to manage our dependencies, but recently started\n> requiring signed commits. This patch adds support for signing commits to\n> git subtree.\n>\n> This is my first submission to the Git project - you don't have to be gentle,\n> but please let me know if I can improve anything.\n\nA few things ;-) starting from the log message where you didn't\nquite add why we would want to do these changes.\n\nAlso, I think these two should be combined into a single patch. We\nadd a feature and make sure the feature works correctly and we also\nmake sure that the feature does not misfire when the user does not\nask it to trigger, all in the same commit.\n"},{"id":"519188","messageId":"xmqqbjrbhtgw.fsf@gitster.g","threadId":"63539","inReplyTo":"20250528130116.21534-2-patrik@pspdfkit.com","subject":"Re: [PATCH 1/2] contrib/subtree: Add -S/--gpg-sign option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-29T23:45:19Z","receivedAt":"2025-05-29T23:45:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrik Weiskircher writes:\n\n> +# Usage: gpg_sign_opt\n> +# Returns the GPG signing option for git commit-tree\n> +gpg_sign_opt () {\n> +\tif test \"${arg_gpg_sign+set}\" = \"set\"\n> +\tthen\n> +\t\tif test -n \"$arg_gpg_sign\"\n> +\t\tthen\n> +\t\t\tprintf \" -S%s\" \"$arg_gpg_sign\"\n> +\t\telse\n> +\t\t\tprintf \" -S\"\n> +\t\tfi\n> +\tfi\n> +}\n\nI wonder if this misbehaves if the user has arg_gpg_sign defined in\ntheir environment. I guess that this contrib script is written\nloosely in that regard, as none of the other arg_* variables are\ncleared upon script start-up, so let's let it pass.\n\nAlso, why is this a separate function that needs to be used via\n$(command substitution)? Wouldn't it be more natural to have\nsomething like\n\n\tif test \"${arg_gpg_sign+set}\" = set\n\tthen\n\t\targ_gpg_sign=-S$arg_gpg_sign\n\telse\n\t\targ_gpg_sign=\n\tfi\n\nafter the option parsing loop and have the caller just interpolate\nit as $arg_gpg_sign (without surrounding double quotes)?\n\nOr you can define arg_gpg_sign to be\n\n * unset, if the user did not give any -S option\n * -S, if the user gave as its value\n * -S, if the user only gave -S without value\n\ndirectly inside the option parsing loop, so there is no such\nclean-up needed, perhaps?\n\nNear the start of \"main\" function, there is an early pre-parse loop\nthat iterates over all the arguments and is aware of which option\ntakes its own value, i.e.\n\n\t\tcase \"$opt\" in\n\t\t\t--annotate|-b|-P|-m|--onto)\n\t\t\t\tshift\n\t\t\t\t;;\n\t\t\t--rejoin)\n\t\t\t\targ_split_rejoin=1\n\t\t\t\t;;\n\nIf you are adding \"-S\" as taking its own value separately, don't you\nneed to add it to that part of the code, next to -b, -P, and their\nfriends?\n\n> die_incompatible_opt () {\n> \tassert test \"$#\" = 2\n> @@ -240,6 +255,15 @@ main () {\n> \t\t\ttest -n \"$allow_addmerge\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n> \t\t\targ_addmerge_squash=\n> \t\t\t;;\n> +\t\t-S)\n> +\t\t\tif test $# -gt 0 && test \"${1#-}\" = \"$1\"\n> +\t\t\tthen\n> +\t\t\t\targ_gpg_sign=\"$1\"\n> +\t\t\t\tshift\n> +\t\t\telse\n> +\t\t\t\targ_gpg_sign=\"\"\n> +\t\t\tfi\n> +\t\t\t;;\n\nThe comparison between \"${1#-}\" and \"$1\" is \"does the next argument\nstart with a dash?\", iow, you attempt to parse this\n\n\tgit subtree -S -P prefix add HEAD\n\nas \"-S\" with empty arg_gpg_sign, instead of slurping \"-P\" as its\nargument. Which is nice, but is that enough to parse this\n\n\tgit subtree -S add HEAD\n\ncorrectly?\n\n> +'git subtree' [] -P [-S[]] add \n> +'git subtree' [] -P [-S[]] add \n> +'git subtree' [] -P [-S[]] merge []\n> +'git subtree' [] -P [-S[]] split []\n\nThese synopsis elements suggest that should be given in the\n\"stuck\" form as the value for the \"-S\" option, and if that were the\ncase, your option parser would look more like\n\n\n\t\t-S*)\n\t\t\targ_gpg_sign=${opt#-S}\n\t\t\t;;\n\nwhich is much simpler and more robust. If you go that route, you\nwould probably want to retrofit -P and other options to also allow\ntheir value in the \"stuck\" form as well as their traditional\n\"separate\" form, which would also be annoying, so I do not know if\nit is worth it.\n\nOr just use \"arg_gpg_sign=$opt\" here, if you are following an\nearlier suggestion to get rid of the gpg_sign_opt helper function\nthat does not seem to be doing anything useful. And then ...\n\n> @@ -537,7 +561,7 @@ copy_commit () {\n> \t\t\tprintf \"%s\" \"$arg_split_annotate\"\n> \t\t\tcat\n> \t\t) |\n> -\t\tgit commit-tree \"$2\" $3 # reads the rest of stdin\n> +\t\tgit commit-tree \"$2\" $(gpg_sign_opt) $3 # reads the rest of stdin\n\n... here, you'd just say\n\n\t\tgit commit-tree \"$2\" $gpg_sign_opt $3 # reads the rest of stdin\n\ninstead. If your has $IFS whitespace, this will break, but\nyour gpg_sign_out function does not help with that issue anyway\n(you'd need to rewrite this and other callers to use eval if you\nreally wanted to treat $IFS whitespace in values properly, but\nlooking at the existing code in this script, I do not see any\ncareful string handling to do so, so it may be more appropriate and\npunt, saying \"no, key IDs with whitespaces in them are not\nsupported\").\n\n"},{"id":"519247","messageId":"CANMzfzhhHX1J_vPH-DMD2kPmQ6G_6YUV=7aZ5JojGy-LU6guAQ@mail.gmail.com","threadId":"63539","inReplyTo":"xmqqfrgnhuuw.fsf@gitster.g","subject":"Re: [PATCH 0/2] contrib/subtree: Add -S/--gpg-sign option","fromName":"Patrik Weiskircher","fromEmail":"patrik.weiskircher@nutrient.io","sentAt":"2025-05-30T14:33:23Z","receivedAt":"2025-05-30T14:33:36Z","isPatch":true,"sender":{"key":"patrik.weiskircher@nutrient.io","avatar":null},"body":"On Thu, May 29, 2025 at 7:15 PM Junio C Hamano wrote:\n>\n> Patrik Weiskircher writes:\n>\n> > Hi!\n> >\n> > We use git subtree a lot to manage our dependencies, but recently started\n> > requiring signed commits. This patch adds support for signing commits to\n> > git subtree.\n> >\n> > This is my first submission to the Git project - you don't have to be gentle,\n> > but please let me know if I can improve anything.\n\nApologies, Junio. I replied only to you and with HTML emails. I'm very\nnew here and I'm very much used to different workflows. I'll be\nbetter. Please disregard my previous emails.\n\n>\n> A few things ;-) starting from the log message where you didn't\n> quite add why we would want to do these changes.\n>\n> Also, I think these two should be combined into a single patch. We\n> add a feature and make sure the feature works correctly and we also\n> make sure that the feature does not misfire when the user does not\n> ask it to trigger, all in the same commit.\n\nI looked at your review, and I agree with everything you said. My\npatch wasn't very well thought out.\n\nI'm also at a crossroads here. I'm not sure if the optional argument\nparsing will work out nicely in bash. Let me explain:\n\ngit-subtree uses `git rev-parse --parseopt`. This *does* have support\nfor optional arguments, but highly recommends using `--stuck-long`,\notherwise unambiguously parsing arguments becomes impossible.\n\nThis is true. There's no way to differentiate between the optional\nargument or the next positional parameter. Therefore, I would have to\nuse `--stuck-long`.\n\nThis in itself isn't overly difficult. There's only a couple of\nparameters in the script that that would affect. The problem I have\nwith it is that I don't see a single user of `--stuck-long` in the\nwhole repo. I don't necessarily want to become the first one. Seems\nlike something destined to be cleaned up at some point.\n\nThis means my options become:\n1. use --stuck-long or\n2. make -S not take any parameter or\n3. make -S require a parameter\n\nI'm not happy with any of these options, but considering I really\nwould like to make this work, I would probably go with 1.\n\nAny opinions on this?\n\nThanks!\nPatrik\n"},{"id":"519291","messageId":"xmqqo6v9es3s.fsf@gitster.g","threadId":"63539","inReplyTo":"CANMzfzhhHX1J_vPH-DMD2kPmQ6G_6YUV=7aZ5JojGy-LU6guAQ@mail.gmail.com","subject":"Re: [PATCH 0/2] contrib/subtree: Add -S/--gpg-sign option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-30T20:55:19Z","receivedAt":"2025-05-30T20:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrik Weiskircher writes:\n\n> This means my options become:\n> 1. use --stuck-long or\n> 2. make -S not take any parameter or\n> 3. make -S require a parameter\n\n4. accept \"-S[]\" in stuck form, and --gpg-sign[=]\n\nwhich is a variant of #1 may be an option?\n\nThanks.\n\n"},{"id":"519457","messageId":"CANMzfzgggS0T9=5FtTdDi0PmVyfQJSp3_nyhj8FxUCj1wSpnZw@mail.gmail.com","threadId":"63539","inReplyTo":"xmqqo6v9es3s.fsf@gitster.g","subject":"Re: [PATCH 0/2] contrib/subtree: Add -S/--gpg-sign option","fromName":"Patrik Weiskircher","fromEmail":"patrik.weiskircher@nutrient.io","sentAt":"2025-06-02T13:12:52Z","receivedAt":"2025-06-02T13:13:05Z","isPatch":true,"sender":{"key":"patrik.weiskircher@nutrient.io","avatar":null},"body":"On Fri, May 30, 2025 at 4:55 PM Junio C Hamano wrote:\n>\n> Patrik Weiskircher writes:\n>\n> > This means my options become:\n> > 1. use --stuck-long or\n> > 2. make -S not take any parameter or\n> > 3. make -S require a parameter\n>\n> 4. accept \"-S[]\" in stuck form, and --gpg-sign[=]\n>\n> which is a variant of #1 may be an option?\n\nNot with git rev-parse, unfortunately. Either `-Sabc` or\n`--gpg-sign=abc` would get transformed into `-S 'abc'` after\nrev-parse, where we run into the ambiguity of what is an optional\nargument again. I *could* parse this manually without git rev-parse\nbeforehand, but that seems like just adding yet-another difficult to\nmaintain thing.\n\n```\n$ echo $OPTS_SPEC | git rev-parse --parseopt -- \"--gpg-sign=abcd\"\nset -- -S 'abcd' --\n\n$ echo $OPTS_SPEC | git rev-parse --parseopt -- \"-Sabcd\"\nset -- -S 'abcd' --\n```\n"}]}