Bug in git tab completion: If git.showSignature = true is set globally, tab completion of commits is garbled

Bug in git tab completion: If git.showSignature = true is set globally, tab completion of commits is garbled

[Adrian Vollmer]

Hi,

to reproduce this bug, set showSignature = true in the log section of
your global git config file, then type git reset <TAB> in a git repo
that has a recent signed commit in its history.

It looks like this:

    README überarbeitet (23 hours ago)       -- [README überarbeitet (23 hours ago)] gpg: Signatu
    gpg                                      --                 issuer "<censored>":[g
    Primary key fingerprint                  --  XXXX XXXX XXXX XXXX XXXX  XXXX XXXX XXXX XXXX 2F
    7ded199                                  -- [7ded199]
    92fdb4d                                  -- [92fdb4d] gpg:                using RSA key XXXXX
    gpg                                      --  Good signature from "<censored>
    92fdb4d                                  -- [92fdb4d] formatting (2 days ago)

Cheers,
Adrian

Re: Bug in git tab completion: If git.showSignature = true is set globally, tab completion of commits is garbled

[dana]

On 7 Mar 2019, at 13:23, Adrian Vollmer <zsh@vollmer.syss.de> wrote:
>to reproduce this bug, set showSignature = true in the log section of
>your global git config file, then type git reset <TAB> in a git repo
>that has a recent signed commit in its history.

I guess this is the simple fix for this particular case. `git log` is called
in a few other places, too, but i'm not sure if they're problematic. Probably
are and i just don't know how to trigger it.

Maybe it would make sense to do something more robust instead, though? For
example, we could have a __git_call wrapper around _call_program that always
calls git with a safe set of -c options, &c. It'd be a lot of lines changed,
but any further issues like this could be fixed in one place. idk.

dana


diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index e5e4ee768..796a98fd5 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -6517,7 +6517,7 @@ __git_recent_commits () {
 
   # Careful: most %d will expand to the empty string.  Quote properly!
   # NOTE: we could use %D directly, but it's not available in git 1.9.1 at least.
-  commits=("${(f)"$(_call_program commits git --no-pager log ${(q)commit_opts} -20 --format='%h%n%d%n%s\ \(%cr\)%n%p')"}")
+  commits=("${(f)"$(_call_program commits git -c log.showSignature=false --no-pager log ${(q)commit_opts} -20 --format='%h%n%d%n%s\ \(%cr\)%n%p')"}")
   __git_command_successful $pipestatus || return 1
 
   for i j k parents in "$commits[@]" ; do

Re: Bug in git tab completion: If git.showSignature = true is set globally, tab completion of commits is garbled

[Daniel Shahaf]

dana wrote on Thu, 07 Mar 2019 23:28 +00:00:
> On 7 Mar 2019, at 13:23, Adrian Vollmer <zsh@vollmer.syss.de> wrote:
> >to reproduce this bug, set showSignature = true in the log section of
> >your global git config file, then type git reset <TAB> in a git repo
> >that has a recent signed commit in its history.
> 
> I guess this is the simple fix for this particular case. `git log` is called
> in a few other places, too, but i'm not sure if they're problematic. Probably
> are and i just don't know how to trigger it.
> 
> Maybe it would make sense to do something more robust instead, though? For
> example, we could have a __git_call wrapper around _call_program that always
> calls git with a safe set of -c options, &c. It'd be a lot of lines changed,
> but any further issues like this could be fixed in one place. idk.
> 

Should we use git-rev-list(1) instead of git-log(1)?  The former is a
plumbing command, so should be the more appropriate interface for this
use-case.

My git(1) does not have the log.showSignature option so I couldn't test
this.  (The knob is missing from _git-config() too.)

Cheers,

Daniel

Re: Bug in git tab completion: If git.showSignature = true is set globally, tab completion of commits is garbled

[dana]

On 8 Mar 2019, at 14:22, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>Should we use git-rev-list(1) instead of git-log(1)?  The former is a
>plumbing command, so should be the more appropriate interface for this
>use-case.

Not sure. For a plumbing command it leaves a bit to be desired; it has no -z
and it doesn't really respect the format you tell it to use. In fact the
'commit 1234567890...' header it prints is actually hard-coded. It *does* seem
to ignore --show-signature/log.showSignature though, so i suppose it would
solve that problem

dana

Re: Bug in git tab completion: If git.showSignature = true is set globally, tab completion of commits is garbled

[Daniel Shahaf]

dana wrote on Sat, 09 Mar 2019 16:26 +00:00:
> On 8 Mar 2019, at 14:22, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >Should we use git-rev-list(1) instead of git-log(1)?  The former is a
> >plumbing command, so should be the more appropriate interface for this
> >use-case.
> 
> Not sure. For a plumbing command it leaves a bit to be desired; it has no -z

It does have %x00, though.

> and it doesn't really respect the format you tell it to use. In fact the
> 'commit 1234567890...' header it prints is actually hard-coded.

Huh.  Odd, but I think we can live with that: we can just have _git
discard the hardcoded header line that git emits.  All we really care
about is that future git versions will continue to emit that line.  I
assume they will, for compatibility reasons.

Cheers,

Daniel

Re: Bug in git tab completion: If git.showSignature = true is set globally, tab completion of commits is garbled

[dana]

On 9 Mar 2019, at 10:43, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>Huh.  Odd, but I think we can live with that: we can just have _git
>discard the hardcoded header line that git emits.  All we really care
>about is that future git versions will continue to emit that line.  I
>assume they will, for compatibility reasons.

In that case, i think it would be something like this? I didn't test very
extensively (not even sure which paths exactly lead to these commands), but it
does seem to work for the scenario at hand

(--oneline is hard-coded to not print the hard-coded header)

dana


diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index e5e4ee768..4eb07e921 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5652,7 +5652,7 @@ __git_describe_branch () {
     local __c
     local -a __commits
     for __c in ${(P)__commits_in}; do
-      __commits+=("${__c}:${$(_call_program describe git log -1 --oneline $__c)//:/\\:}")
+      __commits+=("${__c}:${$(_call_program describe git rev-list -1 --oneline $__c)//:/\\:}")
     done
     _describe -t $__tag $__desc __commits "$@"
   else
@@ -6493,8 +6493,9 @@ __git_commit_objects () {
 
   # Note: the after-the-colon part must be unique across the entire array;
   # see workers/34768
-  commits=(${(f)"$(_call_program commits git --no-pager log -1000 --all --reflog --format='%h:\[%h\]\ %s\ \(%cr\)')"})
+  commits=(${(f)"$(_call_program commits git --no-pager rev-list -1000 --all --reflog --format='%h:\[%h\]\ %s\ \(%cr\)' HEAD)"})
   __git_command_successful $pipestatus || return 1
+  commits=(${commits:#commit [[:xdigit:]](#c40,)})
 
   _describe -Vx -t commits 'commit object name' commits
 }
@@ -6503,7 +6504,7 @@ __git_commit_objects () {
 __git_recent_commits () {
   local gitdir expl start
   declare -a descr tags heads commits argument_array_names commit_opts
-  local i j k ret
+  local h i j k ret
   integer distance_from_head
   local label
   local parents
@@ -6517,10 +6518,11 @@ __git_recent_commits () {
 
   # Careful: most %d will expand to the empty string.  Quote properly!
   # NOTE: we could use %D directly, but it's not available in git 1.9.1 at least.
-  commits=("${(f)"$(_call_program commits git --no-pager log ${(q)commit_opts} -20 --format='%h%n%d%n%s\ \(%cr\)%n%p')"}")
+  commits=("${(f)"$(_call_program commits git --no-pager rev-list ${(q)commit_opts} -20 --format='%h%n%d%n%s\ \(%cr\)%n%p' HEAD)"}")
   __git_command_successful $pipestatus || return 1
 
-  for i j k parents in "$commits[@]" ; do
+  # h => hard-coded 'commit abcdef1234567890...' -- just discarded
+  for h i j k parents in "$commits[@]" ; do
     # Note: the after-the-colon part must be unique across the entire array;
     # see workers/34768
     if (( $#commit_opts )); then

Re: Bug in git tab completion: If git.showSignature = true is set globally, tab completion of commits is garbled

[Daniel Shahaf]

dana wrote on Sat, Mar 09, 2019 at 17:37:47 -0600:
> On 9 Mar 2019, at 10:43, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >Huh.  Odd, but I think we can live with that: we can just have _git
> >discard the hardcoded header line that git emits.  All we really care
> >about is that future git versions will continue to emit that line.  I
> >assume they will, for compatibility reasons.
> 
> In that case, i think it would be something like this? I didn't test very
> extensively (not even sure which paths exactly lead to these commands), but it
> does seem to work for the scenario at hand

Looks good.  Thanks.  More below regarding the codepaths to these.

> +++ b/Completion/Unix/Command/_git
> @@ -5652,7 +5652,7 @@ __git_describe_branch () {

Looking at the callers of this, it should be used by «git checkout <TAB>» (one
of the alternatives is "basenames" of remote branches), refspecs in «git push
$remote …», etc; but an easier way to test this would be to run
.
    compdef __git_describe_commit f
    f <TAB>
.
interactively.

> @@ -6493,8 +6493,9 @@ __git_commit_objects () {

This function completes hashes of commits, as output by `git log --pretty=%h`.
It's used by _git-send-email.  (I'm surprised that it isn't used by
_git-format-patch; those two commands are very similar.)

> @@ -6517,10 +6518,11 @@ __git_recent_commits () {

This is used by «git commit --fixup=<TAB>».

Cheers,

Daniel