[PATCH v2] subtree: fix add and pull for GPG-signed commits

[PATCH v2] subtree: fix add and pull for GPG-signed commits

[Stephen R Guglielmo]

If log.showsignature is true (or --show-signature is passed) while
performing a `subtree add` or `subtree pull`, the command fails.

toptree_for_commit() calls `log` and passes the output to `commit-tree`.
If this output shows the GPG signature data, `commit-tree` throws a
fatal error.

This commit fixes the issue by adding --no-show-signature to `log` calls
in a few places, as well as using the more appropriate `rev-parse`
instead where possible.

Signed-off-by: Stephen R Guglielmo <srg@guglielmo.us>
---
 contrib/subtree/git-subtree.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index dec085a23..9594ca4b5 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -297,7 +297,7 @@ find_latest_squash () {
  main=
  sub=
  git log --grep="^git-subtree-dir: $dir/*\$" \
- --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |
+ --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |
  while read a b junk
  do
  debug "$a $b $junk"
@@ -341,7 +341,7 @@ find_existing_splits () {
  main=
  sub=
  git log --grep="^git-subtree-dir: $dir/*\$" \
- --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |
+ --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |
  while read a b junk
  do
  case "$a" in
@@ -382,7 +382,7 @@ copy_commit () {
  # We're going to set some environment vars here, so
  # do it in a subshell to get rid of them safely later
  debug copy_commit "{$1}" "{$2}" "{$3}"
- git log -1 --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' "$1" |
+ git log --no-show-signature -1
--pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' "$1" |
  (
  read GIT_AUTHOR_NAME
  read GIT_AUTHOR_EMAIL
@@ -462,8 +462,8 @@ squash_msg () {
  oldsub_short=$(git rev-parse --short "$oldsub")
  echo "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
  echo
- git log --pretty=tformat:'%h %s' "$oldsub..$newsub"
- git log --pretty=tformat:'REVERT: %h %s' "$newsub..$oldsub"
+ git log --no-show-signature --pretty=tformat:'%h %s' "$oldsub..$newsub"
+ git log --no-show-signature --pretty=tformat:'REVERT: %h %s'
"$newsub..$oldsub"
  else
  echo "Squashed '$dir/' content from commit $newsub_short"
  fi
@@ -475,7 +475,7 @@ squash_msg () {

 toptree_for_commit () {
  commit="$1"
- git log -1 --pretty=format:'%T' "$commit" -- || exit $?
+ git rev-parse --verify "$commit^{tree}" || exit $?
 }

 subtree_for_commit () {
-- 
2.16.2

Re: [PATCH v2] subtree: fix add and pull for GPG-signed commits

[Junio C Hamano]

Stephen R Guglielmo <srguglielmo@gmail.com> writes:

> If log.showsignature is true (or --show-signature is passed) while
> performing a `subtree add` or `subtree pull`, the command fails.
>
> toptree_for_commit() calls `log` and passes the output to `commit-tree`.
> If this output shows the GPG signature data, `commit-tree` throws a
> fatal error.
>
> This commit fixes the issue by adding --no-show-signature to `log` calls
> in a few places, as well as using the more appropriate `rev-parse`
> instead where possible.
>
> Signed-off-by: Stephen R Guglielmo <srg@guglielmo.us>
> ---
>  contrib/subtree/git-subtree.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

This was too heavily whitespace damaged so I recreated your patch
manually from scratch and queued, during which time I may have made
silly and simple mistakes.  Please double check what appears on the
'pu' branch in a few hours.

Thanks.

I am however starting to feel that

 (1) add gitlog="git log" and then do s/git log/$gitlog/; to the
     remainder of the whole script in patch 1/2; and

 (2) turn the variable definition to gitlog="git log --no-show-signature"
     in patch 2/2

may be a better approach.  After all, this script is not prepared to
be used by any group of people who use signed commits, and showing
commit signature in any of its use of 'git log', either present or
in the future, will not be useful to it, I suspect.

>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index dec085a23..9594ca4b5 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -297,7 +297,7 @@ find_latest_squash () {
>   main=
>   sub=
>   git log --grep="^git-subtree-dir: $dir/*\$" \
> - --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |
> + --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |
>   while read a b junk
>   do
>   debug "$a $b $junk"
> @@ -341,7 +341,7 @@ find_existing_splits () {
>   main=
>   sub=
>   git log --grep="^git-subtree-dir: $dir/*\$" \
> - --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |
> + --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |
>   while read a b junk
>   do
>   case "$a" in
> @@ -382,7 +382,7 @@ copy_commit () {
>   # We're going to set some environment vars here, so
>   # do it in a subshell to get rid of them safely later
>   debug copy_commit "{$1}" "{$2}" "{$3}"
> - git log -1 --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' "$1" |
> + git log --no-show-signature -1
> --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' "$1" |
>   (
>   read GIT_AUTHOR_NAME
>   read GIT_AUTHOR_EMAIL
> @@ -462,8 +462,8 @@ squash_msg () {
>   oldsub_short=$(git rev-parse --short "$oldsub")
>   echo "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
>   echo
> - git log --pretty=tformat:'%h %s' "$oldsub..$newsub"
> - git log --pretty=tformat:'REVERT: %h %s' "$newsub..$oldsub"
> + git log --no-show-signature --pretty=tformat:'%h %s' "$oldsub..$newsub"
> + git log --no-show-signature --pretty=tformat:'REVERT: %h %s'
> "$newsub..$oldsub"
>   else
>   echo "Squashed '$dir/' content from commit $newsub_short"
>   fi
> @@ -475,7 +475,7 @@ squash_msg () {
>
>  toptree_for_commit () {
>   commit="$1"
> - git log -1 --pretty=format:'%T' "$commit" -- || exit $?
> + git rev-parse --verify "$commit^{tree}" || exit $?
>  }
>
>  subtree_for_commit () {

Re: [PATCH v2] subtree: fix add and pull for GPG-signed commits

[Stephen R Guglielmo]

On Fri, Feb 23, 2018 at 5:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stephen R Guglielmo <srguglielmo@gmail.com> writes:
>
>> If log.showsignature is true (or --show-signature is passed) while
>> performing a `subtree add` or `subtree pull`, the command fails.
>>
>> toptree_for_commit() calls `log` and passes the output to `commit-tree`.
>> If this output shows the GPG signature data, `commit-tree` throws a
>> fatal error.
>>
>> This commit fixes the issue by adding --no-show-signature to `log` calls
>> in a few places, as well as using the more appropriate `rev-parse`
>> instead where possible.
>>
>> Signed-off-by: Stephen R Guglielmo <srg@guglielmo.us>
>> ---
>>  contrib/subtree/git-subtree.sh | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> This was too heavily whitespace damaged so I recreated your patch
> manually from scratch and queued, during which time I may have made
> silly and simple mistakes.  Please double check what appears on the
> 'pu' branch in a few hours.
>
> Thanks.
>
> I am however starting to feel that
>
>  (1) add gitlog="git log" and then do s/git log/$gitlog/; to the
>      remainder of the whole script in patch 1/2; and
>
>  (2) turn the variable definition to gitlog="git log --no-show-signature"
>      in patch 2/2
>
> may be a better approach.  After all, this script is not prepared to
> be used by any group of people who use signed commits, and showing
> commit signature in any of its use of 'git log', either present or
> in the future, will not be useful to it, I suspect.


Hi Junio,

I can confirm the changes to the pu branch looks good. I apologize for
the whitespace issue; Gmail must've mangled it.

I'm happy to develop a new patch based on your recommendations. Should
it be on top of the previous patch I sent or should it replace the
previous patch?

Thanks,
Steve

Re: [PATCH v2] subtree: fix add and pull for GPG-signed commits

[Junio C Hamano]

Stephen R Guglielmo <srguglielmo@gmail.com> writes:

> On Fri, Feb 23, 2018 at 5:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>...
>> I am however starting to feel that
>> ...
>> may be a better approach.
> ...
> I'm happy to develop a new patch based on your recommendations. Should
> it be on top of the previous patch I sent or should it replace the
> previous patch?

Even though I said "may be a better approach", to be bluntly honest,
I do not think it is _that_ _much_ better than what you sent ;-)  So
please do the "on top of the previous patch" thing as an independent
effort (not the "git log" workaround bugfix) only if you deeply care
about improving "subtree" script (as opposed to just want to see the
immediate glitch corrected to get on with your life---I happen to be
in the latter category with respect to this issue).  The independent
effort's focus would instead be to improve the script not to make so
heavy use of "git log" (and other Porcelain commands) in it, so that
we do not have to tweak the script to undo improvements we will make
to the Porcelain commands for better human experience in the future.

Notice that one hunk in this patch is a small step in the direction;
it stops using "git log -1" and uses "rev-parse" instead.

For the remainder of the script, we need to identify how "git log"
is used in the script and what they are used for, and then rewrite
them with the more stable lower level interface.  It is a very first
step to mark invocation sites by replacing "git log" with "$git_log"
;-)  

The same may apply to uses of other Porcelain commands.  A general
rule is that scripts should avoid using Porcelain commands unless
they are interested in giving output for human-consumption directly
out of these commands (as opposed to running the Git commands and
then reading their output and reacting to it).

Thanks.