Feature Patch: Use completion to view parameter values

Feature Patch: Use completion to view parameter values

[Marlon Richert]

diff --git a/Completion/Zsh/Type/_parameters b/Completion/Zsh/Type/_parameters
index 207e5cf78..84f078126 100644
--- a/Completion/Zsh/Type/_parameters
+++ b/Completion/Zsh/Type/_parameters
@@ -6,13 +6,18 @@
 # If you specify a -g option with a pattern, the pattern will be used to
 # restrict the type of parameters matched.

-local expl pattern fakes faked tmp i pfilt
+local MATCH MBEGIN MEND \
+  disp dopt expl fakes faked i matches pattern pfilt sep tmp

 if compset -P '*:'; then
   _history_modifiers p
   return
 fi

+_tags parameters
+( _tags && _requested parameters ) ||
+  return
+
 pattern=(-g \*)
 zparseopts -D -K -E g:=pattern

@@ -32,8 +37,18 @@ zstyle -t ":completion:${curcontext}:parameters"
prefix-needed && \
  [[ $PREFIX != [_.]* ]] && \
  pfilt='[^_.]'

-_wanted parameters expl parameter \
-    compadd "$@" -Q - \
-        "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
-        "$fakes[@]" \
-        "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+_description parameters expl parameter
+compadd "$expl[@]" -O matches - \
+  "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
+  "$fakes[@]" \
+  "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+
+if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
+  zstyle -s ":completion:${curcontext}:parameters" list-separator sep ||
+    sep=--
+  zformat -a disp " $sep " ${matches[@]:/(#m)*/"${MATCH}:${(Pkv@q+)MATCH}"}
+  disp=( "${disp[@]:/(#m)*/$MATCH[1,COLUMNS]}" )
+  dopt=( -d disp )
+fi
+
+compadd "$expl[@]" $dopt -Q -a matches
diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 571f3cf16..e1bbab92e 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -222,7 +222,7 @@ F:regression test workers/31611
   comptesteval "zstyle ':completion:*:tst:*' ignored-patterns 2"
   comptest $'tst 1,\t'
   comptesteval "zstyle -d ':completion:*:tst:*' ignored-patterns"
-0:-F doesn't break _sequence
+0:-F doesn’t break _sequence
 >line: {tst 1,}{}
 >DESCRIPTION:{desc}
 >NO:{2}
@@ -237,6 +237,23 @@ F:regression test workers/31611
 >FI:{file1}
 >FI:{file2}

+  comptesteval "bar=({$'\\0'..$'\\C-?'}); baz=\$bar"
+  comptesteval 'zstyle ":completion:*:parameters" extra-verbose yes'
+  comptest $': $ba\t'
+0:extra-verbose shows parameter values
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar -- '^@' '^A' '^B' '^C' '^D' '^E' '^F' '^G' '^H' '\t' '\n'
'^K' '^L' '^M' '^N}
+>NO:{baz -- '^@ ^A ^B ^C ^D ^E ^F ^G ^H \t \n ^K ^L ^M ^N ^O ^P ^Q ^R
^S ^T ^U ^V ^W }
+
+  comptesteval 'zstyle -d ":completion:*:parameters" extra-verbose'
+  comptest $': $ba\t'
+0:parameter values not shown without extra-verbose
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar}
+>NO:{baz}
+
   comptesteval '_tst() { local disp=( {a..z} ); compadd -ld disp
$disp[@]; comppostfuncs=( _pst ) }'
   comptesteval '_pst() { local disp=(
"<INSERT>$compstate[insert]</INSERT>" ); compadd -Qld disp $disp }'
   comptesteval "zstyle ':completion:*' menu select=long-list"

Re: Feature Patch: Use completion to view parameter values

[Daniel Shahaf]

Marlon Richert wrote on Sun, Mar 28, 2021 at 23:53:29 +0300:
> +++ b/Completion/Zsh/Type/_parameters
> @@ -32,8 +37,18 @@ zstyle -t ":completion:${curcontext}:parameters"
> +if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
> +  zstyle -s ":completion:${curcontext}:parameters" list-separator sep ||
> +    sep=--
> +  zformat -a disp " $sep " ${matches[@]:/(#m)*/"${MATCH}:${(Pkv@q+)MATCH}"}

What about parameters implemented by modules?  The getters of those may
run arbitrary code, and may have side effects: e.g., imagine an
$AUTOINCREMENT parameter that returns an incremented-by-one value every
time it's evaluated:

    % repeat 3 echo $AUTOINCREMENT
    1
    2
    3
    % echo $AUTOINCR<TAB><Enter>

Wouldn't that print «5»?

Cheers,

Daniel


> +  disp=( "${disp[@]:/(#m)*/$MATCH[1,COLUMNS]}" )
> +  dopt=( -d disp )
> +fi

Re: Feature Patch: Use completion to view parameter values

[Mikael Magnusson]

On 3/28/21, Marlon Richert <marlon.richert@gmail.com> wrote:
> diff --git a/Completion/Zsh/Type/_parameters

I may only be speaking for myself here, but please always include a
commit message in patches. Trying to reverse engineer your intent from
the patch body and then reviewing it to see how the patch differs from
that intent is somewhat impossible (and "Use completion to view
parameter values" is pretty vague).

I'm assuming this is some sort of RFC since you didn't include any
documentation patch, so then it is even more helpful to have some
description of what you're trying to do.

-- 
Mikael Magnusson

Re: Feature Patch: Use completion to view parameter values

[Marlon Richert]

On Mon, Mar 29, 2021 at 10:39 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> What about parameters implemented by modules?  The getters of those may
> run arbitrary code, and may have side effects: e.g., imagine an
> $AUTOINCREMENT parameter that returns an incremented-by-one value every
> time it's evaluated:

Is there a way to identify these somehow, so I could filter them out?
How about if I never auto-evaluate "special" parameters? So, like
this:

diff --git a/Completion/Zsh/Type/_parameters b/Completion/Zsh/Type/_parameters
index 207e5cf78..4cc8171b0 100644
--- a/Completion/Zsh/Type/_parameters
+++ b/Completion/Zsh/Type/_parameters
@@ -6,13 +6,18 @@
 # If you specify a -g option with a pattern, the pattern will be used to
 # restrict the type of parameters matched.

-local expl pattern fakes faked tmp i pfilt
+local MATCH MBEGIN MEND \
+  disp dopt expl fakes faked i matches pattern pfilt sep tmp

 if compset -P '*:'; then
   _history_modifiers p
   return
 fi

+_tags parameters
+( _tags && _requested parameters ) ||
+  return
+
 pattern=(-g \*)
 zparseopts -D -K -E g:=pattern

@@ -32,8 +37,19 @@ zstyle -t ":completion:${curcontext}:parameters"
prefix-needed && \
  [[ $PREFIX != [_.]* ]] && \
  pfilt='[^_.]'

-_wanted parameters expl parameter \
-    compadd "$@" -Q - \
-        "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
-        "$fakes[@]" \
-        "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+_description parameters expl parameter
+compadd "$expl[@]" -O matches - \
+  "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
+  "$fakes[@]" \
+  "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+
+if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
+  zstyle -s ":completion:${curcontext}:parameters" list-separator sep ||
+    sep=--
+  zformat -a disp " $sep " \
+    ${matches[@]:/(#m)*/"${MATCH}:${${(t)${(P)MATCH}:#*special*}:+${(Pkv@q+)MATCH}}"}
+  disp=( "${disp[@]:/(#m)*/$MATCH[1,COLUMNS]}" )
+  dopt=( -d disp )
+fi
+
+compadd "$expl[@]" $dopt -Q -a matches
diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 571f3cf16..e5901b8e5 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -222,7 +222,7 @@ F:regression test workers/31611
   comptesteval "zstyle ':completion:*:tst:*' ignored-patterns 2"
   comptest $'tst 1,\t'
   comptesteval "zstyle -d ':completion:*:tst:*' ignored-patterns"
-0:-F doesn't break _sequence
+0:-F doesn’t break _sequence
 >line: {tst 1,}{}
 >DESCRIPTION:{desc}
 >NO:{2}
@@ -237,6 +237,28 @@ F:regression test workers/31611
 >FI:{file1}
 >FI:{file2}

+  comptesteval "bar=({$'\\0'..$'\\C-?'}); baz=\$bar"
+  comptesteval 'zstyle ":completion:*:parameters" extra-verbose yes'
+  comptest $': $ba\t'
+0:extra-verbose shows parameter values
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar -- '^@' '^A' '^B' '^C' '^D' '^E' '^F' '^G' '^H' '\t' '\n'
'^K' '^L' '^M' '^N}
+>NO:{baz -- '^@ ^A ^B ^C ^D ^E ^F ^G ^H \t \n ^K ^L ^M ^N ^O ^P ^Q ^R
^S ^T ^U ^V ^W }
+
+  comptest $': $path\C-D'
+0:extra-verbose does not show special parameter values
+>DESCRIPTION:{parameter}
+>NO:{path}
+
+  comptesteval 'zstyle -d ":completion:*:parameters" extra-verbose'
+  comptest $': $ba\t'
+0:parameter values not shown without extra-verbose
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar}
+>NO:{baz}
+
   comptesteval '_tst() { local disp=( {a..z} ); compadd -ld disp
$disp[@]; comppostfuncs=( _pst ) }'
   comptesteval '_pst() { local disp=(
"<INSERT>$compstate[insert]</INSERT>" ); compadd -Qld disp $disp }'
   comptesteval "zstyle ':completion:*' menu select=long-list"

Re: Feature Patch: Use completion to view parameter values

[Marlon Richert]

On Mon, Mar 29, 2021 at 2:48 PM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On 3/28/21, Marlon Richert <marlon.richert@gmail.com> wrote:
> > diff --git a/Completion/Zsh/Type/_parameters
>
> I may only be speaking for myself here, but please always include a
> commit message in patches. Trying to reverse engineer your intent from
> the patch body and then reviewing it to see how the patch differs from
> that intent is somewhat impossible (and "Use completion to view
> parameter values" is pretty vague).
>
> I'm assuming this is some sort of RFC since you didn't include any
> documentation patch, so then it is even more helpful to have some
> description of what you're trying to do.

Good point I hadn't thought of that. Something like this?

commit 74c79f74718df7babc1dcf82bad307de6249f7bb
Author: Marlon Richert <marlon.richert@gmail.com>
Date:   Mon Mar 29 15:04:49 2021 +0300

    Let `zstyle extra-verbose` show parameter values

    When completing parameters and
    `zstyle -t ":completion:${curcontext}:parameters" extra-verbose`,
    display values of non-special parameters as descriptions.

diff --git a/Completion/Zsh/Type/_parameters b/Completion/Zsh/Type/_parameters
index 207e5cf78..4cc8171b0 100644
--- a/Completion/Zsh/Type/_parameters
+++ b/Completion/Zsh/Type/_parameters
@@ -6,13 +6,18 @@
 # If you specify a -g option with a pattern, the pattern will be used to
 # restrict the type of parameters matched.

-local expl pattern fakes faked tmp i pfilt
+local MATCH MBEGIN MEND \
+  disp dopt expl fakes faked i matches pattern pfilt sep tmp

 if compset -P '*:'; then
   _history_modifiers p
   return
 fi

+_tags parameters
+( _tags && _requested parameters ) ||
+  return
+
 pattern=(-g \*)
 zparseopts -D -K -E g:=pattern

@@ -32,8 +37,19 @@ zstyle -t ":completion:${curcontext}:parameters"
prefix-needed && \
  [[ $PREFIX != [_.]* ]] && \
  pfilt='[^_.]'

-_wanted parameters expl parameter \
-    compadd "$@" -Q - \
-        "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
-        "$fakes[@]" \
-        "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+_description parameters expl parameter
+compadd "$expl[@]" -O matches - \
+  "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
+  "$fakes[@]" \
+  "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+
+if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
+  zstyle -s ":completion:${curcontext}:parameters" list-separator sep ||
+    sep=--
+  zformat -a disp " $sep " \
+    ${matches[@]:/(#m)*/"${MATCH}:${${(t)${(P)MATCH}:#*special*}:+${(Pkv@q+)MATCH}}"}
+  disp=( "${disp[@]:/(#m)*/$MATCH[1,COLUMNS]}" )
+  dopt=( -d disp )
+fi
+
+compadd "$expl[@]" $dopt -Q -a matches
diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 571f3cf16..e5901b8e5 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -222,7 +222,7 @@ F:regression test workers/31611
   comptesteval "zstyle ':completion:*:tst:*' ignored-patterns 2"
   comptest $'tst 1,\t'
   comptesteval "zstyle -d ':completion:*:tst:*' ignored-patterns"
-0:-F doesn't break _sequence
+0:-F doesn’t break _sequence
 >line: {tst 1,}{}
 >DESCRIPTION:{desc}
 >NO:{2}
@@ -237,6 +237,28 @@ F:regression test workers/31611
 >FI:{file1}
 >FI:{file2}

+  comptesteval "bar=({$'\\0'..$'\\C-?'}); baz=\$bar"
+  comptesteval 'zstyle ":completion:*:parameters" extra-verbose yes'
+  comptest $': $ba\t'
+0:extra-verbose shows parameter values
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar -- '^@' '^A' '^B' '^C' '^D' '^E' '^F' '^G' '^H' '\t' '\n'
'^K' '^L' '^M' '^N}
+>NO:{baz -- '^@ ^A ^B ^C ^D ^E ^F ^G ^H \t \n ^K ^L ^M ^N ^O ^P ^Q ^R
^S ^T ^U ^V ^W }
+
+  comptest $': $path\C-D'
+0:extra-verbose does not show special parameter values
+>DESCRIPTION:{parameter}
+>NO:{path}
+
+  comptesteval 'zstyle -d ":completion:*:parameters" extra-verbose'
+  comptest $': $ba\t'
+0:parameter values not shown without extra-verbose
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar}
+>NO:{baz}
+
   comptesteval '_tst() { local disp=( {a..z} ); compadd -ld disp
$disp[@]; comppostfuncs=( _pst ) }'
   comptesteval '_pst() { local disp=(
"<INSERT>$compstate[insert]</INSERT>" ); compadd -Qld disp $disp }'
   comptesteval "zstyle ':completion:*' menu select=long-list"

Re: Feature Patch: Use completion to view parameter values

[Marlon Richert]

On Mon, Mar 29, 2021 at 3:06 PM Marlon Richert <marlon.richert@gmail.com> wrote:
>
> On Mon, Mar 29, 2021 at 2:48 PM Mikael Magnusson <mikachu@gmail.com> wrote:
> >
> > On 3/28/21, Marlon Richert <marlon.richert@gmail.com> wrote:
> > > diff --git a/Completion/Zsh/Type/_parameters
> >
> > I'm assuming this is some sort of RFC since you didn't include any
> > documentation patch, so then it is even more helpful to have some
> > description of what you're trying to do.

What exactly would need to be patched in the documentation?

Re: Feature Patch: Use completion to view parameter values

[Daniel Shahaf]

Marlon Richert wrote on Mon, Mar 29, 2021 at 14:55:17 +0300:
> On Mon, Mar 29, 2021 at 10:39 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > What about parameters implemented by modules?  The getters of those may
> > run arbitrary code, and may have side effects: e.g., imagine an
> > $AUTOINCREMENT parameter that returns an incremented-by-one value every
> > time it's evaluated:
> 
> Is there a way to identify these somehow, so I could filter them out?
> How about if I never auto-evaluate "special" parameters? So, like
> this:

I think that's right.  -workers@: Is it possible for a non-PM_SPECIAL
parameter have a custom getfn?

Re: Feature Patch: Use completion to view parameter values

[Bart Schaefer]

On Mon, Mar 29, 2021 at 10:11 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> I think that's right.  -workers@: Is it possible for a non-PM_SPECIAL
> parameter have a custom getfn?

Only with the zsh/param/private module, I think, and in that case the
getfn is just a wrapper around the default and doesn't add any
side-effects.

Re: Feature Patch: Use completion to view parameter values

[Daniel Shahaf]

Bart Schaefer wrote on Mon, Mar 29, 2021 at 10:20:08 -0700:
> On Mon, Mar 29, 2021 at 10:11 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > I think that's right.  -workers@: Is it possible for a non-PM_SPECIAL
> > parameter have a custom getfn?
> 
> Only with the zsh/param/private module, I think, and in that case the
> getfn is just a wrapper around the default and doesn't add any
> side-effects.

Thanks, Bart.

And as to $AUTOINCREMENT, this isn't the first time I mentioned it as
a hypothetical, so I'm going to go ahead and post it here.  I suspect
people from the future will use this for something-or-other.

Works as you'd expect:
.
    % echo $AUTOINCREMENT $AUTOINCREMENT
    0 1
    % 

And in Marlon's patch with the ${(t)…*special*} exclusion bypassed:
.
    % zstyle \* extra-verbose yes
    % AUTOFOO=42
    % echo $AUTO<TAB><TAB>
    AUTOFOO       -- 42  AUTOINCREMENT -- 2
    AUTOFOO       -- 42  AUTOINCREMENT -- 4

Yes, it does actually increment the variable twice, probably because the
_parameters patch uses both ${(t)${(P)}} and then ${(P)}, and the former
does an increment too:
.
    % echo $AUTOINCREMENT ${(tP)AUTOINCREMENT} $AUTOINCREMENT 
    0 array-special 2
    %

I'm not proposing to commit $AUTOINCREMENT.

Cheers,

Daniel

P.S.  Another similar example is Perl's magic flip-flop variable:
«perl -E 'say --$| for (1..10)'»


diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index ef9148d7b..179ac068e 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -2136,6 +2136,24 @@ scanpmusergroups(UNUSED(HashTable ht), ScanFunc func, int flags)
 }
 
 
+/* Functions for the AUTOINCREMENT special parameter. */
+
+static zlong autoincrement = 0;
+
+static zlong
+autoincrementgetfn(UNUSED(Param pm))
+{
+    return autoincrement++;
+}
+
+static void
+autoincrementsetfn(UNUSED(Param pm), zlong value)
+{
+    autoincrement = value;
+}
+
+
+
 /* Table for defined parameters. */
 
 struct pardef {
@@ -2192,8 +2210,13 @@ static const struct gsu_array dirs_gsu =
 static const struct gsu_array historywords_gsu =
 { histwgetfn, arrsetfn, stdunsetfn };
 
+static const struct gsu_integer autoincrement_gsu =
+{ autoincrementgetfn, autoincrementsetfn, stdunsetfn };
+
 /* Make sure to update autofeatures in parameter.mdd if necessary */
 static struct paramdef partab[] = {
+    SPECIALPMDEF("AUTOINCREMENT", PM_SPECIAL | PM_INTEGER,
+	    &autoincrement_gsu, NULL, NULL),
     SPECIALPMDEF("aliases", 0,
 	    &pmraliases_gsu, getpmralias, scanpmraliases),
     SPECIALPMDEF("builtins", PM_READONLY_SPECIAL, NULL, getpmbuiltin, scanpmbuiltins),
diff --git a/Src/Modules/parameter.mdd b/Src/Modules/parameter.mdd
index f71c17a72..b8fb93d54 100644
--- a/Src/Modules/parameter.mdd
+++ b/Src/Modules/parameter.mdd
@@ -2,6 +2,6 @@ name=zsh/parameter
 link=either
 load=yes
 
-autofeatures="p:parameters p:commands p:functions p:dis_functions p:functions_source p:dis_functions_source p:funcfiletrace p:funcsourcetrace p:funcstack p:functrace p:builtins p:dis_builtins p:reswords p:dis_reswords p:patchars p:dis_patchars p:options p:modules p:dirstack p:history p:historywords p:jobtexts p:jobdirs p:jobstates p:nameddirs p:userdirs p:usergroups p:aliases p:dis_aliases p:galiases p:dis_galiases p:saliases p:dis_saliases"
+autofeatures="p:parameters p:commands p:functions p:dis_functions p:functions_source p:dis_functions_source p:funcfiletrace p:funcsourcetrace p:funcstack p:functrace p:builtins p:dis_builtins p:reswords p:dis_reswords p:patchars p:dis_patchars p:options p:modules p:dirstack p:history p:historywords p:jobtexts p:jobdirs p:jobstates p:nameddirs p:userdirs p:usergroups p:aliases p:dis_aliases p:galiases p:dis_galiases p:saliases p:dis_saliases p:AUTOINCREMENT"
 
 objects="parameter.o"

Re: Feature Patch: Use completion to view parameter values

[Marlon Richert]

On Mon, Mar 29, 2021 at 9:14 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> And in Marlon's patch with the ${(t)…*special*} exclusion bypassed:
> .
>     % zstyle \* extra-verbose yes
>     % AUTOFOO=42
>     % echo $AUTO<TAB><TAB>
>     AUTOFOO       -- 42  AUTOINCREMENT -- 2
>     AUTOFOO       -- 42  AUTOINCREMENT -- 4
>
> Yes, it does actually increment the variable twice, probably because the
> _parameters patch uses both ${(t)${(P)}} and then ${(P)}, and the former
> does an increment too:
> .
>     % echo $AUTOINCREMENT ${(tP)AUTOINCREMENT} $AUTOINCREMENT
>     0 array-special 2
>     %

Hm, that made me realize that using ${(tP)…} is wholly unnecessary,
since _parameter already uses $parameters anyway. So, here's a new
patch that uses $parameters instead of ${(tP)…}:

commit 5ef8fcaabe243c8c4e04de92e1f971543470ce87
Author: Marlon Richert <marlon.richert@gmail.com>
Date:   Mon Mar 29 22:56:34 2021 +0300

    Let `zstyle extra-verbose` show parameter values

    When completing parameters and
    `zstyle -t ":completion:${curcontext}:parameters" extra-verbose`,
    display values of non-special parameters as descriptions.

diff --git a/Completion/Zsh/Type/_parameters b/Completion/Zsh/Type/_parameters
index 207e5cf78..b32e049f7 100644
--- a/Completion/Zsh/Type/_parameters
+++ b/Completion/Zsh/Type/_parameters
@@ -6,13 +6,18 @@
 # If you specify a -g option with a pattern, the pattern will be used to
 # restrict the type of parameters matched.

-local expl pattern fakes faked tmp i pfilt
+local MATCH MBEGIN MEND \
+  disp dopt expl fakes faked i matches pattern pfilt sep tmp

 if compset -P '*:'; then
   _history_modifiers p
   return
 fi

+_tags parameters
+( _tags && _requested parameters ) ||
+  return
+
 pattern=(-g \*)
 zparseopts -D -K -E g:=pattern

@@ -32,8 +37,19 @@ zstyle -t ":completion:${curcontext}:parameters"
prefix-needed && \
  [[ $PREFIX != [_.]* ]] && \
  pfilt='[^_.]'

-_wanted parameters expl parameter \
-    compadd "$@" -Q - \
-        "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
-        "$fakes[@]" \
-        "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+_description parameters expl parameter
+compadd "$expl[@]" -O matches - \
+  "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
+  "$fakes[@]" \
+  "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+
+if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
+  zstyle -s ":completion:${curcontext}:parameters" list-separator sep ||
+    sep=--
+  zformat -a disp " $sep " \
+    ${matches[@]:/(#m)*/"${MATCH}:${${parameters[$MATCH]:#*special*}:+${(Pkv@q+)MATCH}}"}
+  disp=( "${disp[@]:/(#m)*/$MATCH[1,COLUMNS]}" )
+  dopt=( -d disp )
+fi
+
+compadd "$expl[@]" $dopt -Q -a matches
diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 571f3cf16..e5901b8e5 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -222,7 +222,7 @@ F:regression test workers/31611
   comptesteval "zstyle ':completion:*:tst:*' ignored-patterns 2"
   comptest $'tst 1,\t'
   comptesteval "zstyle -d ':completion:*:tst:*' ignored-patterns"
-0:-F doesn't break _sequence
+0:-F doesn’t break _sequence
 >line: {tst 1,}{}
 >DESCRIPTION:{desc}
 >NO:{2}
@@ -237,6 +237,28 @@ F:regression test workers/31611
 >FI:{file1}
 >FI:{file2}

+  comptesteval "bar=({$'\\0'..$'\\C-?'}); baz=\$bar"
+  comptesteval 'zstyle ":completion:*:parameters" extra-verbose yes'
+  comptest $': $ba\t'
+0:extra-verbose shows parameter values
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar -- '^@' '^A' '^B' '^C' '^D' '^E' '^F' '^G' '^H' '\t' '\n'
'^K' '^L' '^M' '^N}
+>NO:{baz -- '^@ ^A ^B ^C ^D ^E ^F ^G ^H \t \n ^K ^L ^M ^N ^O ^P ^Q ^R
^S ^T ^U ^V ^W }
+
+  comptest $': $path\C-D'
+0:extra-verbose does not show special parameter values
+>DESCRIPTION:{parameter}
+>NO:{path}
+
+  comptesteval 'zstyle -d ":completion:*:parameters" extra-verbose'
+  comptest $': $ba\t'
+0:parameter values not shown without extra-verbose
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar}
+>NO:{baz}
+
   comptesteval '_tst() { local disp=( {a..z} ); compadd -ld disp
$disp[@]; comppostfuncs=( _pst ) }'
   comptesteval '_pst() { local disp=(
"<INSERT>$compstate[insert]</INSERT>" ); compadd -Qld disp $disp }'
   comptesteval "zstyle ':completion:*' menu select=long-list"

Re: Feature Patch: Use completion to view parameter values

[Daniel Shahaf]

Marlon Richert wrote on Mon, 29 Mar 2021 20:00 +00:00:
> On Mon, Mar 29, 2021 at 9:14 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > And in Marlon's patch with the ${(t)…*special*} exclusion bypassed:
> > .
> >     % zstyle \* extra-verbose yes
> >     % AUTOFOO=42
> >     % echo $AUTO<TAB><TAB>
> >     AUTOFOO       -- 42  AUTOINCREMENT -- 2
> >     AUTOFOO       -- 42  AUTOINCREMENT -- 4
> >
> > Yes, it does actually increment the variable twice, probably because the
> > _parameters patch uses both ${(t)${(P)}} and then ${(P)}, and the former
> > does an increment too:
> > .
> >     % echo $AUTOINCREMENT ${(tP)AUTOINCREMENT} $AUTOINCREMENT
> >     0 array-special 2
> >     %
> 
> Hm, that made me realize that using ${(tP)…} is wholly unnecessary,
> since _parameter already uses $parameters anyway. So, here's a new
> patch that uses $parameters instead of ${(tP)…}:
> 

Yes, using $parameters[AUTOINCREMENT] doesn't increment it.

> commit 5ef8fcaabe243c8c4e04de92e1f971543470ce87
> Author: Marlon Richert <marlon.richert@gmail.com>
> Date:   Mon Mar 29 22:56:34 2021 +0300
> 

It's better to post git-format-patch(1) output than git-show(1) output
because the former can be fed to git-am(1) but the latter can't.
(Unless that has changed since I last look?  It's rather like today's xkcd…)

Also, your previous patch was hard wrapped, so it couldn't easily be
applied.  It's best to send patches as an attachment named *.txt (unless
you look up your MUA-specific way of saying "Don't munge whitespace in
the body in any way").

Cheers,

Daniel

>     Let `zstyle extra-verbose` show parameter values
> 
>     When completing parameters and
>     `zstyle -t ":completion:${curcontext}:parameters" extra-verbose`,
>     display values of non-special parameters as descriptions.
> 
> diff --git a/Completion/Zsh/Type/_parameters b/Completion/Zsh/Type/_parameters
> index 207e5cf78..b32e049f7 100644
> --- a/Completion/Zsh/Type/_parameters
> +++ b/Completion/Zsh/Type/_parameters
> @@ -6,13 +6,18 @@
>  # If you specify a -g option with a pattern, the pattern will be used to
>  # restrict the type of parameters matched.
> 
> -local expl pattern fakes faked tmp i pfilt
> +local MATCH MBEGIN MEND \
> +  disp dopt expl fakes faked i matches pattern pfilt sep tmp
> 
>  if compset -P '*:'; then
>    _history_modifiers p
>    return
>  fi
> 
> +_tags parameters
> +( _tags && _requested parameters ) ||
> +  return
> +
>  pattern=(-g \*)
>  zparseopts -D -K -E g:=pattern
> 
> @@ -32,8 +37,19 @@ zstyle -t ":completion:${curcontext}:parameters"
> prefix-needed && \
>   [[ $PREFIX != [_.]* ]] && \
>   pfilt='[^_.]'
> 
> -_wanted parameters expl parameter \
> -    compadd "$@" -Q - \
> -        
> "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
> -        "$fakes[@]" \
> -        "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
> +_description parameters expl parameter
> +compadd "$expl[@]" -O matches - \
> +  "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
> +  "$fakes[@]" \
> +  "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
> +
> +if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
> +  zstyle -s ":completion:${curcontext}:parameters" list-separator sep 
> ||
> +    sep=--
> +  zformat -a disp " $sep " \
> +    
> ${matches[@]:/(#m)*/"${MATCH}:${${parameters[$MATCH]:#*special*}:+${(Pkv@q+)MATCH}}"}
> +  disp=( "${disp[@]:/(#m)*/$MATCH[1,COLUMNS]}" )
> +  dopt=( -d disp )
> +fi
> +
> +compadd "$expl[@]" $dopt -Q -a matches
> diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
> index 571f3cf16..e5901b8e5 100644
> --- a/Test/Y01completion.ztst
> +++ b/Test/Y01completion.ztst
> @@ -222,7 +222,7 @@ F:regression test workers/31611
>    comptesteval "zstyle ':completion:*:tst:*' ignored-patterns 2"
>    comptest $'tst 1,\t'
>    comptesteval "zstyle -d ':completion:*:tst:*' ignored-patterns"
> -0:-F doesn't break _sequence
> +0:-F doesn’t break _sequence
>  >line: {tst 1,}{}
>  >DESCRIPTION:{desc}
>  >NO:{2}
> @@ -237,6 +237,28 @@ F:regression test workers/31611
>  >FI:{file1}
>  >FI:{file2}
> 
> +  comptesteval "bar=({$'\\0'..$'\\C-?'}); baz=\$bar"
> +  comptesteval 'zstyle ":completion:*:parameters" extra-verbose yes'
> +  comptest $': $ba\t'
> +0:extra-verbose shows parameter values
> +>line: {: $ba}{}
> +>DESCRIPTION:{parameter}
> +>NO:{bar -- '^@' '^A' '^B' '^C' '^D' '^E' '^F' '^G' '^H' '\t' '\n'
> '^K' '^L' '^M' '^N}
> +>NO:{baz -- '^@ ^A ^B ^C ^D ^E ^F ^G ^H \t \n ^K ^L ^M ^N ^O ^P ^Q ^R
> ^S ^T ^U ^V ^W }
> +
> +  comptest $': $path\C-D'
> +0:extra-verbose does not show special parameter values
> +>DESCRIPTION:{parameter}
> +>NO:{path}
> +
> +  comptesteval 'zstyle -d ":completion:*:parameters" extra-verbose'
> +  comptest $': $ba\t'
> +0:parameter values not shown without extra-verbose
> +>line: {: $ba}{}
> +>DESCRIPTION:{parameter}
> +>NO:{bar}
> +>NO:{baz}
> +
>    comptesteval '_tst() { local disp=( {a..z} ); compadd -ld disp
> $disp[@]; comppostfuncs=( _pst ) }'
>    comptesteval '_pst() { local disp=(
> "<INSERT>$compstate[insert]</INSERT>" ); compadd -Qld disp $disp }'
>    comptesteval "zstyle ':completion:*' menu select=long-list"
>

Re: Feature Patch: Use completion to view parameter values

[Peter Stephenson]

> On 29 March 2021 at 18:20 Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> 
> On Mon, Mar 29, 2021 at 10:11 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > I think that's right.  -workers@: Is it possible for a non-PM_SPECIAL
> > parameter have a custom getfn?
> 
> Only with the zsh/param/private module, I think, and in that case the
> getfn is just a wrapper around the default and doesn't add any
> side-effects.

I think it depends a bit what you mean by "custom" --- getfn's can be
as generic as you like, but it's probably safe to say that a function
that isn't getting and setting straight data, whatever its source,
ought to be marked SPECIAL or isn't really playing by the rules.

pws

Re: Feature Patch: Use completion to view parameter values

[Marlon Richert]

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

On Mon, Mar 29, 2021 at 11:06 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> It's better to post git-format-patch(1) output than git-show(1) output
> because the former can be fed to git-am(1) but the latter can't.
> (Unless that has changed since I last look?  It's rather like today's xkcd…)
>
> Also, your previous patch was hard wrapped, so it couldn't easily be
> applied.  It's best to send patches as an attachment named *.txt (unless
> you look up your MUA-specific way of saying "Don't munge whitespace in
> the body in any way").

Sorry, I didn't know that. Is this better?

[-- Attachment #2: 0001-Let-zstyle-extra-verbose-show-parameter-values.txt --]
[-- Type: text/plain, Size: 3749 bytes --]

From 5ef8fcaabe243c8c4e04de92e1f971543470ce87 Mon Sep 17 00:00:00 2001
From: Marlon Richert <marlon.richert@gmail.com>
Date: Mon, 29 Mar 2021 22:56:34 +0300
Subject: [PATCH] Let `zstyle extra-verbose` show parameter values

When completing parameters and
`zstyle -t ":completion:${curcontext}:parameters" extra-verbose`,
display values of non-special parameters as descriptions.
---
 Completion/Zsh/Type/_parameters | 28 ++++++++++++++++++++++------
 Test/Y01completion.ztst         | 24 +++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/Completion/Zsh/Type/_parameters b/Completion/Zsh/Type/_parameters
index 207e5cf78..b32e049f7 100644
--- a/Completion/Zsh/Type/_parameters
+++ b/Completion/Zsh/Type/_parameters
@@ -6,13 +6,18 @@
 # If you specify a -g option with a pattern, the pattern will be used to
 # restrict the type of parameters matched.
 
-local expl pattern fakes faked tmp i pfilt
+local MATCH MBEGIN MEND \
+  disp dopt expl fakes faked i matches pattern pfilt sep tmp
 
 if compset -P '*:'; then
   _history_modifiers p
   return
 fi
 
+_tags parameters
+( _tags && _requested parameters ) ||
+  return
+
 pattern=(-g \*)
 zparseopts -D -K -E g:=pattern
 
@@ -32,8 +37,19 @@ zstyle -t ":completion:${curcontext}:parameters" prefix-needed && \
  [[ $PREFIX != [_.]* ]] && \
  pfilt='[^_.]'
 
-_wanted parameters expl parameter \
-    compadd "$@" -Q - \
-        "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
-        "$fakes[@]" \
-        "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+_description parameters expl parameter
+compadd "$expl[@]" -O matches - \
+  "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
+  "$fakes[@]" \
+  "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+
+if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
+  zstyle -s ":completion:${curcontext}:parameters" list-separator sep ||
+    sep=--
+  zformat -a disp " $sep " \
+    ${matches[@]:/(#m)*/"${MATCH}:${${parameters[$MATCH]:#*special*}:+${(Pkv@q+)MATCH}}"}
+  disp=( "${disp[@]:/(#m)*/$MATCH[1,COLUMNS]}" )
+  dopt=( -d disp )
+fi
+
+compadd "$expl[@]" $dopt -Q -a matches
diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 571f3cf16..e5901b8e5 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -222,7 +222,7 @@ F:regression test workers/31611
   comptesteval "zstyle ':completion:*:tst:*' ignored-patterns 2"
   comptest $'tst 1,\t'
   comptesteval "zstyle -d ':completion:*:tst:*' ignored-patterns"
-0:-F doesn't break _sequence
+0:-F doesn’t break _sequence
 >line: {tst 1,}{}
 >DESCRIPTION:{desc}
 >NO:{2}
@@ -237,6 +237,28 @@ F:regression test workers/31611
 >FI:{file1}
 >FI:{file2}
 
+  comptesteval "bar=({$'\\0'..$'\\C-?'}); baz=\$bar"
+  comptesteval 'zstyle ":completion:*:parameters" extra-verbose yes'
+  comptest $': $ba\t'
+0:extra-verbose shows parameter values
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar -- '^@' '^A' '^B' '^C' '^D' '^E' '^F' '^G' '^H' '\t' '\n' '^K' '^L' '^M' '^N}
+>NO:{baz -- '^@ ^A ^B ^C ^D ^E ^F ^G ^H \t \n ^K ^L ^M ^N ^O ^P ^Q ^R ^S ^T ^U ^V ^W }
+
+  comptest $': $path\C-D'
+0:extra-verbose does not show special parameter values
+>DESCRIPTION:{parameter}
+>NO:{path}
+
+  comptesteval 'zstyle -d ":completion:*:parameters" extra-verbose'
+  comptest $': $ba\t'
+0:parameter values not shown without extra-verbose
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar}
+>NO:{baz}
+
   comptesteval '_tst() { local disp=( {a..z} ); compadd -ld disp $disp[@]; comppostfuncs=( _pst ) }'
   comptesteval '_pst() { local disp=( "<INSERT>$compstate[insert]</INSERT>" ); compadd -Qld disp $disp }'
   comptesteval "zstyle ':completion:*' menu select=long-list"
-- 
2.31.0

Re: Feature Patch: Use completion to view parameter values

[Mikael Magnusson]

On 3/29/21, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Bart Schaefer wrote on Mon, Mar 29, 2021 at 10:20:08 -0700:
>> On Mon, Mar 29, 2021 at 10:11 AM Daniel Shahaf <d.s@daniel.shahaf.name>
>> wrote:
>> >
>> > I think that's right.  -workers@: Is it possible for a non-PM_SPECIAL
>> > parameter have a custom getfn?
>>
>> Only with the zsh/param/private module, I think, and in that case the
>> getfn is just a wrapper around the default and doesn't add any
>> side-effects.
>
> Thanks, Bart.
>
> And as to $AUTOINCREMENT, this isn't the first time I mentioned it as
> a hypothetical, so I'm going to go ahead and post it here.  I suspect
> people from the future will use this for something-or-other.
>
> Works as you'd expect:
> .
>     % echo $AUTOINCREMENT $AUTOINCREMENT
>     0 1
>     %
>
> And in Marlon's patch with the ${(t)…*special*} exclusion bypassed:
> .
>     % zstyle \* extra-verbose yes
>     % AUTOFOO=42
>     % echo $AUTO<TAB><TAB>
>     AUTOFOO       -- 42  AUTOINCREMENT -- 2
>     AUTOFOO       -- 42  AUTOINCREMENT -- 4
>
> Yes, it does actually increment the variable twice, probably because the
> _parameters patch uses both ${(t)${(P)}} and then ${(P)}, and the former
> does an increment too:
> .
>     % echo $AUTOINCREMENT ${(tP)AUTOINCREMENT} $AUTOINCREMENT
>     0 array-special 2
>     %
>
> I'm not proposing to commit $AUTOINCREMENT.
>
> Cheers,
>
> Daniel
>
> P.S.  Another similar example is Perl's magic flip-flop variable:
> «perl -E 'say --$| for (1..10)'»
>
>
> diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
> index ef9148d7b..179ac068e 100644
> --- a/Src/Modules/parameter.c
> +++ b/Src/Modules/parameter.c
> @@ -2136,6 +2136,24 @@ scanpmusergroups(UNUSED(HashTable ht), ScanFunc func,
> int flags)
>  }
>
>
> +/* Functions for the AUTOINCREMENT special parameter. */
> +
> +static zlong autoincrement = 0;
> +
> +static zlong
> +autoincrementgetfn(UNUSED(Param pm))
> +{
> +    return autoincrement++;
> +}
> +
> +static void
> +autoincrementsetfn(UNUSED(Param pm), zlong value)
> +{
> +    autoincrement = value;
> +}

If someone hypothetically wanted to use this, this (eg, the static
zlong autoincrement variable) should either be a zulong, or
autoincrementgetfn should check for wrapping, otherwise this is
undefined behavior (hypothetically) (unless we use -fwrapv, but I
don't think we do).

-- 
Mikael Magnusson

Re: Feature Patch: Use completion to view parameter values

[Daniel Shahaf]

Mikael Magnusson wrote on Tue, Mar 30, 2021 at 07:41:05 +0200:
> On 3/29/21, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Bart Schaefer wrote on Mon, Mar 29, 2021 at 10:20:08 -0700:
> >> On Mon, Mar 29, 2021 at 10:11 AM Daniel Shahaf <d.s@daniel.shahaf.name>
> >> wrote:
> >> >
> >> > I think that's right.  -workers@: Is it possible for a non-PM_SPECIAL
> >> > parameter have a custom getfn?
> >>
> >> Only with the zsh/param/private module, I think, and in that case the
> >> getfn is just a wrapper around the default and doesn't add any
> >> side-effects.
> >
> > Thanks, Bart.
> >
> > And as to $AUTOINCREMENT, this isn't the first time I mentioned it as
> > a hypothetical, so I'm going to go ahead and post it here.  I suspect
> > people from the future will use this for something-or-other.
> >
> > Works as you'd expect:
> > .
> >     % echo $AUTOINCREMENT $AUTOINCREMENT
> >     0 1
> >     %
> >
> > And in Marlon's patch with the ${(t)…*special*} exclusion bypassed:
> > .
> >     % zstyle \* extra-verbose yes
> >     % AUTOFOO=42
> >     % echo $AUTO<TAB><TAB>
> >     AUTOFOO       -- 42  AUTOINCREMENT -- 2
> >     AUTOFOO       -- 42  AUTOINCREMENT -- 4
> >
> > Yes, it does actually increment the variable twice, probably because the
> > _parameters patch uses both ${(t)${(P)}} and then ${(P)}, and the former
> > does an increment too:
> > .
> >     % echo $AUTOINCREMENT ${(tP)AUTOINCREMENT} $AUTOINCREMENT
> >     0 array-special 2
> >     %
> >
> > I'm not proposing to commit $AUTOINCREMENT.
> >
> > Cheers,
> >
> > Daniel
> >
> > P.S.  Another similar example is Perl's magic flip-flop variable:
> > «perl -E 'say --$| for (1..10)'»
> >
> >
> > diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
> > index ef9148d7b..179ac068e 100644
> > --- a/Src/Modules/parameter.c
> > +++ b/Src/Modules/parameter.c
> > @@ -2136,6 +2136,24 @@ scanpmusergroups(UNUSED(HashTable ht), ScanFunc func,
> > int flags)
> >  }
> >
> >
> > +/* Functions for the AUTOINCREMENT special parameter. */
> > +
> > +static zlong autoincrement = 0;
> > +
> > +static zlong
> > +autoincrementgetfn(UNUSED(Param pm))
> > +{
> > +    return autoincrement++;
> > +}
> > +
> > +static void
> > +autoincrementsetfn(UNUSED(Param pm), zlong value)
> > +{
> > +    autoincrement = value;
> > +}
> 
> If someone hypothetically wanted to use this, this (eg, the static
> zlong autoincrement variable) should either be a zulong, or
> autoincrementgetfn should check for wrapping, otherwise this is
> undefined behavior (hypothetically) (unless we use -fwrapv, but I
> don't think we do).

Thanks, Mikael.  Completely forgot about this.  The signatures in
gsu_integer specify signed and I don't want to deal with signed/unsigned
differences, so:

diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index 179ac068e..638ccc886 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -2143,6 +2143,10 @@ static zlong autoincrement = 0;
 static zlong
 autoincrementgetfn(UNUSED(Param pm))
 {
+    if (autoincrement == ZLONG_MAX) {
+	autoincrement = ZLONG_MIN;
+	return ZLONG_MAX;
+    }
     return autoincrement++;
 }
 
diff --git a/Src/zsh.h b/Src/zsh.h
index 6cf1b4186..536d728c5 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -38,12 +38,15 @@
 typedef ZSH_64_BIT_TYPE zlong;
 #if defined(ZLONG_IS_LONG_LONG) && defined(LLONG_MAX)
 #define ZLONG_MAX LLONG_MAX
+#define ZLONG_MIN LLONG_MIN
 #else
 #ifdef ZLONG_IS_LONG_64
 #define ZLONG_MAX LONG_MAX
+#define ZLONG_MIN LONG_MIN
 #else
 /* umm... */
 #define  ZLONG_MAX ((zlong)9223372036854775807)
+#define  ZLONG_MIN ((zlong)-9223372036854775808)
 #endif
 #endif
 #ifdef ZSH_64_BIT_UTYPE
@@ -55,6 +58,7 @@ typedef unsigned zlong zulong;
 typedef long zlong;
 typedef unsigned long zulong;
 #define ZLONG_MAX LONG_MAX
+#define ZLONG_MIN LONG_MIN
 #endif
 
 /*
diff --git a/Test/V06parameter.ztst b/Test/V06parameter.ztst
index 27d587852..610422abd 100644
--- a/Test/V06parameter.ztst
+++ b/Test/V06parameter.ztst
@@ -92,6 +92,37 @@
 >foo
 >bar
 
+ repeat 3 echo $AUTOINCREMENT
+ :
+ AUTOINCREMENT=42; 
+ repeat 3 echo $AUTOINCREMENT
+ :
+ AUTOINCREMENT=2147483647
+ repeat 3 echo $AUTOINCREMENT
+ :
+ AUTOINCREMENT=4294967295
+ repeat 3 echo $AUTOINCREMENT
+ :
+ AUTOINCREMENT=9223372036854775807
+ repeat 3 echo $AUTOINCREMENT
+0:$AUTOINCREMENT unit test
+>0
+>1
+>2
+>42
+>43
+>44
+>2147483647
+>2147483648
+>2147483649
+>4294967295
+>4294967296
+>4294967297
+>9223372036854775807
+*>(-9223372036854775808|9223372036854775809)
+*>(-9223372036854775807|9223372036854775810)
+F:This test assumes zlong is at least a 64-bit type.
+
 %clean
 
  rm -f autofn functrace.zsh rocky3.zsh sourcedfile myfunc

Cheers,

Daniel

Re: Feature Patch: Use completion to view parameter values

[Daniel Shahaf]

Daniel Shahaf wrote on Wed, Mar 31, 2021 at 22:55:14 +0000:
> +>4294967295
> +>4294967296
> +>4294967297
> +>9223372036854775807
> +*>(-9223372036854775808|9223372036854775809)
> +*>(-9223372036854775807|9223372036854775810)

Fixed the off-by-one on the RHS.

Also pushed the result to my github, if anyone wants to play with this.

Cheers,

Daniel

Re: Feature Patch: Use completion to view parameter values

[Marlon Richert]

> On 29. Mar 2021, at 23.35, Marlon Richert <marlon.richert@gmail.com> wrote:
> Sorry, I didn't know that. Is this better?
> <0001-Let-zstyle-extra-verbose-show-parameter-values.txt>

So, did we come to a conclusion? Was my patch accepted? Or do you need more changes?

Re: Feature Patch: Use completion to view parameter values

[Daniel Shahaf]

Marlon Richert wrote on Thu, Apr 01, 2021 at 07:28:23 +0300:
> 
> > On 29. Mar 2021, at 23.35, Marlon Richert <marlon.richert@gmail.com> wrote:
> > Sorry, I didn't know that. Is this better?
> > <0001-Let-zstyle-extra-verbose-show-parameter-values.txt>
> 
> So, did we come to a conclusion? Was my patch accepted? Or do you need more changes?

The patch has, at this point, neither been accepted nor been rejected.

For instance, we've confirmed that my concern about module-provided
parameters would be addressed by filtering out PM_SPECIAL, but I don't
know if the _implementation_ of the filtering has been reviewed.

Feel free to ping the thread again the patch hasn't been reviewed after
a few days.

Mikael: Have you seen Marlon's question to you in 48318?

Cheers,

Daniel

Re: Feature Patch: Use completion to view parameter values

[Oliver Kiddle]

On 29 Mar, Marlon Richert wrote:
> When completing parameters and
> `zstyle -t ":completion:${curcontext}:parameters" extra-verbose`,
> display values of non-special parameters as descriptions.

Incidentally, we actually already do this for array elements (with the
verbose style) but not for associative array elements. Numeric
indices are rather less meaningful so that only needing the verbose
style while this uses extra-verbose is probably sensible.

Following is a review of the patch:

> -local expl pattern fakes faked tmp i pfilt
> +local MATCH MBEGIN MEND \
> +  disp dopt expl fakes faked i matches pattern pfilt sep tmp

It isn't important but I tend to split these up by type and use `local
-a' or `local -i'. Setting the right type can avoid certain issues such
as += using the declared type.

> +_tags parameters
> +( _tags && _requested parameters ) ||
> +  return

I'm not entirely sure this is needed. Tag loop nesting isn't especially
ideal and helpers like _parameters are only called from places that
already have one. But it perhaps needs a little testing. I'm aware that
the old function had this in the form of _wanted.

> -_wanted parameters expl parameter \
> -    compadd "$@" -Q - \

Note the old function was passing "$@" which you've lost. This does
matter. Passed descriptions get thrown away, similarly pressing [ to
auto-remove space suffixes on arrays. "$@" is typically passed before
"$expl[@]".

The use of double indentation for continuation lines in the removed
lines is intentional by the way, see Etc/completion-style-guide

> +_description parameters expl parameter
> +compadd "$expl[@]" -O matches - \
> +  "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
> +  "$fakes[@]" \
> +  "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"

The faked parameters are not going to have a value. You can just add a
separate compadd for them right at the end (take care over return
status then). It will also then need to check if any faked parameters are
duplicates of real ones otherwise they'll appear twice.

> +
> +if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
> +  zstyle -s ":completion:${curcontext}:parameters" list-separator sep ||
> +    sep=--
> +  zformat -a disp " $sep " \
> +    ${matches[@]:/(#m)*/"${MATCH}:${${parameters[$MATCH]:#*special*}:+${(Pkv@q+)MATCH}}"}

We should probably also honour the typeset -H flag so the pattern would
need to be *(hideval|special)*

By not using _describe, this has the advantage that if the values are
all short, it can use columns. Unfortunately, many variables force it to
be a single column and then many lines have only a single short
parameter on their own. Compounding this, the parameters ! - ? @ * # $
and 0 get sorted near the top each getting a whole line.

_describe does the matches with a description first with the -l option
to compadd and then adds in undescribed matches. Perhaps this can be
presented better even if the parameters need to be spliced up.

For my own setup, I'd likely limit this style to when at least a few
characters have been typed which would avoid this; something like:
  zstyle -e ':completion:*:parameters' extra-verbose 'reply=( $(( ($#PREFIX+$#SUFFIX) > 2 )) )'

The formatting for arrays, associative arrays and empty strings could
perhaps also be nicer. Perhaps that's something for a future update.

> -0:-F doesn't break _sequence
> +0:-F doesn’t break _sequence

This is a somewhat separate change. Do we want to be using unicode here?

And, thanks. This looks useful.

Oliver

Re: Feature Patch: Use completion to view parameter values

[Lawrence Velázquez]

On Thu, Apr 1, 2021, at 8:50 PM, Oliver Kiddle wrote:
> Following is a review of the patch:

bump

vq

Re: Feature Patch: Use completion to view parameter values

[Marlon Richert]

> On 10 Apr 2021, at 23:20, Lawrence Velázquez <larryv@zsh.org> wrote:
> 
> On Thu, Apr 1, 2021, at 8:50 PM, Oliver Kiddle wrote:
>> Following is a review of the patch:
> 
> bump

I have read Oliver’s review, but I haven’t yet had the time to act on all of it.

Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Bart Schaefer]

With appreciation for Lawrence's efforts, I'd respectfully request
that the criteria for when to send a "bump" become a matter of record.

There seem to me to be these cases:

1.  The patch has never been reviewed or discussed.
2.  The patch was reviewed and is acceptable, but was never applied.
3.  There was a discussion, but it ended without resolution.
4.  The patch was referred back to the author after review or discussion.

There is room for subjective interpretation of "is acceptable".  A
possible resolution of #2 is that the patch is rejected after all
(perhaps it has become obsolete in the meantime).

I mention this mostly because I think the useful elapsed time before
"bumping" might be different in each case.  In particular #4 seems
like it could be left considerably longer, unless the patch is fixing
a serious bug or security issue.

Thoughts?

Re: Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Marlon]

> On 12 Apr 2021, at 00:24, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> With appreciation for Lawrence's efforts, I'd respectfully request
> that the criteria for when to send a "bump" become a matter of record.
> 
> There seem to me to be these cases:
> 
> 1.  The patch has never been reviewed or discussed.
> 2.  The patch was reviewed and is acceptable, but was never applied.
> 3.  There was a discussion, but it ended without resolution.
> 4.  The patch was referred back to the author after review or discussion.
> 
> There is room for subjective interpretation of "is acceptable".  A
> possible resolution of #2 is that the patch is rejected after all
> (perhaps it has become obsolete in the meantime).
> 
> I mention this mostly because I think the useful elapsed time before
> "bumping" might be different in each case.  In particular #4 seems
> like it could be left considerably longer, unless the patch is fixing
> a serious bug or security issue.
> 
> Thoughts?

I would suggest the following minimum wait times before bumping:

* To remind about an unresolved patch (not yet reviewed, not yet responded to by author after review, not yet accepted/rejected/committed, etc.):
  * security issues: 2 days
  * critical bug fixes: 1 week
  * all other patches: 2 weeks
* Everything else: 1 month

Additionally, it would be helpful if committers remember to inform us when a when a patch has been accepted/rejected/applied, to avoid unnecessary bumps.

Re: Feature Patch: Use completion to view parameter values

[Marlon]

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


> On 2 Apr 2021, at 03:50, Oliver Kiddle <opk@zsh.org> wrote:
> 
> Following is a review of the patch:

Attached is a new version of the patch that hopefully addresses the concerns you raised. Please review.


> And, thanks. This looks useful.

Great, I’m happy to hear that. :)


—Marlon


[-- Attachment #2: 0001-Let-extra-verbose-completion-show-parameter-values.txt --]
[-- Type: text/plain, Size: 4489 bytes --]

From ddc538f39966abecca8938464e8b9cee82c56a43 Mon Sep 17 00:00:00 2001
From: Marlon Richert <marlon.richert@gmail.com>
Date: Mon, 12 Apr 2021 23:17:23 +0300
Subject: [PATCH] Let extra-verbose completion show parameter values

---
 Completion/Zsh/Type/_parameters | 44 ++++++++++++++++++++++-----------
 Test/Y01completion.ztst         | 29 +++++++++++++++++++++-
 2 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/Completion/Zsh/Type/_parameters b/Completion/Zsh/Type/_parameters
index 207e5cf78..00c181e11 100644
--- a/Completion/Zsh/Type/_parameters
+++ b/Completion/Zsh/Type/_parameters
@@ -6,18 +6,39 @@
 # If you specify a -g option with a pattern, the pattern will be used to
 # restrict the type of parameters matched.
 
-local expl pattern fakes faked tmp i pfilt
-
 if compset -P '*:'; then
   _history_modifiers p
   return
 fi
 
-pattern=(-g \*)
+local MATCH i pfilt
+local -i MBEGIN MEND nm=$compstate[nmatches]
+local -a expl pattern=(-g \*) normal described verbose faked fakes tmp
+
+zstyle -t ":completion:${curcontext}:parameters" prefix-needed &&
+    [[ $PREFIX != [_.]* ]] &&
+        pfilt='[^_.]'
+_description parameters expl parameter
 zparseopts -D -K -E g:=pattern
 
-fakes=()
-faked=()
+if zstyle -t ":completion:${curcontext}:parameters" extra-verbose; then
+  described=(
+      "${(@M)${(@k)parameters[(R)$~pattern[2]~*(hideval|local|special)*]}:#$~pfilt*}"
+  )
+  compadd "$@" "$expl[@]" -D described -a - described
+  if (( $#described )); then
+    verbose=(
+        ${described[@]:/(#m)*/"${MATCH}:${(@q+)${(Pkv@q+)MATCH}//\\/\\\\}"} )
+    _describe -t parameters parameter verbose "$@" "$expl[@]"
+  fi
+
+  normal=(
+      "${(@M)${(@k)parameters[(R)$~pattern[2]~^(*(hideval|special)*)~*local*]}:#$~pfilt*}"
+  )
+else
+  normal=( "${(@M)${(@k)parameters[(R)$~pattern[2]~*local*]}:#$~pfilt*}" )
+fi
+
 if zstyle -a ":completion:${curcontext}:" fake-parameters tmp; then
   for i in "$tmp[@]"; do
     if [[ "$i" = *:* ]]; then
@@ -27,13 +48,8 @@ if zstyle -a ":completion:${curcontext}:" fake-parameters tmp; then
     fi
   done
 fi
+compadd "$@" "$expl[@]" - "$normal[@]" "${(@)fakes:|described}" \
+    "${(@)${(@)${(@M)faked:#${~pattern[2]}}%%:*}:|described}"
 
-zstyle -t ":completion:${curcontext}:parameters" prefix-needed && \
- [[ $PREFIX != [_.]* ]] && \
- pfilt='[^_.]'
-
-_wanted parameters expl parameter \
-    compadd "$@" -Q - \
-        "${(@M)${(@k)parameters[(R)${pattern[2]}~*local*]}:#${~pfilt}*}" \
-        "$fakes[@]" \
-        "${(@)${(@M)faked:#${~pattern[2]}}%%:*}"
+(( compstate[nmatches] > nm ))
+return 0
diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 858fa7220..2c6d25d9a 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -240,7 +240,7 @@ F:regression test workers/31611
   comptesteval "zstyle ':completion:*:tst:*' ignored-patterns 2"
   comptest $'tst 1,\t'
   comptesteval "zstyle -d ':completion:*:tst:*' ignored-patterns"
-0:-F doesn't break _sequence
+0:-F does not break _sequence
 >line: {tst 1,}{}
 >DESCRIPTION:{desc}
 >NO:{2}
@@ -255,6 +255,33 @@ F:regression test workers/31611
 >FI:{file1}
 >FI:{file2}
 
+  comptesteval "bar=({$'\\0'..$'\\C-?'}); baz=\$bar"
+  comptesteval 'zstyle ":completion:*:parameters" extra-verbose yes'
+  comptesteval 'zstyle ":completion:*" fake-parameters bar baz:array'
+  comptest $': $ba\t'
+0:extra-verbose shows parameter values
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar  -- '^@' '^A' '^B' '^C' '^D' '^E' '^F' '^G' '^H' '\t' '\n' '^K' '^L' '^M}
+>NO:{baz  -- '^@ ^A ^B ^C ^D ^E ^F ^G ^H \t \n ^K ^L ^M ^N ^O ^P ^Q ^R ^S ^T ^U ^}
+
+  comptesteval "path=( $ZTST_srcdir:A )"
+  comptesteval 'typeset -H paths=HIDDEN'
+  comptest $': $path\t'
+0:extra-verbose doesn't show special or hidden parameter values
+>line: {: $path}{}
+>DESCRIPTION:{parameter}
+>NO:{path}
+>NO:{paths}
+
+  comptesteval 'zstyle -d ":completion:*:parameters" extra-verbose'
+  comptest $': $ba\t'
+0:parameter values not shown without extra-verbose
+>line: {: $ba}{}
+>DESCRIPTION:{parameter}
+>NO:{bar}
+>NO:{baz}
+
   comptesteval '_tst() { local disp=( {a..z} ); compadd -ld disp $disp[@]; comppostfuncs=( _pst ) }'
   comptesteval '_pst() { local disp=( "<INSERT>$compstate[insert]</INSERT>" ); compadd -Qld disp $disp }'
   comptesteval "zstyle ':completion:*' menu select=long-list"
-- 
2.31.0

Re: Feature Patch: Use completion to view parameter values

[Bart Schaefer]

On Mon, Apr 12, 2021 at 1:23 PM Marlon <marlon.richert@gmail.com> wrote:
>
> +local -a expl pattern=(-g \*) normal described verbose faked fakes tmp
[...]
> -fakes=()
> -faked=()

I don't think it matters in this instance, but what you have and what
was there are not equivalent if the declarednull changes are in
effect.

Mainly this reminds me to be sure _comp_options establishes the correct state.

Re: Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Lawrence Velázquez]

> On Apr 11, 2021, at 5:24 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> With appreciation for Lawrence's efforts, I'd respectfully request
> that the criteria for when to send a "bump" become a matter of record.

Certainly!  I've been combing the lists every Saturday afternoon/evening
UTC and uniformly bumping recent discussions that have been inactive for
more than five days (to loop in the preceding weekend).

> There seem to me to be these cases:
> 
> 1.  The patch has never been reviewed or discussed.
> 2.  The patch was reviewed and is acceptable, but was never applied.
> 3.  There was a discussion, but it ended without resolution.
> 4.  The patch was referred back to the author after review or discussion.

I've observed a parallel set of cases initiated by committers'
requests for feedback.  The dynamic is somewhat different; the
discussions are never held up due to a participant's technical
constraints, and they tend to be more open-ended.  Plus, the
"contributor morale" benefits [*] of reminders don't apply.

> I mention this mostly because I think the useful elapsed time before
> "bumping" might be different in each case.  In particular #4 seems
> like it could be left considerably longer

I've been thinking along similar lines myself and have found an
alternative taxonomy to be clarifying:

(A) A noncommitter is waiting on a committer (#1, #2, #3).
(B) A committer is waiting on a noncommitter (#4).
(C) A committer is waiting on other committers (the parallel cases).

As the reason the "patch manager" role exists [*], group A should
be handled expeditiously, while groups B and C can wait.  (This
classification reflects how meddlesome I feel when I send reminders.)

I think a threshold of 1-2 weeks remains appropriate for A, but
perhaps ~1 month would better suit B and C?

> unless the patch is fixing a serious bug or security issue.

Do you think we need to prescribe a standard for these?  They seem
pretty rare, and committers are unlikely to let them drop through
the cracks.  My initial inclination is to leave them to committers'
discretion.  (I'm not privy to zsh-security@ anyway.)


  [*]: https://producingoss.com/en/share-management.html#patch-manager
       "The project might miss out on a useful patch this way, and
       there are other harmful side effects as well: it is discouraging
       to the author, who invested work in the patch, and it is
       discouraging to others considering writing patches."


-- 
vq

Re: Feature Patch: Use completion to view parameter values

[Marlon Richert]

> On 13 Apr 2021, at 00:49, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> On Mon, Apr 12, 2021 at 1:23 PM Marlon <marlon.richert@gmail.com> wrote:
>> 
>> +local -a expl pattern=(-g \*) normal described verbose faked fakes tmp
> [...]
>> -fakes=()
>> -faked=()
> 
> I don't think it matters in this instance, but what you have and what
> was there are not equivalent if the declarednull changes are in
> effect.

Sorry, I’m not familiar with those changes. Can you summarize them for me or point me to a summary?

Re: Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Daniel Shahaf]

[Marlon: Your quote attribution line is indented one more level than is
conventional]

Marlon wrote on Mon, Apr 12, 2021 at 11:18:43 +0300:
> On 12 Apr 2021, at 00:24, Bart Schaefer <schaefer@brasslantern.com> wrote:
> > With appreciation for Lawrence's efforts, I'd respectfully request
> > that the criteria for when to send a "bump" become a matter of record.
> > 
> > There seem to me to be these cases:
> > 
> > 1.  The patch has never been reviewed or discussed.
> > 2.  The patch was reviewed and is acceptable, but was never applied.
> > 3.  There was a discussion, but it ended without resolution.
> > 4.  The patch was referred back to the author after review or discussion.
> > 
> > There is room for subjective interpretation of "is acceptable".  A
> > possible resolution of #2 is that the patch is rejected after all
> > (perhaps it has become obsolete in the meantime).
> > 
> > I mention this mostly because I think the useful elapsed time before
> > "bumping" might be different in each case.  In particular #4 seems
> > like it could be left considerably longer, unless the patch is fixing
> > a serious bug or security issue.
> > 
> > Thoughts?

Leaving #4 "considerably longer" would decrease temporal locality in the
patch authors' brains, would reap less "the project noticed my lack of
response" benefits (cf.
https://producingoss.com/en/managing-participants.html#delegation-followup),
and would be more likely to find the patch author busy with other things
and unable to follow up and post a revised patch.

You don't actually say what benefits you seek to attain.  Is it about,
say, bumps that are replies to previous bumps?  If so, I'd propose, say:

- A bump that is a reply to a non-bump should be sent with delay X.

- A bump that is a reply to a bump should be sent with delay Y.

- A bump that is a reply to a bump that is a reply to a bump should be
  sent with delay Z and added to git (as proposed in workers/48303).

- A bump (that is a reply to a bump)³ should not be sent.

(Feel free to read "should" and "should not" in their rfc2119 senses.)

> Additionally, it would be helpful if committers remember to inform us
> when a when a patch has been accepted/rejected/applied,

This would be helpful for other other reasons too:

- It would let the patch submitter know their patch has been accepted.
  (Simply committing the patch to git without replying to it might leave
  the patch submitter think they were ignored.)

- It would make it easier for other committers to skip PATCH threads
  that have already been handled.

> to avoid unnecessary bumps.

Cheers,

Daniel

Re: Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Daniel Shahaf]

Marlon wrote on Mon, Apr 12, 2021 at 11:18:43 +0300:
> On 12 Apr 2021, at 00:24, Bart Schaefer <schaefer@brasslantern.com> wrote:
> > 1.  The patch has never been reviewed or discussed.
> > 2.  The patch was reviewed and is acceptable, but was never applied.
> > 3.  There was a discussion, but it ended without resolution.
> > 4.  The patch was referred back to the author after review or discussion.
⋮
> > I mention this mostly because I think the useful elapsed time before
> > "bumping" might be different in each case.  In particular #4 seems
> > like it could be left considerably longer, unless the patch is fixing
> > a serious bug or security issue.
> 
> I would suggest the following minimum wait times before bumping:
> 
> * To remind about an unresolved patch (not yet reviewed, not yet responded to by author after review, not yet accepted/rejected/committed, etc.):
>   * security issues: 2 days
>   * critical bug fixes: 1 week
>   * all other patches: 2 weeks
> * Everything else: 1 month

I'm not sure what cases fall into "etc." and what cases fall into
"everything else", nor what would a "critical" bugfix be.

Lawrence Velázquez wrote on Mon, Apr 12, 2021 at 22:47:50 -0400:
> On Apr 11, 2021, at 5:24 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> > With appreciation for Lawrence's efforts, I'd respectfully request
> > that the criteria for when to send a "bump" become a matter of record.
> 
> Certainly!  I've been combing the lists every Saturday afternoon/evening
> UTC and uniformly bumping recent discussions that have been inactive for
> more than five days (to loop in the preceding weekend).

> 

⋮

> (A) A noncommitter is waiting on a committer (#1, #2, #3).
> (B) A committer is waiting on a noncommitter (#4).
> (C) A committer is waiting on other committers (the parallel cases).
> 
> As the reason the "patch manager" role exists [*], group A should
> be handled expeditiously, while groups B and C can wait.  (This
> classification reflects how meddlesome I feel when I send reminders.)
> 
> I think a threshold of 1-2 weeks remains appropriate for A, but
> perhaps ~1 month would better suit B and C?

How would differential delays affect your workflow?  A uniform criterion
(such as "Thread has been dormant for >5 days") should be easier to apply.

Regarding your taxonomy, would it be accurate to say that in cases
A and B a submitted patch is awaiting resolution, whereas in case C it's
generally a design question that's awaiting resolution?  In A+B the
person in question is the patch submitter; in C the person in question
is probably a regular developer.

(Aside: Note the terminology: "developer", not "committer", since in
general, distinctions between people who do and don't have commit access
shouldn't be made, except when it's necessary to actually invoke «git
push».¹)

Regarding the magic numbers, I think one month is too long for case B
(cf. my remarks today in workers/48526).  We don't want to bump _too_
soon either, but I'd aim for something on the order of a week (for the
first ping, again as per 48526).  "Once a week for patches ≥6 days old"
achieves that, as would, say, "at least 48 weekend hours and at least
72 weekday hours".

No comment from me on case C.

Cheers,

Daniel

> > unless the patch is fixing a serious bug or security issue.
> 
> Do you think we need to prescribe a standard for these?  They seem
> pretty rare, and committers are unlikely to let them drop through
> the cracks.  My initial inclination is to leave them to committers'
> discretion.  (I'm not privy to zsh-security@ anyway.)

Re: Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Lawrence Velázquez]

> On Apr 13, 2021, at 8:32 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> 
> [Marlon: Your quote attribution line is indented one more level than is
> conventional]

This is how Apple's email clients quote.  I couldn't tell you why.

> Leaving #4 "considerably longer" would decrease temporal locality in the
> patch authors' brains, would reap less "the project noticed my lack of
> response" benefits (cf.
> https://producingoss.com/en/managing-participants.html#delegation-followup),
> and would be more likely to find the patch author busy with other things
> and unable to follow up and post a revised patch.

Good point.

> You don't actually say what benefits you seek to attain. Is it about,
> say, bumps that are replies to previous bumps?  If so, I'd propose, say:
> 
> - A bump that is a reply to a non-bump should be sent with delay X.
> 
> - A bump that is a reply to a bump should be sent with delay Y.
> 
> - A bump that is a reply to a bump that is a reply to a bump should be
>  sent with delay Z and added to git (as proposed in workers/48303).
> 
> - A bump (that is a reply to a bump)³ should not be sent.

I'm not sure about varying delays, but it would be nice to have a
place to stash discussions to avoid infinite reminders.

>> Additionally, it would be helpful if committers remember to inform us
>> when a when a patch has been accepted/rejected/applied,
> 
> This would be helpful for other other reasons too:
> 
> - It would let the patch submitter know their patch has been accepted.
>  (Simply committing the patch to git without replying to it might leave
>  the patch submitter think they were ignored.)
> 
> - It would make it easier for other committers

(and me!)

> to skip PATCH threads that have already been handled.

-- 
vq

Re: Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Lawrence Velázquez]

> On Apr 13, 2021, at 9:35 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> 
> Marlon wrote on Mon, Apr 12, 2021 at 11:18:43 +0300:
>> On 12 Apr 2021, at 00:24, Bart Schaefer <schaefer@brasslantern.com> wrote:
>>> 1.  The patch has never been reviewed or discussed.
>>> 2.  The patch was reviewed and is acceptable, but was never applied.
>>> 3.  There was a discussion, but it ended without resolution.
>>> 4.  The patch was referred back to the author after review or discussion.
> ⋮
>>> I mention this mostly because I think the useful elapsed time before
>>> "bumping" might be different in each case.  In particular #4 seems
>>> like it could be left considerably longer, unless the patch is fixing
>>> a serious bug or security issue.
>> 
>> I would suggest the following minimum wait times before bumping:
>> 
>> * To remind about an unresolved patch (not yet reviewed, not yet responded to by author after review, not yet accepted/rejected/committed, etc.):
>>  * security issues: 2 days
>>  * critical bug fixes: 1 week
>>  * all other patches: 2 weeks
>> * Everything else: 1 month
> 
> I'm not sure what cases fall into "etc." and what cases fall into
> "everything else", nor what would a "critical" bugfix be.

"Everything else" seems like "anything not involving an unresolved
patch", which is out of scope as far as I'm concerned.

> How would differential delays affect your workflow?  A uniform criterion
> (such as "Thread has been dormant for >5 days") should be easier to apply.

The uniform criterion is definitely easier.  I'm open to giving
certain patch discussions more room to breathe, but at the granularity
of two classes (more urgent vs. less urgent) and not four or more.

> Regarding your taxonomy, would it be accurate to say that in cases
> A and B a submitted patch is awaiting resolution, whereas in case C it's
> generally a design question that's awaiting resolution?  In A+B the
> person in question is the patch submitter; in C the person in question
> is probably a regular developer.

That tracks with what I've seen, although in all cases there's at
least one patch in flight.  (After all, the role is "patch manager",
not "discussion manager".)

Type A also includes discussions wherein a contribution has been
accepted but not yet committed.

Many type C discussions are just a committer saying "I'm thinking
about committing this, what does everyone think?" and then waiting
on any feedback, from nitpicks to overarching design critique.

> (Aside: Note the terminology: "developer", not "committer", since in
> general, distinctions between people who do and don't have commit access
> shouldn't be made, except when it's necessary to actually invoke «git
> push».¹)

Sure, but commit access is relevant to patch discussions involving
noncommitting contributors (types A and B) because the commit step
is often the only thing holding things up.

> Regarding the magic numbers, I think one month is too long for case B
> (cf. my remarks today in workers/48526).  We don't want to bump _too_
> soon either, but I'd aim for something on the order of a week (for the
> first ping, again as per 48526).  "Once a week for patches ≥6 days old"
> achieves that, as would, say, "at least 48 weekend hours and at least
> 72 weekday hours".

This effectively retains my current policy for types A and B.  I'd
be fine with this, but we haven't heard Bart's rationale for longer
intervals.

> No comment from me on case C.

Should I continue bumping developer-only patch discussions at all?
If so, I'm inclined to let them simmer for longer -- perhaps a month
(as per workers/48516).  I feel pretty pesky basically reminding
committers to commit their own patches, but everyone forgets things
now and then.  Is it helpful or annoying?

-- 
vq

Re: Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Bart Schaefer]

On Tue, Apr 13, 2021 at 2:32 PM Lawrence Velázquez <larryv@zsh.org> wrote:
>
> This effectively retains my current policy for types A and B.  I'd
> be fine with this, but we haven't heard Bart's rationale for longer
> intervals.

Mostly that in cases where there's been feedback to which the patch
submitter has not responded, pinging them is more likely than anything
to elicit either no response or a "sorry, ran out of time to work on
this", and in either case pinging again the same number of days later
is probably more annoying than helpful.  Patches that have been
entirely ignored or that are waiting on someone to push, are a
different matter.

> Should I continue bumping developer-only patch discussions at all?
> If so, I'm inclined to let them simmer for longer -- perhaps a month
> (as per workers/48516).  I feel pretty pesky basically reminding
> committers to commit their own patches, but everyone forgets things
> now and then.  Is it helpful or annoying?

In my case, once is helpful, twice in 10 days would be annoying.  But
I don't know how other committers allocate their time (and it's
currently more difficult for me to commit other people's patches than
it ought to be, and I can't say when I'll get that rectified).

Re: Patch bumping (was Re: Feature Patch: Use completion to view parameter values)

[Daniel Shahaf]

Lawrence Velázquez wrote on Tue, Apr 13, 2021 at 17:31:30 -0400:
> On Apr 13, 2021, at 9:35 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Regarding your taxonomy, would it be accurate to say that in cases
> > A and B a submitted patch is awaiting resolution, whereas in case C it's
> > generally a design question that's awaiting resolution?  In A+B the
> > person in question is the patch submitter; in C the person in question
> > is probably a regular developer.
> 
> Many type C discussions are just a committer saying "I'm thinking
> about committing this, what does everyone think?" and then waiting
> on any feedback, from nitpicks to overarching design critique.

In these, I think it matters whether the "this" referred to is the
committer's work or a patch submitter's work.  In the latter case, it's
essentially case A again.

> > (Aside: Note the terminology: "developer", not "committer", since in
> > general, distinctions between people who do and don't have commit access
> > shouldn't be made, except when it's necessary to actually invoke «git
> > push».¹)
> 
> Sure, but commit access is relevant to patch discussions involving
> noncommitting contributors (types A and B) because the commit step
> is often the only thing holding things up.

Personally, I don't recall that many threads ending with "the patch
LGTM, just need to push it when I'll get home" or similar.

> > No comment from me on case C.
> 
> Should I continue bumping developer-only patch discussions at all?
> If so, I'm inclined to let them simmer for longer -- perhaps a month
> (as per workers/48516).  I feel pretty pesky basically reminding
> committers to commit their own patches, but everyone forgets things
> now and then.  Is it helpful or annoying?

Personally, I keep on my todo list patches I've posted, but not patches
I've reviewed or random ideas I've floated.  I suspect that in general
reminders would be helpful when a patch submission requests
feedback/comments/reviews or asks a question.

Cheers,

Daniel

[META] Tone of voice / Writing style in patch reviews (was Re: Patch bumping)

[Marlon]

Hi, all!

On 13 Apr 2021, at 21:08, Lawrence Velázquez <larryv@zsh.org> wrote:
> On Apr 13, 2021, at 8:32 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>> Leaving #4 "considerably longer" would decrease temporal locality in the
>> patch authors' brains, would reap less "the project noticed my lack of
>> response" benefits (cf.
>> https://producingoss.com/en/managing-participants.html#delegation-followup),
>> and would be more likely to find the patch author busy with other things
>> and unable to follow up and post a revised patch.
> 
> Good point.

After writing workers/48583, I would like to add something to this that I think is even more important for keeping new contributors motivated to stay engaged (from the same resource as above):

From [Praise and Criticism](https://producingoss.com/en/managing-participants.html#praise-and-criticism):
> An important feature of technical culture is that detailed, dispassionate criticism is often taken as a kind of praise (as discussed in the section called “Recognizing Rudeness”), because of the implication that the recipient's work is worth the time required to analyze it. However, both of those conditions — detailed and dispassionate — must be met for this to be true. For example, if someone makes a sloppy change to the code, it is useless (and actually harmful) to follow up saying simply "That was sloppy." Sloppiness is ultimately a characteristic of a person, not of their work, and it's important to keep your reactions focused on the work. It's much more effective to describe all the things wrong with the change, tactfully and without malice.

From [Recognizing Rudeness](https://producingoss.com/en/you-are-what-you-write.html#rudeness):
> So what is rude?
> 
> By the same principle under which detailed technical criticism is a form of flattery, failure to provide quality criticism can be a kind of insult. I don't mean simply ignoring someone's work, be it a proposal, code change, new ticket filing, or whatever. Unless you explicitly promised a detailed reaction in advance, it's usually okay to simply not react at all. People will assume you just didn't have time to say anything. But if you do react, don't skimp: take the time to really analyze things, provide concrete examples where appropriate, dig around in the archives to find related posts from the past, etc. Or if you don't have time to put in that kind of effort, but still need to write some sort of brief response, then state the shortcoming openly in your message ("I think there's a ticket filed for this, but unfortunately didn't have time to search for it, sorry"). The main thing is to recognize the existence of the cultural norm, either by fulfilling it or by openly acknowledging that one has fallen short this time. Either way, the norm is strengthened. But failing to meet that norm, while at the same time not explaining why you failed to meet it, is like saying the topic (and those participating in it) was not worth much of your time — that your time is more valuable than theirs. Better to show that your time is valuable by being terse than by being lazy.

I know I am new to this project, but I found Daniel's tone of voice/writing style in workers/48571 quite rude, both on a personal level and according to the definition above. Let’s not treat each other like this, shall we? Just point out why and how you think I should fix the mistakes I make, and I will be happy to oblige. But if you neither explain _why_ what I did was wrong nor _how_ I should fix it, then your feedback is neither constructive nor actionable.

Finally, I would like to quote two points from [The 10 Commandments of Egoless Programming](https://blog.codinghorror.com/the-ten-commandments-of-egoless-programming/):
> 5. Treat people who know less than you with respect, deference, and patience. Nontechnical people who deal with developers on a regular basis almost universally hold the opinion that we are prima donnas at best and crybabies at worst. Don't reinforce this stereotype with anger and impatience.
> […]
> 10. Critique code instead of people – be kind to the coder, not to the code. As much as possible, make all of your comments positive and oriented to improving the code.


Kind regards,
—Marlon

PS: Look, I even fixed the indentation of my quote attribution lines for you, Daniel, in this email. You can’t say I don’t listen. ;)

RE: [META] Tone of voice / Writing style in patch reviews (was Re: Patch bumping)

[zeurkous]

Haai,

"Marlon" <marlon.richert@gmail.com> wrote:
>Hi, all!
>
> After writing workers/48583,

Disclaimer: didn't read that. Should me?

> I would like to add something to this that I think is even more important for keeping new contributors motivated to stay engaged (from the same resource as above):
>
> From [Praise and Criticism](https://producingoss.com/en/managing-participants.html#praise-and-criticism):
>> An important feature of technical culture is that detailed, dispassionate criticism is often taken as a kind of praise (as discussed in the section called "Recognizing Rudeness"), because of the implication that the recipient's work is worth the time required to analyze it. However, both of those conditions -- detailed and dispassionate -- must be met for this to be true. For example, if someone makes a sloppy change to the code, it is useless (and actually harmful) to follow up saying simply "That was sloppy." Sloppiness is ultimately a characteristic of a person, not of their work, and it's important to keep your reactions focused on the work. It's much more effective to describe all the things wrong with the change, tactfully and without malice.

A work tends to inherit some (many?) of the creator's characteristics,
though. And devastating criticism can be easily given without malice.

The flip side of the latter is that lavish praise can be given without
good feelings on the part of the praiser.

Me's new here (obviously), but to me, it's actually a relief that toes
do not appear to be easily stepped on. That's different in much of the
{,``OSS'' }world. Me*thinks* me knows why, but that would appear to be
beyond the scope of this discussion.

> From [Recognizing Rudeness](https://producingoss.com/en/you-are-what-you-write.html#rudeness):
>> So what is rude?
>>
>> By the same principle under which detailed technical criticism is a form of flattery, failure to provide quality criticism can be a kind of insult. I don't mean simply ignoring someone's work, be it a proposal, code change, new ticket filing, or whatever. Unless you explicitly promised a detailed reaction in advance, it's usually okay to simply not react at all. People will assume you just didn't have time to say anything. But if you do react, don't skimp: take the time to really analyze things, provide concrete examples where appropriate, dig around in the archives to find related posts from the past, etc. Or if you don't have time to put in that kind of effort, but still need to write some sort of brief response, then state the shortcoming openly in your message ("I think there's a ticket filed for this, but unfortunately didn't have time to search for it, sorry"). The main thing is to recognize the existence of the cultural norm, either by fulfilling it or by openly acknowledging that one has fallen short this time. Either way, the norm is strengthened. But failing to meet that norm, while at the same time not explaining why you failed to meet it, is like saying the topic (and those participating in it) was not worth much of your time -- that your time is more valuable than theirs. Better to show that your time is valuable by being terse than by being lazy.

Cultural norms vary from place to place -- otherwise it wouldn't be
culture. Are you suggesting that the cultural norm on this list be
changed...? 

> I know I am new to this project, but I found Daniel's tone of voice/writing style in workers/48571 quite rude, both on a personal level and according to the definition above. Let's not treat each other like this, shall we? Just point out why and how you think I should fix the mistakes I make, and I will be happy to oblige. But if you neither explain _why_ what I did was wrong nor _how_ I should fix it, then your feedback is neither constructive nor actionable.

Rude? You know what's rude? "Don't see the point, sorry.". Or
purposefully ignorning trivial patches to obvious problems, even after n
pings. Or mud-slinging them (not even bothering to make a good flame)
because the author isn't liked. Or shunning a contributor 'cause he
can't help but point out larger issues along the way, issues too big for
them to chew. All of that, and more, has happened to me. And rest
assured me's not the only one.

Again, there's a flip side: fluffy and sweet coders, who may've followed
a course in coding or two, but have no idea how to program, let alone
how to communicate about programming. Many of those mantain projects,
perhaps even projects that you and me depend on. Me'd argue that's no
good, either. Some ballast is needed.

Now, me's not going to stick too many feathers up the butt of a person
me's only exchanged a couple of msgs with, yet... compared to the above
attiudes, the "friendly, to-the-point" style of Daniel is
$DEITY_RESIDENCE to me.

That leaves me to wonder if the "do your homework" bit is what ticked
you off in particular.

> Finally, I would like to quote two points from [The 10 Commandments of Egoless Programming](https://blog.codinghorror.com/the-ten-commandments-of-egoless-programming/):
>> 5. Treat people who know less than you with respect, deference, and patience. Nontechnical people who deal with developers on a regular basis almost universally hold the opinion that we are prima donnas at best and crybabies at worst. Don't reinforce this stereotype with anger and impatience.

Most self-declared "techies" *are* primadonnas and crybabies.
$UNDEITY_RESIDENCE, even many of those that *are* actually competent,
are successfully running major projects, and are generally safe and
secure in their current social position, act that way.

Me supposes they're just lusers in disguise.

>> [...]
>> 10. Critique code instead of people - be kind to the coder, not to the code. As much as possible, make all of your comments positive and oriented to improving the code.

As me's more than hinted at above: code is often an extension of its
creator. Even if the reviewer can make the distinction, the creator
often can't. The reality is that many creators have long toes, but that
makes them just as responsible for implementing the obvious workaround:
"Keep a little distance.".

> Kind regards,
> --Marlon
>
> PS: Look, I even fixed the indentation of my quote attribution lines for you, Daniel, in this email. You can't say I don't listen. ;)

That's great, but please, next time when you quote from a WWW page, do
fold(1) the paragraphs.

Take care,

        --zeurkous.

-- 
Friggin' Machines!