Re: Bug + patch: `zstyle ':completion:*' menu select=long-list` fails to start menu selection

["Daniel Shahaf" <d.s@daniel.shahaf.name>]

Replying on-list with permission and fullquote.

Peter Stephenson wrote on Fri, 12 Mar 2021 14:42 +00:00:
> > On 12 March 2021 at 14:14 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Peter Stephenson wrote on Fri, 12 Mar 2021 13:36 +00:00:
> > > 
> > > > On 12 March 2021 at 13:11 Marlon Richert <marlon.richert@gmail.com> wrote:
> > > > 
> > > > 
> > > > I found the culprit: I had
> > > > 
> > > > export GREP_OPTIONS='--color=always'
> > > > 
> > > > in my `.zshrc` file and that mangled the .mdd file names.
> > > 
> > > It's probably worth having the following.  It doesn't cover all the
> > > possible cases where you can get into trouble, but it's a useful
> > > blanket for the standard case where everything is done immediately
> > > from configure.
> > > 
> > 
> > I'm not sure I agree.
> > 
> > - This seems to be a cases of "hard cases make bad law".  Setting
> >   --color=always in the environment will break any script that expects
> >   grep(1)'s standard semantics, not just configure.  The patch just
> >   papers over the problem.
> > 
> > - We shouldn't second-guess the user.  If the user has GREP_OPTIONS set
> >   in the environment, that might actually be needed in order to have
> >   grep(1) behave correctly.  What if some system uses GREP_OPTIONS to
> >   make its grep(1) tool behave POSIX compatibly?
> 
> I think that's missing the point of the patch, which is that for anyone
> setting up zsh, configure should just run in a sandbox with whatever is
> going on outside irrelevant to it;

It is an API promise of autoconf that AC_PROG_GREP will use the
environment variable $GREP to find a grep program.

I would therefore expect «GREP=/bin/foo GREP_OPTIONS=bar ./configure» to
also be supported, even for values of foo other than GNU grep.

> it's an implementation detail that it's a shell script at all.

Well, yes, configure could be implemented in Rust for all anyone cares,
but it would still have to obey that API promise about the "GREP"
environment variable.

> GREP_OPTIONS is a user extension, there's no
> question of it being needed for POSIX --- GNU do things this way exactly so
> that if you have a vanilla environment it is basically (but not necessarily
> completely, as this is a complicated area) POSIXy.
> 

In GNU's case, yes, but non-GNU implementations of grep may also use
envvars named GREP_OPTIONS, and your patch could easily break those.

Even with GNU grep, I could imagine someone setting
GREP_OPTIONS=--exclude='.git', and then a configure script using «grep -R»
if $GREP happened to be GNU grep.  (even if zsh's configure script doesn't do that)

Also, someone might run «CC=/my/wrapper ./configure» where /my/wrapper
is a shell script that uses grep and relies on GREP_OPTIONS being set.

> If you do feel the need to pursue a more complicated path, however, you're
> welcome to do so and I'll keep out of the way.
> 

Let's take a step back, please.  You proposed a change.  I don't think
that change is a good one to make.  For instance, consider that if the
patch is a good change, it would need to be applied to every single
configure script out there; actually, to _every single portable script_
that uses grep.

I view the issue as a bug in Marlon's dotfiles; a bug which he has
discovered and fixed.  The failure mode wasn't ideal, of course, and we
could look into improving that; for example, by s/grep/$GREP/ as I
suggested and proposing a "look for colour codes in the output" logic to
AC_PROG_GREP.

I did propose further alternatives upthread and I welcome feedback and
questions on them.

And needless to say, I didn't mean to discourage you from participating.

> > - If this fix is needed, we should send it to autoconf upstream to be
> >   incorporated into AC_PROG_GREP.
> 
> That does sound entirely reasonable, if no one's noticed yet.

*nod*

Cheers,

Daniel