zsh-workers
 help / color / Atom feed
* [PATCH] _git: Fix __git_recent_branches for case when commit has empty message
@ 2020-01-31 16:57 WGH
  2020-02-01  0:09 ` Daniel Shahaf
  0 siblings, 1 reply; 4+ messages in thread
From: WGH @ 2020-01-31 16:57 UTC (permalink / raw)
  To: zsh-workers

Although I don't think anyone sane would ever do that, but I happened to
create one on in my local repo, and, curiously, zsh git completion
broke.

Pressing "Tab" after entering "git checkout " will cause the completion
script to crash with the following error:

    __git_recent_branches:21: bad set of key/value pairs for associative array

This error happens when an associative array is assigned odd number of
elements. Unless the variable being split is inside double quotes, empty
elements are removed (which is what causes the problem).

This commit fixes it.

Since --format=%(refname)%00%(subject)%00 appends null byte to every line,
using this format would cause an extra empty element appear, breaking the
assigment again. In order to fix this, I removed the trailing null byte
from the format string, and replaced newlines separating entries with
null byte (they were previously removed). Since neither refname nor
subject appears to be capable of containing newlines, this change should
be safe.
---
 Completion/Unix/Command/_git | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index ba1852699..97ab26512 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -6662,8 +6662,8 @@ __git_recent_branches() {
 
   # 4. Obtain log messages for all of them in one shot.
   # TODO: we'd really like --sort=none here...  but git doesn't support such a thing.
-  # The \n removal is because for-each-ref prints a \n after each entry.
-  descriptions=( ${(0)"$(_call_program all-descriptions "git --no-pager for-each-ref --format='%(refname)%00%(subject)%00'" refs/heads/${(q)^branches} "--")"//$'\n'} )
+  local z=$'\0'
+  descriptions=( "${(0)"$(_call_program all-descriptions "git --no-pager for-each-ref --format='%(refname)%00%(subject)'" refs/heads/${(q)^branches} "--")"//$'\n'/$z}" )
 
   # 5. Synthesize the data structure _describe wants.
   local -a branches_colon_descriptions
-- 
2.24.1



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] _git: Fix __git_recent_branches for case when commit has empty message
  2020-01-31 16:57 [PATCH] _git: Fix __git_recent_branches for case when commit has empty message WGH
@ 2020-02-01  0:09 ` Daniel Shahaf
  2020-02-01 23:50   ` WGH
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel Shahaf @ 2020-02-01  0:09 UTC (permalink / raw)
  To: WGH; +Cc: zsh-workers

WGH wrote on Fri, 31 Jan 2020 19:57 +0300:
> Pressing "Tab" after entering "git checkout " will cause the completion
> script to crash with the following error:
> 
>     __git_recent_branches:21: bad set of key/value pairs for associative array

Thanks, I can reproduce this.

> This error happens when an associative array is assigned odd number of
> elements. Unless the variable being split is inside double quotes, empty
> elements are removed (which is what causes the problem).
> 
> This commit fixes it.

Not on my machine.

First, the unidiff was improperly serialized: the spaces at the start
of each context line were converted to non-breaking spaces (U+00A0).
I applied the patch manually, and completion did work, but stderr was
spammed with two warnings:

+__git_recent_branches:21>   local $'z=\C-@'
__git_recent_branches:21: command not found:  
+__git_recent_branches:22> _call_program all-descriptions 'git --no-pager for-each-ref --format='\''%(refname)%00%(subject)'\' refs/heads/master --                                                                
__git_recent_branches:22: unknown file attribute:

I'm not sure why that happened.  I assume "unknown file attribute"
means parentheses were taken for glob qualifiers, but I don't see why
the patch would have that effect.  I was testing in an empty
repository, produced by «cd "$(mktemp -d)" && git init && git commit
--allow-empty{,-message} && git checkout -b foo».  Does anyone see what caused
the warnings?

As to the bug, I found a workaround:

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index ba1852699..7fccc11b0 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -6663,13 +6663,14 @@ __git_recent_branches() {
   # 4. Obtain log messages for all of them in one shot.
   # TODO: we'd really like --sort=none here...  but git doesn't support such a thing.
   # The \n removal is because for-each-ref prints a \n after each entry.
+  # The x is added in case %(subject)'s expansion is the empty string.
-  descriptions=( ${(0)"$(_call_program all-descriptions "git --no-pager for-each-ref --format='%(refname)%00%(subject)%00'" refs/heads/${(q)^branches} "--")"//$'\n'} )
+  descriptions=( ${(0)"$(_call_program all-descriptions "git --no-pager for-each-ref --format='%(refname)%00x%(subject)%00'" refs/heads/${(q)^branches} "--")"//$'\n'} )
 
   # 5. Synthesize the data structure _describe wants.
   local -a branches_colon_descriptions
   local branch
   for branch in ${branches} ; do
-    branches_colon_descriptions+="${branch//:/\:}:${descriptions[refs/heads/${(b)branch}]}"
+    branches_colon_descriptions+="${branch//:/\:}:${descriptions[refs/heads/${(b)branch}]#x}"
   done
 
   _describe -V -t recent-branches "recent branches" branches_colon_descriptions

> Since --format=%(refname)%00%(subject)%00 appends null byte to every line,
> using this format would cause an extra empty element appear, breaking the
> assigment again. In order to fix this, I removed the trailing null byte
> from the format string, and replaced newlines separating entries with
> null byte (they were previously removed). Since neither refname nor
> subject appears to be capable of containing newlines, this change should
> be safe.

Agreed.

Thanks for the bug report and the patch.  I guess I'll commit my
version, since it fixes the bug, but I do want to understand why your
patch didn't work for me.

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] _git: Fix __git_recent_branches for case when commit has empty message
  2020-02-01  0:09 ` Daniel Shahaf
@ 2020-02-01 23:50   ` WGH
  2020-02-02  7:34     ` Daniel Shahaf
  0 siblings, 1 reply; 4+ messages in thread
From: WGH @ 2020-02-01 23:50 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

On 2/1/20 3:09 AM, Daniel Shahaf wrote:
> Not on my machine.
>
> First, the unidiff was improperly serialized: the spaces at the start
> of each context line were converted to non-breaking spaces (U+00A0).

Oh, damn, despite all my efforts, Thunderbird still messed the patch up. Sigh. (gotta finally find some sane mail client)

Anyway, copypasting it from the mailing list archive results in something that applies cleanly: https://www.zsh.org/mla/workers/2020/msg00190.html

> I applied the patch manually, and completion did work, but stderr was
> spammed with two warnings:
>
> +__git_recent_branches:21>   local $'z=\C-@'
> __git_recent_branches:21: command not found:

That isn't supposed to happen. This should merely assign a string contained a single null byte to variable z. Maybe you didn't clean up all those NBSP?

>  
> +__git_recent_branches:22> _call_program all-descriptions 'git --no-pager for-each-ref --format='\''%(refname)%00%(subject)'\' refs/heads/master --                                                                
> __git_recent_branches:22: unknown file attribute:

Likewise, I have no clue what it is.




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] _git: Fix __git_recent_branches for case when commit has empty message
  2020-02-01 23:50   ` WGH
@ 2020-02-02  7:34     ` Daniel Shahaf
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Shahaf @ 2020-02-02  7:34 UTC (permalink / raw)
  To: WGH; +Cc: zsh-workers

WGH wrote on Sun, 02 Feb 2020 02:50 +0300:
> On 2/1/20 3:09 AM, Daniel Shahaf wrote:
> > Not on my machine.
> >
> > First, the unidiff was improperly serialized: the spaces at the start
> > of each context line were converted to non-breaking spaces (U+00A0).
> 
> Oh, damn, despite all my efforts, Thunderbird still messed the patch up. Sigh. (gotta finally find some sane mail client)
> 

Thanks for your efforts.

For Thunderbird there's
https://wiki.openstack.org/wiki/MailingListEtiquette#Thunderbird , but
I don't think it affects you since your email wasn't format=flowed.

Usually, I send patches through one of git-send-email(1), Mutt, and
Claws.  None of them munge whitespace (some of them had to be
configured appropriately first).

> Anyway, copypasting it from the mailing list archive results in something that applies cleanly: https://www.zsh.org/mla/workers/2020/msg00190.html

If you look at the HTML source of that page you'll find it has the
non-breaking spaces still.  Firefox, at least, translates NBSP to
ordinary space upon copying, so that might explain why copy-pasting
that way worked.  The problem is that copying text this way would also
obliterate any non-breaking spaces that were added intentionally.

> > I applied the patch manually, and completion did work, but stderr was
> > spammed with two warnings:
> >
> > +__git_recent_branches:21>   local $'z=\C-@'
> > __git_recent_branches:21: command not found:
> 
> That isn't supposed to happen. This should merely assign a string contained a single null byte to variable z. Maybe you didn't clean up all those NBSP?

Yes, sounds plausible.  If the first column was a non-breaking space,
that would cause the observed error message.

> >  
> > +__git_recent_branches:22> _call_program all-descriptions 'git --no-pager for-each-ref --format='\''%(refname)%00%(subject)'\' refs/heads/master --                                                                
> > __git_recent_branches:22: unknown file attribute:
> 
> Likewise, I have no clue what it is.

That'd be the same issue.  The word «descriptions=(…)» was taken as an
ordinary positional argument followed by glob qualifiers to the « »
(NBSP) command.  The character «=» is not special when not in
a pre-command assignment; it's just an ordinary filename character.

I've applied your patch to master with a grammar tweak; it'll be
in 5.8.  Thanks again!

Daniel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 16:57 [PATCH] _git: Fix __git_recent_branches for case when commit has empty message WGH
2020-02-01  0:09 ` Daniel Shahaf
2020-02-01 23:50   ` WGH
2020-02-02  7:34     ` Daniel Shahaf

zsh-workers

Archives are clonable: git clone --mirror http://inbox.vuxu.org/zsh-workers

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git