Re: [PATCH 0/3] completion: make: various improvements

[Felipe Contreras <felipe.contreras@xxxxxxxxx>]

On Tue, Aug 2, 2022 at 5:43 AM Jun T <takimoto-j@xxxxxxxxxxxxxxxxx> wrote:
>
> > 2022/07/30 10:03, Felipe Contreras <felipe.contreras@xxxxxxxxx> wrote:
> >
> > While using `call-command` I found very serious issues, for starters
> > trying to complete `make` in the zsh source code takes several minutes
> > to complete.
> >
> >  zstyle ':completion:*:make:*:targets' call-command true

> [PATCH 1/3]
> > At least in GNU make 4.3 the -n option is *not* respected and
> > --always-make builds everything.
>
> This is indeed serious; just hitting <TAB> for completion should
> _never_ build anything.
>
> > Instead use a fake .DEFAULT target the way bash-completion does.
>
> If there is a target .DEFAULT in the user's Makefile, its recipe
> is not executed (due to the -n option) but is echoed to stdout
> (although the -s option is passed to make; I don't know why).
> In the _worst_ case this may confuse make-parseDataBase().
> For example, if Makefile has the following two lines:
>
> .DEFAULT:
>         echo foo: bar
>
> then the output of 'make -nsp .DEFAULT' contains a line
>
> echo foo: bar
>
> and 'echo foo' will be offered as a possible target.
> But I think we can ignore such (extremely) rare cases.

That's right. I used .DEFAULT because that's what bash-completion
does, but in their script they ignore everything before the Files
section in the GNU make database output, so that's not a problem.

We could alternatively use ":" which I believe is impossible to name a
target that way. I've seen comments online using that, but I decided
to go for something that was tried-and-true.

I don't care either way. Anything that doesn't build all the targets
is fine by me.

> [PATCH 2/3]
> > The make program does all the heavy lifting, there's no need to use a
> > full parser.
>
> With this patch, if I type 'make <TAB>' in the zsh src directory,
> it offers the following files as targets:
>     configure.ac, Makefile.in, aclocal.m4   etc.
> These are not make targets, and in the output of 'make -nsp',
> these are preceded by a line
>
> # Not a target:
>
> I think a (wrong) target after this line should be ignored.

The current code which uses _make-parseMakefile already offers those
targets anyway, so there is no change.

Even if _make-parseDataBase was smarter and ignored those non-targets,
configure.ac is a file and will still be completed by _files.

Personally I don't like the current design, which is why I don't use
the official make completion, and use my own instead [1]. I think only
the targets returned by make should be completed (no _files), and
anything with "# Not a target" be ignored. For that I use this script
in my version:

    awk -v RS= -F: -v PRX="$1" '!match($1, "^" PFX) { next } $1 ~
/^[^#%]+$/ { print $1 }'

This parses paragraph by paragraph, and since "# Not a target" is a
paragraph which starts with a comment, it's ignored. I'm fine
implementing something like that in _make-parseDataBase, but that
would be a new feature, and anyway I don't use that code.

I sent these patches not because I use them, but because while doing
tests I found these low hanging fruit.

I believe these can be applied as-is, with further improvements later
on (by somebody else). They are still an improvement from the current
situation by orders of magnitude: from several minutes to
milliseconds.

Cheers.

[1] https://github.com/felipec/dotfiles/blob/master/.zsh/_make

-- 
Felipe Contreras