zsh-workers
 help / color / mirror / code / Atom feed
* [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 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 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 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 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-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 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).