{"thread":{"id":"47914","subject":"[PATCH v2] subtree: fix add and pull for GPG-signed commits","startedAt":"2018-02-23T20:41:33Z","lastAt":"2018-02-26T17:28:31Z","messageCount":4,"participants":["Stephen R Guglielmo","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"340069","messageId":"CADfK3RVJ9pYtpX9x2=CZSKLVy2qxBKeyyGA_S=jo8K-Fa4FOqA@mail.gmail.com","threadId":"47914","inReplyTo":null,"subject":"[PATCH v2] subtree: fix add and pull for GPG-signed commits","fromName":"Stephen R Guglielmo","fromEmail":"srguglielmo@gmail.com","sentAt":"2018-02-23T20:41:25Z","receivedAt":"2018-02-23T20:41:33Z","isPatch":true,"sender":{"key":"srguglielmo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7794973?v=4"},"body":"If log.showsignature is true (or --show-signature is passed) while\nperforming a `subtree add` or `subtree pull`, the command fails.\n\ntoptree_for_commit() calls `log` and passes the output to `commit-tree`.\nIf this output shows the GPG signature data, `commit-tree` throws a\nfatal error.\n\nThis commit fixes the issue by adding --no-show-signature to `log` calls\nin a few places, as well as using the more appropriate `rev-parse`\ninstead where possible.\n\nSigned-off-by: Stephen R Guglielmo \n---\n contrib/subtree/git-subtree.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex dec085a23..9594ca4b5 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -297,7 +297,7 @@ find_latest_squash () {\n main=\n sub=\n git log --grep=\"^git-subtree-dir: $dir/*\\$\" \\\n- --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |\n+ --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |\n while read a b junk\n do\n debug \"$a $b $junk\"\n@@ -341,7 +341,7 @@ find_existing_splits () {\n main=\n sub=\n git log --grep=\"^git-subtree-dir: $dir/*\\$\" \\\n- --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |\n+ --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |\n while read a b junk\n do\n case \"$a\" in\n@@ -382,7 +382,7 @@ copy_commit () {\n # We're going to set some environment vars here, so\n # do it in a subshell to get rid of them safely later\n debug copy_commit \"{$1}\" \"{$2}\" \"{$3}\"\n- git log -1 --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' \"$1\" |\n+ git log --no-show-signature -1\n--pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' \"$1\" |\n (\n read GIT_AUTHOR_NAME\n read GIT_AUTHOR_EMAIL\n@@ -462,8 +462,8 @@ squash_msg () {\n oldsub_short=$(git rev-parse --short \"$oldsub\")\n echo \"Squashed '$dir/' changes from $oldsub_short..$newsub_short\"\n echo\n- git log --pretty=tformat:'%h %s' \"$oldsub..$newsub\"\n- git log --pretty=tformat:'REVERT: %h %s' \"$newsub..$oldsub\"\n+ git log --no-show-signature --pretty=tformat:'%h %s' \"$oldsub..$newsub\"\n+ git log --no-show-signature --pretty=tformat:'REVERT: %h %s'\n\"$newsub..$oldsub\"\n else\n echo \"Squashed '$dir/' content from commit $newsub_short\"\n fi\n@@ -475,7 +475,7 @@ squash_msg () {\n\n toptree_for_commit () {\n commit=\"$1\"\n- git log -1 --pretty=format:'%T' \"$commit\" -- || exit $?\n+ git rev-parse --verify \"$commit^{tree}\" || exit $?\n }\n\n subtree_for_commit () {\n-- \n2.16.2\n"},{"id":"340090","messageId":"xmqq7er3s4t7.fsf@gitster-ct.c.googlers.com","threadId":"47914","inReplyTo":"CADfK3RVJ9pYtpX9x2=CZSKLVy2qxBKeyyGA_S=jo8K-Fa4FOqA@mail.gmail.com","subject":"Re: [PATCH v2] subtree: fix add and pull for GPG-signed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-23T22:45:56Z","receivedAt":"2018-02-23T22:46:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen R Guglielmo writes:\n\n> If log.showsignature is true (or --show-signature is passed) while\n> performing a `subtree add` or `subtree pull`, the command fails.\n>\n> toptree_for_commit() calls `log` and passes the output to `commit-tree`.\n> If this output shows the GPG signature data, `commit-tree` throws a\n> fatal error.\n>\n> This commit fixes the issue by adding --no-show-signature to `log` calls\n> in a few places, as well as using the more appropriate `rev-parse`\n> instead where possible.\n>\n> Signed-off-by: Stephen R Guglielmo \n> ---\n> contrib/subtree/git-subtree.sh | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n\nThis was too heavily whitespace damaged so I recreated your patch\nmanually from scratch and queued, during which time I may have made\nsilly and simple mistakes. Please double check what appears on the\n'pu' branch in a few hours.\n\nThanks.\n\nI am however starting to feel that\n\n (1) add gitlog=\"git log\" and then do s/git log/$gitlog/; to the\n remainder of the whole script in patch 1/2; and\n\n (2) turn the variable definition to gitlog=\"git log --no-show-signature\"\n in patch 2/2\n\nmay be a better approach. After all, this script is not prepared to\nbe used by any group of people who use signed commits, and showing\ncommit signature in any of its use of 'git log', either present or\nin the future, will not be useful to it, I suspect.\n\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index dec085a23..9594ca4b5 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -297,7 +297,7 @@ find_latest_squash () {\n> main=\n> sub=\n> git log --grep=\"^git-subtree-dir: $dir/*\\$\" \\\n> - --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |\n> + --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |\n> while read a b junk\n> do\n> debug \"$a $b $junk\"\n> @@ -341,7 +341,7 @@ find_existing_splits () {\n> main=\n> sub=\n> git log --grep=\"^git-subtree-dir: $dir/*\\$\" \\\n> - --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |\n> + --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |\n> while read a b junk\n> do\n> case \"$a\" in\n> @@ -382,7 +382,7 @@ copy_commit () {\n> # We're going to set some environment vars here, so\n> # do it in a subshell to get rid of them safely later\n> debug copy_commit \"{$1}\" \"{$2}\" \"{$3}\"\n> - git log -1 --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' \"$1\" |\n> + git log --no-show-signature -1\n> --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' \"$1\" |\n> (\n> read GIT_AUTHOR_NAME\n> read GIT_AUTHOR_EMAIL\n> @@ -462,8 +462,8 @@ squash_msg () {\n> oldsub_short=$(git rev-parse --short \"$oldsub\")\n> echo \"Squashed '$dir/' changes from $oldsub_short..$newsub_short\"\n> echo\n> - git log --pretty=tformat:'%h %s' \"$oldsub..$newsub\"\n> - git log --pretty=tformat:'REVERT: %h %s' \"$newsub..$oldsub\"\n> + git log --no-show-signature --pretty=tformat:'%h %s' \"$oldsub..$newsub\"\n> + git log --no-show-signature --pretty=tformat:'REVERT: %h %s'\n> \"$newsub..$oldsub\"\n> else\n> echo \"Squashed '$dir/' content from commit $newsub_short\"\n> fi\n> @@ -475,7 +475,7 @@ squash_msg () {\n>\n> toptree_for_commit () {\n> commit=\"$1\"\n> - git log -1 --pretty=format:'%T' \"$commit\" -- || exit $?\n> + git rev-parse --verify \"$commit^{tree}\" || exit $?\n> }\n>\n> subtree_for_commit () {\n"},{"id":"340305","messageId":"CADfK3RXa9wngOZ9YmurThm=Xu8J=nq0tK5yaMOMT4T0be2sUZA@mail.gmail.com","threadId":"47914","inReplyTo":"xmqq7er3s4t7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] subtree: fix add and pull for GPG-signed commits","fromName":"Stephen R Guglielmo","fromEmail":"srguglielmo@gmail.com","sentAt":"2018-02-26T14:46:32Z","receivedAt":"2018-02-26T14:46:40Z","isPatch":true,"sender":{"key":"srguglielmo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7794973?v=4"},"body":"On Fri, Feb 23, 2018 at 5:45 PM, Junio C Hamano wrote:\n> Stephen R Guglielmo writes:\n>\n>> If log.showsignature is true (or --show-signature is passed) while\n>> performing a `subtree add` or `subtree pull`, the command fails.\n>>\n>> toptree_for_commit() calls `log` and passes the output to `commit-tree`.\n>> If this output shows the GPG signature data, `commit-tree` throws a\n>> fatal error.\n>>\n>> This commit fixes the issue by adding --no-show-signature to `log` calls\n>> in a few places, as well as using the more appropriate `rev-parse`\n>> instead where possible.\n>>\n>> Signed-off-by: Stephen R Guglielmo \n>> ---\n>> contrib/subtree/git-subtree.sh | 12 ++++++------\n>> 1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> This was too heavily whitespace damaged so I recreated your patch\n> manually from scratch and queued, during which time I may have made\n> silly and simple mistakes. Please double check what appears on the\n> 'pu' branch in a few hours.\n>\n> Thanks.\n>\n> I am however starting to feel that\n>\n> (1) add gitlog=\"git log\" and then do s/git log/$gitlog/; to the\n> remainder of the whole script in patch 1/2; and\n>\n> (2) turn the variable definition to gitlog=\"git log --no-show-signature\"\n> in patch 2/2\n>\n> may be a better approach. After all, this script is not prepared to\n> be used by any group of people who use signed commits, and showing\n> commit signature in any of its use of 'git log', either present or\n> in the future, will not be useful to it, I suspect.\n\n\nHi Junio,\n\nI can confirm the changes to the pu branch looks good. I apologize for\nthe whitespace issue; Gmail must've mangled it.\n\nI'm happy to develop a new patch based on your recommendations. Should\nit be on top of the previous patch I sent or should it replace the\nprevious patch?\n\nThanks,\nSteve\n"},{"id":"340312","messageId":"xmqqwoyzr77r.fsf@gitster-ct.c.googlers.com","threadId":"47914","inReplyTo":"CADfK3RXa9wngOZ9YmurThm=Xu8J=nq0tK5yaMOMT4T0be2sUZA@mail.gmail.com","subject":"Re: [PATCH v2] subtree: fix add and pull for GPG-signed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-26T17:28:24Z","receivedAt":"2018-02-26T17:28:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen R Guglielmo writes:\n\n> On Fri, Feb 23, 2018 at 5:45 PM, Junio C Hamano wrote:\n>...\n>> I am however starting to feel that\n>> ...\n>> may be a better approach.\n> ...\n> I'm happy to develop a new patch based on your recommendations. Should\n> it be on top of the previous patch I sent or should it replace the\n> previous patch?\n\nEven though I said \"may be a better approach\", to be bluntly honest,\nI do not think it is _that_ _much_ better than what you sent ;-) So\nplease do the \"on top of the previous patch\" thing as an independent\neffort (not the \"git log\" workaround bugfix) only if you deeply care\nabout improving \"subtree\" script (as opposed to just want to see the\nimmediate glitch corrected to get on with your life---I happen to be\nin the latter category with respect to this issue). The independent\neffort's focus would instead be to improve the script not to make so\nheavy use of \"git log\" (and other Porcelain commands) in it, so that\nwe do not have to tweak the script to undo improvements we will make\nto the Porcelain commands for better human experience in the future.\n\nNotice that one hunk in this patch is a small step in the direction;\nit stops using \"git log -1\" and uses \"rev-parse\" instead.\n\nFor the remainder of the script, we need to identify how \"git log\"\nis used in the script and what they are used for, and then rewrite\nthem with the more stable lower level interface. It is a very first\nstep to mark invocation sites by replacing \"git log\" with \"$git_log\"\n;-) \n\nThe same may apply to uses of other Porcelain commands. A general\nrule is that scripts should avoid using Porcelain commands unless\nthey are interested in giving output for human-consumption directly\nout of these commands (as opposed to running the Git commands and\nthen reading their output and reacting to it).\n\nThanks.\n\n\n\n\n"}]}