* [PATCH 0/3] completion: make: various improvements @ 2022-07-30 1:03 Felipe Contreras 2022-07-30 1:03 ` [PATCH 1/3] completion: make: don't build everything Felipe Contreras ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Felipe Contreras @ 2022-07-30 1:03 UTC (permalink / raw) To: zsh-workers; +Cc: Oliver Kiddle, Felipe Contreras Hi, 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 Now it takes less than 100ms. Felipe Contreras (3): completion: make: don't build everything completion: make: add a simpler parser completion: make: fix whitespaces Completion/Unix/Command/_make | 80 +++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 31 deletions(-) -- 2.37.1.225.gfa48d685d2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] completion: make: don't build everything 2022-07-30 1:03 [PATCH 0/3] completion: make: various improvements Felipe Contreras @ 2022-07-30 1:03 ` Felipe Contreras 2022-07-30 1:03 ` [PATCH 2/3] completion: make: add a simpler parser Felipe Contreras ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Felipe Contreras @ 2022-07-30 1:03 UTC (permalink / raw) To: zsh-workers; +Cc: Oliver Kiddle, Felipe Contreras, Daniel Hahler At least in GNU make 4.3 the -n option is *not* respected and --always-make builds everything. Instead use a fake .DEFAULT target the way bash-completion does. This essentially reverts 597acaab4 (44722: _make: use --always-make instead of .PHONY for GNU make, 2019-09-02). Cc: Daniel Hahler <git@thequod.de> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Completion/Unix/Command/_make | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make index ae91440f0..28c529a88 100644 --- a/Completion/Unix/Command/_make +++ b/Completion/Unix/Command/_make @@ -239,7 +239,7 @@ _make() { if [[ $is_gnu == gnu ]] then if zstyle -t ":completion:${curcontext}:targets" call-command; then - _make-parseMakefile < <(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" --always-make 2> /dev/null) + _make-parseMakefile < <(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" .DEFAULT 2> /dev/null) else _make-parseMakefile < $file fi -- 2.37.1.225.gfa48d685d2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] completion: make: add a simpler parser 2022-07-30 1:03 [PATCH 0/3] completion: make: various improvements Felipe Contreras 2022-07-30 1:03 ` [PATCH 1/3] completion: make: don't build everything Felipe Contreras @ 2022-07-30 1:03 ` Felipe Contreras 2022-07-30 1:03 ` [PATCH 3/3] completion: make: fix whitespaces Felipe Contreras 2022-08-02 10:42 ` [PATCH 0/3] completion: make: various improvements Jun T 3 siblings, 0 replies; 12+ messages in thread From: Felipe Contreras @ 2022-07-30 1:03 UTC (permalink / raw) To: zsh-workers; +Cc: Oliver Kiddle, Felipe Contreras The make program does all the heavy lifting, there's no need to use a full parser. In the git build system I get an improvement of more than 3 times (from 3.68 to 1.07 seconds). Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Completion/Unix/Command/_make | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make index 28c529a88..94747ae58 100644 --- a/Completion/Unix/Command/_make +++ b/Completion/Unix/Command/_make @@ -118,6 +118,24 @@ _make-parseMakefile () { done } +_make-parseDataBase () { + local input var TAB=$'\t' IFS= + + while read input + do + case "$input " in + ([[:alnum:]][[:alnum:]_]#[" "$TAB]#(\?|:|::|)=*) + var=${input%%[ $TAB]#(\?|:|::|)=*} + VARIABLES[$var]=1 + ;; + + ([[*?[:alnum:]$][^$TAB:=%]#:[^=]*) + TARGETS+=( ${input%%:*} ) + ;; + esac + done +} + _make() { local prev="$words[CURRENT-1]" file expl tmp is_gnu incl match basedir nul=$'\0' @@ -239,7 +257,7 @@ _make() { if [[ $is_gnu == gnu ]] then if zstyle -t ":completion:${curcontext}:targets" call-command; then - _make-parseMakefile < <(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" .DEFAULT 2> /dev/null) + _make-parseDataBase < <(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" .DEFAULT 2> /dev/null) else _make-parseMakefile < $file fi -- 2.37.1.225.gfa48d685d2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] completion: make: fix whitespaces 2022-07-30 1:03 [PATCH 0/3] completion: make: various improvements Felipe Contreras 2022-07-30 1:03 ` [PATCH 1/3] completion: make: don't build everything Felipe Contreras 2022-07-30 1:03 ` [PATCH 2/3] completion: make: add a simpler parser Felipe Contreras @ 2022-07-30 1:03 ` Felipe Contreras 2022-08-04 8:50 ` Jun T 2022-08-02 10:42 ` [PATCH 0/3] completion: make: various improvements Jun T 3 siblings, 1 reply; 12+ messages in thread From: Felipe Contreras @ 2022-07-30 1:03 UTC (permalink / raw) To: zsh-workers; +Cc: Oliver Kiddle, Felipe Contreras Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Completion/Unix/Command/_make | 60 +++++++++++++++++------------------ 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make index 94747ae58..0fd0b3270 100644 --- a/Completion/Unix/Command/_make +++ b/Completion/Unix/Command/_make @@ -12,35 +12,35 @@ _make-expandVars() { case $rest[2] in ($) # '$$'. may not appear in target and variable's value - front=$front\$\$ - rest=${rest[3,-1]} - continue - ;; + front=$front\$\$ + rest=${rest[3,-1]} + continue + ;; (\() # Variable of the form $(foobar) - open='(' - close=')' - ;; + open='(' + close=')' + ;; ({) # ${foobar} - open='{' - close='}' - ;; + open='{' + close='}' + ;; ([[:alpha:]]) # $foobar. This is exactly $(f)oobar. - open='' - close='' - var=$rest[2] - ;; + open='' + close='' + var=$rest[2] + ;; (*) # bad parameter name - print -- $front$rest - return 1 - ;; + print -- $front$rest + return 1 + ;; esac if [[ -n $open ]]; then if [[ $rest == \$$open(#b)([[:alnum:]_]##)(#B)$close* ]]; then - var=$match + var=$match else # unmatched () or {}, or bad parameter name - print -- $front$rest - return 1 + print -- $front$rest + return 1 fi fi @@ -49,17 +49,17 @@ _make-expandVars() { val=${VAR_ARGS[$var]} else if [[ -n $opt_args[(I)(-e|--environment-overrides)] ]]; then - if [[ $parameters[$var] == scalar-export* ]]; then - val=${(P)var} - elif [[ -n ${VARIABLES[(i)$var]} ]]; then - val=${VARIABLES[$var]} - fi + if [[ $parameters[$var] == scalar-export* ]]; then + val=${(P)var} + elif [[ -n ${VARIABLES[(i)$var]} ]]; then + val=${VARIABLES[$var]} + fi else - if [[ -n ${VARIABLES[(i)$var]} ]]; then - val=${VARIABLES[$var]} - elif [[ $parameters[$var] == scalar-export* ]]; then - val=${(P)var} - fi + if [[ -n ${VARIABLES[(i)$var]} ]]; then + val=${VARIABLES[$var]} + elif [[ $parameters[$var] == scalar-export* ]]; then + val=${(P)var} + fi fi fi rest=${rest//\$$open$var$close/$val} -- 2.37.1.225.gfa48d685d2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] completion: make: fix whitespaces 2022-07-30 1:03 ` [PATCH 3/3] completion: make: fix whitespaces Felipe Contreras @ 2022-08-04 8:50 ` Jun T 2022-08-04 15:57 ` Felipe Contreras 0 siblings, 1 reply; 12+ messages in thread From: Jun T @ 2022-08-04 8:50 UTC (permalink / raw) To: zsh-workers As this patch shows, _make has some indentation problems (and also lots of other scripts have them). > 2022/07/30 10:03, Felipe Contreras <felipe.contreras@gmail.com> write: > > diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make > @@ -12,35 +12,35 @@ _make-expandVars() { > > case $rest[2] in > ($) # '$$'. may not appear in target and variable's value > - front=$front\$\$ > - rest=${rest[3,-1]} > - continue > - ;; > + front=$front\$\$ > + rest=${rest[3,-1]} (snip) # I didn't know that indent_style for scripts has changed from # tab to space. I have a vague memory of argument that it's not good to fix indent of lines not related with the real fix because it makes 'git blame' rather useless, and then someone wrote that 'git blame -w' can be used in that case. So, in general, is it better (or not) to apply this type of patch? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] completion: make: fix whitespaces 2022-08-04 8:50 ` Jun T @ 2022-08-04 15:57 ` Felipe Contreras 0 siblings, 0 replies; 12+ messages in thread From: Felipe Contreras @ 2022-08-04 15:57 UTC (permalink / raw) To: Jun T; +Cc: zsh-workers On Thu, Aug 4, 2022 at 3:55 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: > > As this patch shows, _make has some indentation problems > (and also lots of other scripts have them). > > > 2022/07/30 10:03, Felipe Contreras <felipe.contreras@gmail.com> write: > > > > diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make > > > @@ -12,35 +12,35 @@ _make-expandVars() { > > > > case $rest[2] in > > ($) # '$$'. may not appear in target and variable's value > > - front=$front\$\$ > > - rest=${rest[3,-1]} > > - continue > > - ;; > > + front=$front\$\$ > > + rest=${rest[3,-1]} > (snip) > > # I didn't know that indent_style for scripts has changed from > # tab to space. > > I have a vague memory of argument that it's not good to fix indent of > lines not related with the real fix because it makes 'git blame' > rather useless, and then someone wrote that 'git blame -w' can be used > in that case. Even if -w didn't exist, `git blame` can find previous modifications, it's not just a tool to find the last one, so it wouldn't be "useless". You can tell it to start from the previous commit that made modifications: git blame 6ab65fae7a~ -L12,35 Completion/Unix/Command/_make And you can of course list *all* the previous modifications to those lines: git log -L12,35:Completion/Unix/Command/_make > So, in general, is it better (or not) to apply this type of patch? I've sent many patches like this one to the git project itself, so presumably git has no trouble with them. Ultimately it's up to you (the developers). Do you want to have inconsistent whitespaces forever (or at least until somebody modifies those lines, which seems to happen every 5 years)? Or would you rather have it clean now? Personally, sometimes I use ts=2 in shell scripts, so it's very annoying to see those badly indented lines, but your mileage may vary. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] completion: make: various improvements 2022-07-30 1:03 [PATCH 0/3] completion: make: various improvements Felipe Contreras ` (2 preceding siblings ...) 2022-07-30 1:03 ` [PATCH 3/3] completion: make: fix whitespaces Felipe Contreras @ 2022-08-02 10:42 ` Jun T 2022-08-02 23:02 ` Felipe Contreras 3 siblings, 1 reply; 12+ messages in thread From: Jun T @ 2022-08-02 10:42 UTC (permalink / raw) To: zsh-workers > 2022/07/30 10:03, Felipe Contreras <felipe.contreras@gmail.com> 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 Thanks. Could you please update [PATCH 2/3]? (The comment for [PATCH 1/3] is just a note and can be ignored.) [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. [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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] completion: make: various improvements 2022-08-02 10:42 ` [PATCH 0/3] completion: make: various improvements Jun T @ 2022-08-02 23:02 ` Felipe Contreras 2022-08-03 8:48 ` Jun T 0 siblings, 1 reply; 12+ messages in thread From: Felipe Contreras @ 2022-08-02 23:02 UTC (permalink / raw) To: Jun T; +Cc: zsh-workers On Tue, Aug 2, 2022 at 5:43 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: > > > 2022/07/30 10:03, Felipe Contreras <felipe.contreras@gmail.com> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] completion: make: various improvements 2022-08-02 23:02 ` Felipe Contreras @ 2022-08-03 8:48 ` Jun T 2022-08-03 14:36 ` Felipe Contreras 0 siblings, 1 reply; 12+ messages in thread From: Jun T @ 2022-08-03 8:48 UTC (permalink / raw) To: zsh-workers > 2022/08/03 8:02, Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Tue, Aug 2, 2022 at 5:43 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: >> # 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. If 'call-command' style is set to false, then these targets are not offered (as a possible target). > 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), You may try the following: zstyle ':completion:*:make:*:' tag-order 'targets variables' ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] completion: make: various improvements 2022-08-03 8:48 ` Jun T @ 2022-08-03 14:36 ` Felipe Contreras 2022-08-04 9:33 ` Jun T 0 siblings, 1 reply; 12+ messages in thread From: Felipe Contreras @ 2022-08-03 14:36 UTC (permalink / raw) To: Jun T; +Cc: zsh-workers On Wed, Aug 3, 2022 at 3:54 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: > > > 2022/08/03 8:02, Felipe Contreras <felipe.contreras@gmail.com> wrote: > > > > On Tue, Aug 2, 2022 at 5:43 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: > > >> # 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. > > If 'call-command' style is set to false, then these targets are not > offered (as a possible target). But if you set it to true they are. We are talking about the *current code*. The current code already has this behavior. > > 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), > > You may try the following: > > zstyle ':completion:*:make:*:' tag-order 'targets variables' Sure, in that case those files won't be listed initially, but they still will be completed. -- Felipe Contreras ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] completion: make: various improvements 2022-08-03 14:36 ` Felipe Contreras @ 2022-08-04 9:33 ` Jun T 2022-08-04 17:03 ` Felipe Contreras 0 siblings, 1 reply; 12+ messages in thread From: Jun T @ 2022-08-04 9:33 UTC (permalink / raw) To: zsh-workers As pointed out by Felipe Contreras, the biggest problem with the current _make (with call-cammand on) is that it actually runs make to build all. This is serious and need be fixed. I will soon push slightly modified patch (see the end of this post) unless any problem is found. > 2022/08/03 23:36, Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Wed, Aug 3, 2022 at 3:54 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: >> >> If 'call-command' style is set to false, then these targets are not >> offered (as a possible target). > > But if you set it to true they are. > > We are talking about the *current code*. The current code already has > this behavior. # I was thinking we were talking about how we can improve the # current code. Without your patch, if call-command is on, 'make <TAB>' at the top level of the zsh source tree gives, after a very long wait, huge number of possible targets (a few hundreds or more?), but most of them are not valid targets. With your patch it immediately gives about 60 candidates. (Most of them are valid, but they still include configure.ac.) So I thought (probably mistakenly) that part of the objectives of the patch was to offer only valid targets. But anyway, I think it is better to filter out invalid targets if it is easy to do so. >> zstyle ':completion:*:make:*:' tag-order 'targets variables' > > Sure, in that case those files won't be listed initially, but they > still will be completed. I think files are completed only if there is no matching targets. This is enough for me, but maybe not for everyone. The patch below is basically [1/3]+[2/3], with a few additions: (1) In _make-parseDataBase(), filter out targets following the line '# Not a target', and variables following '# environment'. I think not offering environment variables is OK, but not sure everyone agrees with this. (2) Use make option -q instead of -s (as in your awk-version). (3) add options -C and -S to _arguments. (4) Do not call _make-parse{Makefile,DataBase}() when completing options for 'make -<TAB>'. # As you write, this is much slower than your awk-version. diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make index ae91440f0..510368e8b 100644 --- a/Completion/Unix/Command/_make +++ b/Completion/Unix/Command/_make @@ -118,10 +118,34 @@ _make-parseMakefile () { done } +_make-parseDataBase () { + local input var TAB=$'\t' IFS= skip=0 + + while read input + do + if [[ $skip = 1 ]]; then + skip=0 + continue + fi + case "$input " in + (\# Not a target*|\# environment*) + skip=1 # skip next line + ;; + ([[:alnum:]][[:alnum:]_]#[" "$TAB]#(\?|:|::|)=*) + var=${input%%[ $TAB]#(\?|:|::|)=*} + VARIABLES[$var]=1 + ;; + ([[*?[:alnum:]$][^$TAB:=%]#:[^=]*) + TARGETS+=( ${input%%:*} ) + ;; + esac + done +} + _make() { local prev="$words[CURRENT-1]" file expl tmp is_gnu incl match basedir nul=$'\0' - local context state state_descr line + local curcontext=$curcontext state state_descr line local -a option_specs local -A VARIABLES VAR_ARGS opt_args local -aU TARGETS keys @@ -185,7 +209,7 @@ _make() { ) fi - _arguments -s $option_specs \ + _arguments -C -S -s $option_specs \ '*:make target:->target' && ret=0 [[ $state = cdir ]] && cdir=-2 @@ -214,6 +238,10 @@ _make() { ;; (target) + # target name starting with '-' is allowed only after '--' + if [[ $words[CURRENT] = -* ]] && (( ${words[(i)--]} > CURRENT )); then + return ret + fi file=${(v)opt_args[(I)(-f|--file|--makefile)]} if [[ -n $file ]] then @@ -239,7 +267,7 @@ _make() { if [[ $is_gnu == gnu ]] then if zstyle -t ":completion:${curcontext}:targets" call-command; then - _make-parseMakefile < <(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" --always-make 2> /dev/null) + _make-parseDataBase < <(_call_program targets "$words[1]" -nqp --no-print-directory -f "$file" .DEFAULT 2> /dev/null) else _make-parseMakefile < $file fi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] completion: make: various improvements 2022-08-04 9:33 ` Jun T @ 2022-08-04 17:03 ` Felipe Contreras 0 siblings, 0 replies; 12+ messages in thread From: Felipe Contreras @ 2022-08-04 17:03 UTC (permalink / raw) To: Jun T; +Cc: zsh-workers On Thu, Aug 4, 2022 at 4:33 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: > > 2022/08/03 23:36, Felipe Contreras <felipe.contreras@gmail.com> wrote: > > > > On Wed, Aug 3, 2022 at 3:54 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: > >> > >> If 'call-command' style is set to false, then these targets are not > >> offered (as a possible target). > > > > But if you set it to true they are. > > > > We are talking about the *current code*. The current code already has > > this behavior. > > # I was thinking we were talking about how we can improve the > # current code. Code can be improved in any number of ways. One way is to make small incremental changes that minimize the potential for regressions in logically independent patches. Doing all the improvements you can make in one go doesn't minimize the potential for regressions. > Without your patch, if call-command is on, 'make <TAB>' at the top level > of the zsh source tree gives, after a very long wait, huge number of > possible targets (a few hundreds or more?), but most of them are not > valid targets. > > With your patch it immediately gives about 60 candidates. > (Most of them are valid, but they still include configure.ac.) > > So I thought (probably mistakenly) that part of the objectives of > the patch was to offer only valid targets. But anyway, I think it is > better to filter out invalid targets if it is easy to do so. The objective of the patch is to improve the current situation to the point where typing 'make <tab>' is actually usable. Once this has been done in step 1, improving the list of targets can be done in step 2. There's no need to try to do two things at the same time, at least not in the same patch. > The patch below is basically [1/3]+[2/3], with a few additions: > (2) Use make option -q instead of -s (as in your awk-version). For the record, I use -q because that's what bash-completion uses: -npq. I do not mind any of these changes, but I would have done them on top of my changes. I would rebase my changes and include yours, but IIRC you guys squash all the commits anyway. Either way, I think anything is better than the current situation. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-08-04 17:09 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-30 1:03 [PATCH 0/3] completion: make: various improvements Felipe Contreras 2022-07-30 1:03 ` [PATCH 1/3] completion: make: don't build everything Felipe Contreras 2022-07-30 1:03 ` [PATCH 2/3] completion: make: add a simpler parser Felipe Contreras 2022-07-30 1:03 ` [PATCH 3/3] completion: make: fix whitespaces Felipe Contreras 2022-08-04 8:50 ` Jun T 2022-08-04 15:57 ` Felipe Contreras 2022-08-02 10:42 ` [PATCH 0/3] completion: make: various improvements Jun T 2022-08-02 23:02 ` Felipe Contreras 2022-08-03 8:48 ` Jun T 2022-08-03 14:36 ` Felipe Contreras 2022-08-04 9:33 ` Jun T 2022-08-04 17:03 ` Felipe Contreras
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).