[PATCH] Increase $COLUMNS when generating long option completions

[PATCH] Increase $COLUMNS when generating long option completions

[Marlon Richert]

[-- Attachment #1: Type: text/plain, Size: 67 bytes --]

Otherwise, option descriptions can appear cropped on wide screens.

[-- Attachment #2: 0001-Increase-COLUMNS-when-generating-long-option-complet.txt --]
[-- Type: text/plain, Size: 1047 bytes --]

From 2075b9f42cb9f0ad3cd1ac330302208048d65a25 Mon Sep 17 00:00:00 2001
From: Marlon Richert <marlonrichert@users.noreply.github.com>
Date: Sun, 1 Aug 2021 22:22:14 +0300
Subject: [PATCH] Increase $COLUMNS when generating long option completions

Otherwise, option descriptions can appear cropped on wide screens.
---
 Completion/Base/Utility/_arguments | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Completion/Base/Utility/_arguments b/Completion/Base/Utility/_arguments
index 3f1b39304..5db3926fb 100644
--- a/Completion/Base/Utility/_arguments
+++ b/Completion/Base/Utility/_arguments
@@ -95,7 +95,9 @@ if (( long )); then
     # option up to the end.
 
    tmp=()
-   _call_program $lflag options ${~words[1]} --help 2>&1 |
+
+   # Increase $COLUMNS, so --help output won't get cropped.
+   _call_program $lflag options COLUMNS=999 ${~words[1]} --help 2>&1 |
      while IFS= read -r opt; do
      if (( ${#tmp} )); then
        # Previous line had no comment.  Is the current one suitable?
-- 
2.30.1 (Apple Git-130)

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Bart Schaefer]

I'm puzzled, doesn't this imply that the value of $COLUMNS is
incorrect on entry to the function?

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Marlon Richert]

On Mon, Aug 2, 2021 at 2:07 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> I'm puzzled, doesn't this imply that the value of $COLUMNS is
> incorrect on entry to the function?

No, the problem results from _call_program being connected to a pipe:

   _call_program $lflag options ${~words[1]} --help 2>&1 |
     while IFS= read -r opt; do

When ${~words[1]} is an external command, it will then not see
$COLUMNS, unless $COLUMNS has been exported.

For example, compare the output of the following:

% pip3 --help
% _call_program tst pip3 --help
% _call_program tst pip3 --help | cat
% print -r -- "$( pip3 --help )"

The first two will produce output that is as wide as the terminal,
whereas the latter two will always produce 80 columns of output.

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Lawrence Velázquez]

On Tue, Aug 3, 2021, at 10:05 AM, Marlon Richert wrote:
> On Mon, Aug 2, 2021 at 2:07 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > I'm puzzled, doesn't this imply that the value of $COLUMNS is
> > incorrect on entry to the function?
> 
> No, the problem results from _call_program being connected to a pipe:
> 
>    _call_program $lflag options ${~words[1]} --help 2>&1 |
>      while IFS= read -r opt; do
> 
> When ${~words[1]} is an external command, it will then not see
> $COLUMNS, unless $COLUMNS has been exported.

Isn't that the case in general?  External commands *never* see
COLUMNS if it isn't in the environment.  The pipe is a red herring.

> For example, compare the output of the following:
> 
> % pip3 --help
> % _call_program tst pip3 --help
> % _call_program tst pip3 --help | cat
> % print -r -- "$( pip3 --help )"

Seems like pip is behaving differently depending on whether it's
outputting to a tty or not.

-- 
vq

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Bart Schaefer]

On Tue, Aug 3, 2021 at 6:12 PM Lawrence Velázquez <larryv@zsh.org> wrote:
>
> On Tue, Aug 3, 2021, at 10:05 AM, Marlon Richert wrote:
> >
> > No, the problem results from _call_program being connected to a pipe:
> >
> > When ${~words[1]} is an external command, it will then not see
> > $COLUMNS, unless $COLUMNS has been exported.
>
> Isn't that the case in general?  External commands *never* see
> COLUMNS if it isn't in the environment.

I was thinking the same thing.

> The pipe is a red herring.

Well, not exactly. As you said:

> Seems like pip is behaving differently depending on whether it's
> outputting to a tty or not.

Probably, if the output is a tty then when it finds no $COLUMNS, it
reads the width from the tty, and when it has neither it falls back to
80.

Still that would argue for using
... COLUMNS=$COLUMNS ${~words[1]} ...
That is, export an accurate value rather than an arbitrarily large one.

The next question would be, is _arguments the only place this is
useful?  Or would it be better  if _call_program always did so? E.g.
  local -x COLUMNS

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Marlon Richert]

On Wed, Aug 4, 2021 at 4:12 AM Lawrence Velázquez <larryv@zsh.org> wrote:
>
> On Tue, Aug 3, 2021, at 10:05 AM, Marlon Richert wrote:
> > On Mon, Aug 2, 2021 at 2:07 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
> > >
> > > I'm puzzled, doesn't this imply that the value of $COLUMNS is
> > > incorrect on entry to the function?
> >
> > No, the problem results from _call_program being connected to a pipe:
> >
> >    _call_program $lflag options ${~words[1]} --help 2>&1 |
> >      while IFS= read -r opt; do
> >
> > When ${~words[1]} is an external command, it will then not see
> > $COLUMNS, unless $COLUMNS has been exported.
>
> Isn't that the case in general?  External commands *never* see
> COLUMNS if it isn't in the environment.  The pipe is a red herring.
>
> > For example, compare the output of the following:
> >
> > % pip3 --help
> > % _call_program tst pip3 --help
> > % _call_program tst pip3 --help | cat
> > % print -r -- "$( pip3 --help )"
>
> Seems like pip is behaving differently depending on whether it's
> outputting to a tty or not.

I feel that cannot be the whole story, because if I do any of the
following, I get terminal-width output, instead of 80 columns:

% COLUMNS=$COLUMNS pip3 --help | cat
% print -r -- "$( COLUMNS=$COLUMNS pip3 --help )"
% _call_program tst COLUMNS=$COLUMNS pip3 --help | cat

And pip isn't the only command that works that way. For example:

% ls -x /
Applications Library System Users Volumes bin cores dev etc home
opt private sbin tmp usr var
% ls -x / | cat
Applications Library System Users Volumes
bin cores dev etc home
opt private sbin tmp usr
var
% COLUMNS=$COLUMNS ls -x / | cat
Applications Library System Users Volumes bin cores dev etc home
opt private sbin tmp usr var
%

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Lawrence Velázquez]

> On Aug 4, 2021, at 2:53 AM, Marlon Richert <marlon.richert@gmail.com> wrote:
> On Wed, Aug 4, 2021 at 4:12 AM Lawrence Velázquez <larryv@zsh.org> wrote:
>> 
>> On Tue, Aug 3, 2021, at 10:05 AM, Marlon Richert wrote:
>>> On Mon, Aug 2, 2021 at 2:07 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>>>> I'm puzzled, doesn't this imply that the value of $COLUMNS is
>>>> incorrect on entry to the function?
>>> No, the problem results from _call_program being connected to a pipe:
>>>   _call_program $lflag options ${~words[1]} --help 2>&1 |
>>>     while IFS= read -r opt; do
>>> When ${~words[1]} is an external command, it will then not see
>>> $COLUMNS, unless $COLUMNS has been exported.
>> 
>> Isn't that the case in general?  External commands *never* see
>> COLUMNS if it isn't in the environment.  The pipe is a red herring.
>> 
>>> For example, compare the output of the following:
>>> % pip3 --help
>>> % _call_program tst pip3 --help
>>> % _call_program tst pip3 --help | cat
>>> % print -r -- "$( pip3 --help )"
>> 
>> Seems like pip is behaving differently depending on whether it's
>> outputting to a tty or not.
> 
> I feel that cannot be the whole story, because if I do any of the
> following, I get terminal-width output, instead of 80 columns:
> 
> % COLUMNS=$COLUMNS pip3 --help | cat
> % print -r -- "$( COLUMNS=$COLUMNS pip3 --help )"

Yes, these commands are explicitly setting COLUMNS in pip’s environment. As Bart said, the tty vs. not-tty distinction probably only matters when COLUMNS is not in pip’s environment.

> % _call_program tst COLUMNS=$COLUMNS pip3 --help | cat
> 
> And pip isn't the only command that works that way. For example:
> 
> % ls -x /
> Applications Library System Users Volumes bin cores dev etc home
> opt private sbin tmp usr var
> % ls -x / | cat
> Applications Library System Users Volumes
> bin cores dev etc home
> opt private sbin tmp usr
> var
> % COLUMNS=$COLUMNS ls -x / | cat
> Applications Library System Users Volumes bin cores dev etc home
> opt private sbin tmp usr var
> %

Same thing here. You are explicitly setting COLUMNS in the environment of ls, and ls is respecting it.

-- 
vq
Sent from my iPhone

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Marlon Richert]

On Wed, Aug 4, 2021 at 10:19 AM Lawrence Velázquez <larryv@zsh.org> wrote:
>
> Yes, these commands are explicitly setting COLUMNS in pip’s environment. As Bart said, the tty vs. not-tty distinction probably only matters when COLUMNS is not in pip’s environment.
>
> Same thing here. You are explicitly setting COLUMNS in the environment of ls, and ls is respecting it.

OK. Thanks for clearing that up.

I don't think this all changes anything about the patch, though?

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Bart Schaefer]

On Wed, Aug 4, 2021 at 11:20 PM Marlon Richert <marlon.richert@gmail.com> wrote:
>
> I don't think this all changes anything about the patch, though?

On Tue, Aug 3, 2021 at 8:48 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> The next question would be, is _arguments the only place this is
> useful?  Or would it be better  if _call_program always did so? E.g.
>   local -x COLUMNS

If the answer to that is to leave it in _arguments, then I think
COLUMNS=$COLUMNS is preferable to COLUMNS=999

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Marlon Richert]

On Thu, Aug 5, 2021 at 9:24 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Wed, Aug 4, 2021 at 11:20 PM Marlon Richert <marlon.richert@gmail.com> wrote:
> >
> > I don't think this all changes anything about the patch, though?
>
> On Tue, Aug 3, 2021 at 8:48 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > The next question would be, is _arguments the only place this is
> > useful?  Or would it be better  if _call_program always did so? E.g.
> >   local -x COLUMNS

Sorry, I missed this. I don't know if it makes sense to always set
COLUMNS in _call_program, because it is not always called to produce
visual output. On the other hand, if you think it won't hurt, then
yes, that might be a better option.

> If the answer to that is to leave it in _arguments, then I think
> COLUMNS=$COLUMNS is preferable to COLUMNS=999

I made it 999, rather than $COLUMNS, because the output is cached and
terminal emulators can be resized.

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Bart Schaefer]

On Thu, Aug 5, 2021 at 11:12 AM Marlon Richert <marlon.richert@gmail.com> wrote:
>
> I don't know if it makes sense to always set
> COLUMNS in _call_program, because it is not always called to produce
> visual output. On the other hand, if you think it won't hurt, then
> yes, that might be a better option.

I can't think of a case where it would be an issue ... if the program
isn't producing output meant for the terminal, it shouldn't be
bothered by either the presence or absence of $COLUMNS.  I'm now
pondering whether your original method does either a better or a worse
(or neither) job if the called program uses a remote shell or the
like.

> I made it 999, rather than $COLUMNS, because the output is cached and
> terminal emulators can be resized.

Hrm.  That sounds like a problem with the cache validation rules, but
I suppose setting a large value of COLUMNS is a reasonable safeguard.

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Marlon Richert]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Fri, Aug 6, 2021 at 2:44 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Thu, Aug 5, 2021 at 11:12 AM Marlon Richert <marlon.richert@gmail.com> wrote:
> >
> > I don't know if it makes sense to always set
> > COLUMNS in _call_program, because it is not always called to produce
> > visual output. On the other hand, if you think it won't hurt, then
> > yes, that might be a better option.
>
> I can't think of a case where it would be an issue ... if the program
> isn't producing output meant for the terminal, it shouldn't be
> bothered by either the presence or absence of $COLUMNS.  I'm now
> pondering whether your original method does either a better or a worse
> (or neither) job if the called program uses a remote shell or the
> like.
>
> > I made it 999, rather than $COLUMNS, because the output is cached and
> > terminal emulators can be resized.
>
> Hrm.  That sounds like a problem with the cache validation rules, but
> I suppose setting a large value of COLUMNS is a reasonable safeguard.

How does this patch look to you?

[-- Attachment #2: 0001-Set-COLUMNS-in-_call_program-to-ensure-cached-comman.txt --]
[-- Type: text/plain, Size: 1423 bytes --]

From 1912a575fbdf34e4937eeea9387807351342fc37 Mon Sep 17 00:00:00 2001
From: Marlon Richert <marlonrichert@users.noreply.github.com>
Date: Sat, 7 Aug 2021 22:52:21 +0300
Subject: [PATCH] Set $COLUMNS in _call_program to ensure cached command output
 is sufficiently wide

---
 Completion/Base/Utility/_call_program | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Completion/Base/Utility/_call_program b/Completion/Base/Utility/_call_program
index 73f3ef6d2..cc7c9adf9 100644
--- a/Completion/Base/Utility/_call_program
+++ b/Completion/Base/Utility/_call_program
@@ -1,6 +1,7 @@
 #autoload +X
 
-local curcontext="${curcontext}" tmp err_fd=-1 clocale='_comp_locale;'
+local cmd clocale='_comp_locale;' cols='COLUMNS=999' curcontext="${curcontext}"
+local -i err_fd=-1
 local -a prefix
 
 if [[ "$1" = -p ]]; then
@@ -22,14 +23,14 @@ fi
 
 {	# Begin "always" block
 
-if zstyle -s ":completion:${curcontext}:${1}" command tmp; then
-  if [[ "$tmp" = -* ]]; then
-    eval $clocale "$tmp[2,-1]" "$argv[2,-1]"
+if zstyle -s ":completion:${curcontext}:${1}" command cmd; then
+  if [[ "$cmd" = -* ]]; then
+    eval $clocale $cols "$cmd[2,-1]" "$argv[2,-1]"
   else
-    eval $clocale $prefix "$tmp"
+    eval $clocale $cols $prefix "$cmd"
   fi
 else
-  eval $clocale $prefix "$argv[2,-1]"
+  eval $clocale $cols $prefix "$argv[2,-1]"
 fi 2>&$err_fd
 
 } always {
-- 
2.30.1 (Apple Git-130)

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Bart Schaefer]

On Sat, Aug 7, 2021 at 12:56 PM Marlon Richert <marlon.richert@gmail.com> wrote:
>
> How does this patch look to you?

Frankly, it looks unnecessarily complicated.  What's the rationale for
using COLUMNS=999 as a command prefix in every "eval", rather than
just declaring once
  local -x COLUMNS=999
at the start of the function??  Why rearrange the other declarations?

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Marlon Richert]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Sun, Aug 8, 2021 at 1:41 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Sat, Aug 7, 2021 at 12:56 PM Marlon Richert <marlon.richert@gmail.com> wrote:
> >
> > How does this patch look to you?
>
> Frankly, it looks unnecessarily complicated.  What's the rationale for
> using COLUMNS=999 as a command prefix in every "eval", rather than
> just declaring once
>   local -x COLUMNS=999
> at the start of the function??  Why rearrange the other declarations?

TIL you can export parameters locally. Here's a new patch.

[-- Attachment #2: 0001-Set-COLUMNS-in-_call_program-to-ensure-cached-comman.txt --]
[-- Type: text/plain, Size: 728 bytes --]

From 0f7ed76a199710b1323c12cc7e7e17fb10f5f748 Mon Sep 17 00:00:00 2001
From: Marlon Richert <marlon.richert@gmail.com>
Date: Tue, 10 Aug 2021 22:02:11 +0300
Subject: [PATCH] Set $COLUMNS in _call_program to ensure cached command output
 is sufficiently wide

---
 Completion/Base/Utility/_call_program | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Completion/Base/Utility/_call_program b/Completion/Base/Utility/_call_program
index 73f3ef6d2..55712b04b 100644
--- a/Completion/Base/Utility/_call_program
+++ b/Completion/Base/Utility/_call_program
@@ -1,5 +1,6 @@
 #autoload +X
 
+local -xi COLUMNS=999
 local curcontext="${curcontext}" tmp err_fd=-1 clocale='_comp_locale;'
 local -a prefix
 
-- 
2.30.1 (Apple Git-130)

Re: [PATCH] Increase $COLUMNS when generating long option completions

[Marlon Richert]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

On Tue, Aug 10, 2021 at 10:04 PM Marlon Richert
<marlon.richert@gmail.com> wrote:
>
> On Sun, Aug 8, 2021 at 1:41 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > On Sat, Aug 7, 2021 at 12:56 PM Marlon Richert <marlon.richert@gmail.com> wrote:
> > >
> > > How does this patch look to you?
> >
> > Frankly, it looks unnecessarily complicated.  What's the rationale for
> > using COLUMNS=999 as a command prefix in every "eval", rather than
> > just declaring once
> >   local -x COLUMNS=999
> > at the start of the function??  Why rearrange the other declarations?
>
> TIL you can export parameters locally. Here's a new patch.

And since we're on the topic, this patch adds `-x` to `local` completion.

[-- Attachment #2: 0001-Complete-local-x.txt --]
[-- Type: text/plain, Size: 779 bytes --]

From 8742ba63ebccbba1c367663094e0f1c10dd2107b Mon Sep 17 00:00:00 2001
From: Marlon Richert <marlon.richert@gmail.com>
Date: Tue, 10 Aug 2021 22:11:28 +0300
Subject: [PATCH] Complete `local -x`

---
 Completion/Zsh/Command/_typeset | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Completion/Zsh/Command/_typeset b/Completion/Zsh/Command/_typeset
index 3f7de9706..5f7fb24fa 100644
--- a/Completion/Zsh/Command/_typeset
+++ b/Completion/Zsh/Command/_typeset
@@ -69,7 +69,7 @@ case ${service} in
     allargs[i]='-i+[specify arithmetic base for output]:: :_guard "[0-9]#" base' \
   ;;
   readonly) use="${use/r/}" ;;
-  local) use="${use/[fkz]/}" ;&
+  local) use="${use/[fgkz]/}" ;;
   export) use="${${use//[gkz]/}/x/}" ;;
 esac
 
-- 
2.30.1 (Apple Git-130)