PATCH: Improve quote handling in _ant completion

PATCH: Improve quote handling in _ant completion

[German Riano]

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

In the current completion, if a task description contains a quote (') the completions do not render properly
In this patch I propose to build the tmp array via
tmp=( ${(f@)"$(command)"} )
and print one line at a time. This way quotes are handled correctly
Also I am proposing to shield 'find_targets' with '(( $#functions[find_targets] )) ||' so that users can easily override the function.



Germán Riaño, Ph. D. http://GermanRiano.com Seattle, WA, USA

diff --git a/Completion/Unix/Command/_ant b/Completion/Unix/Command/_ant
index 36c7c0e89..c7bc7eb5f 100644
--- a/Completion/Unix/Command/_ant
+++ b/Completion/Unix/Command/_ant
@@ -6,6 +6,7 @@ local curcontext="$curcontext" state line expl ret=1
 typeset -A opt_args
 local buildfile classpath cp userjars importedfiles target='*:target:->target' targets tmp
 
+(( $#functions[find_targets] )) ||
 find_targets() {
     importedfiles=( $(sed -n "s/ *<import[^>]* file=[\"']\([^\"']*\)[\"'].*/\1/p" < $1) )
     # Tweaked to omit targets beginning with "-" that can't
@@ -103,8 +104,8 @@ case $state in
         # For the array to be set with correct argument boundaries, the entire
         # set statement needs to be eval'd.  On Cygwin, need to kill \r's output
         # from Java or parsing will fail.
-        eval set -A tmp "${$(_call_program targets "$words[1]" -buildfile $buildfile -projecthelp |
-          while read target desc
+        tmp=(${(f@)"$(_call_program targets "$words[1]" -buildfile $buildfile -projecthelp |
+          while read -r target desc
           do
             # This loop reads ant -projecthelp output from versions 1.3 to 1.6
             ln="${target}${desc:+:$desc}"
@@ -122,17 +123,18 @@ case $state in
                         read default_target junk
                     fi
                     # Output target again indicating its the default one.
-                    print -n "'${default_target}:(Default target) ' "
+                    print -- "${default_target}:(Default target)"
                 ;;
                 (Searching:*|Main:targets:|Subtargets:|BUILD:SUCCESSFUL|Total:time:*)
                 ;;
                 (*)
                     # Return target and description
-                    print -n "'$ln' "
+                    print -- "$ln"
                 ;;
             esac
           done
-        )//$'\015'}"
+        )"//$'\015'})
+
         _describe 'target' tmp && ret=0
       else
         targets=( $(find_targets $buildfile) )



[-- Attachment #2: Type: text/html, Size: 5406 bytes --]

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

On Tue, Nov 28, 2023 at 5:30 PM German Riano <griano@yahoo.com> wrote:
>
> In the current completion, if a task description contains a quote (') the completions do not render properly
>
> In this patch I propose to build the tmp array via
> tmp=( ${(f@)"$(command)"} )
> and print one line at a time.

Does this smaller change work instead?  (I suspect the "eval" could be
eliminated by tweaking this further.)

diff --git a/Completion/Unix/Command/_ant b/Completion/Unix/Command/_ant
index 36c7c0e89..aea3f51ec 100644
--- a/Completion/Unix/Command/_ant
+++ b/Completion/Unix/Command/_ant
@@ -122,7 +122,7 @@ case $state in
                         read default_target junk
                     fi
                     # Output target again indicating its the default one.
-                    print -n "'${default_target}:(Default target) ' "
+                    print -rn -- "${(qq):-${default_target}:(Default
target) } "
                 ;;

(Searching:*|Main:targets:|Subtargets:|BUILD:SUCCESSFUL|Total:time:*)
                 ;;

> Also I am proposing to shield 'find_targets' with '(( $#functions[find_targets] )) ||' so that users can easily override the function.

No particular objection to this, though I wonder if it should be tied
to the "targets" style and _call_program (Oliver?)

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

On Wed, Nov 29, 2023 at 11:18 AM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> +                    print -rn -- "${(qq):-${default_target}:(Default
> target) } "

Sigh.  Sorry about the gmail line break.

Re: PATCH: Improve quote handling in _ant completion

[Mikael Magnusson]

On 11/29/23, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Tue, Nov 28, 2023 at 5:30 PM German Riano <griano@yahoo.com> wrote:
>>
>> In the current completion, if a task description contains a quote (') the
>> completions do not render properly
>>
>> In this patch I propose to build the tmp array via
>> tmp=( ${(f@)"$(command)"} )
>> and print one line at a time.
>
> Does this smaller change work instead?  (I suspect the "eval" could be
> eliminated by tweaking this further.)
>
> diff --git a/Completion/Unix/Command/_ant b/Completion/Unix/Command/_ant
> index 36c7c0e89..aea3f51ec 100644
> --- a/Completion/Unix/Command/_ant
> +++ b/Completion/Unix/Command/_ant
> @@ -122,7 +122,7 @@ case $state in
>                          read default_target junk
>                      fi
>                      # Output target again indicating its the default one.
> -                    print -n "'${default_target}:(Default target) ' "
> +                    print -rn -- "${(qq):-${default_target}:(Default
> target) } "
>                  ;;
>
> (Searching:*|Main:targets:|Subtargets:|BUILD:SUCCESSFUL|Total:time:*)
>                  ;;
>
>> Also I am proposing to shield 'find_targets' with '((
>> $#functions[find_targets] )) ||' so that users can easily override the
>> function.
>
> No particular objection to this, though I wonder if it should be tied
> to the "targets" style and _call_program (Oliver?)

It should probably also be renamed to _ant_find_targets or something
similar, it's very unexpected that a completer pollutes the main
function namespace.

-- 
Mikael Magnusson

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

On Wed, Nov 29, 2023 at 11:56 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On 11/29/23, Bart Schaefer <schaefer@brasslantern.com> wrote:
> > -                    print -n "'${default_target}:(Default target) ' "
> > +                    print -rn -- "${(qq):-${default_target}:(Default target) } "

A similar change needs to be applied a bit later, I think:

                 (*)
                     # Return target and description
-                    print -n "'$ln' "
+                    print -rn -- "${(qq):-$ln} "
                 ;;

If this seems OK I'll prepare a proper patch.

> >> Also I am proposing to shield 'find_targets' with '((
> >> $#functions[find_targets] )) ||' so that users can easily override the
> >> function.
>
> It should probably also be renamed to _ant_find_targets or something

Agreed.

Re: PATCH: Improve quote handling in _ant completion

[German Riano]

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


I checked and this works. Even just using 'print -qn' works.

Questions:
1. Is "${(qq):-$ln} equivalent to "${(qq)ln} ?2. Does the read command needs '-r' ?3. Does using eval creates the risk of arbitrary execution of code?


    On Wednesday, November 29, 2023 at 03:45:08 PM PST, Bart Schaefer <schaefer@brasslantern.com> wrote:  
 
 On Wed, Nov 29, 2023 at 11:56 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On 11/29/23, Bart Schaefer <schaefer@brasslantern.com> wrote:
> > -                    print -n "'${default_target}:(Default target) ' "
> > +                    print -rn -- "${(qq):-${default_target}:(Default target) } "

A similar change needs to be applied a bit later, I think:

                (*)
                    # Return target and description
-                    print -n "'$ln' "
+                    print -rn -- "${(qq):-$ln} "
                ;;

If this seems OK I'll prepare a proper patch.

> >> Also I am proposing to shield 'find_targets' with '((
> >> $#functions[find_targets] )) ||' so that users can easily override the
> >> function.
>
> It should probably also be renamed to _ant_find_targets or something

Agreed.
  

[-- Attachment #2: Type: text/html, Size: 3128 bytes --]

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

On Wed, Nov 29, 2023 at 4:15 PM German Riano <griano@yahoo.com> wrote:
>
> Even just using 'print -qn' works.

Er, what?

% print -qn foo
print: bad option: -q

Do you mean "printf %q ..." ?

> Questions:
> 1. Is "${(qq):-$ln} equivalent to "${(qq)ln} ?

Yes.  I left it with the ":-" because in the other case there's a
trailing space inside the right brace (ultimately, inside the right
single-quote) and I thought one might be needed here as well.

> 2. Does the read command needs '-r' ?

I don't think so, the backslashes will already have been removed by
"eval" ... but perhaps if there's actually a literal backslash in the
target name, yes.

> 3. Does using eval creates the risk of arbitrary execution of code?

It does as written before any of these patches, because a target name
could embed matched pairs of single quotes and $(...) in such a way
that the eval would run the substitution.

I think fixing the inner-single-quoting via (qq) removes that
possibility, but it would still be better not to need the eval.

Re: PATCH: Improve quote handling in _ant completion

[German Riano]

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

Sorry, I mean '-r', not '-q'.  In other words, this seems to work: 'print -rn "'$ln' ". But (qq) looks better

 

    On Wednesday, November 29, 2023 at 04:52:48 PM PST, Bart Schaefer <schaefer@brasslantern.com> wrote:  
 
 On Wed, Nov 29, 2023 at 4:15 PM German Riano <griano@yahoo.com> wrote:
>
> Even just using 'print -qn' works.

Er, what?

% print -qn foo
print: bad option: -q

Do you mean "printf %q ..." ?

> Questions:
> 1. Is "${(qq):-$ln} equivalent to "${(qq)ln} ?

Yes.  I left it with the ":-" because in the other case there's a
trailing space inside the right brace (ultimately, inside the right
single-quote) and I thought one might be needed here as well.

> 2. Does the read command needs '-r' ?

I don't think so, the backslashes will already have been removed by
"eval" ... but perhaps if there's actually a literal backslash in the
target name, yes.

> 3. Does using eval creates the risk of arbitrary execution of code?

It does as written before any of these patches, because a target name
could embed matched pairs of single quotes and $(...) in such a way
that the eval would run the substitution.

I think fixing the inner-single-quoting via (qq) removes that
possibility, but it would still be better not to need the eval.
  

[-- Attachment #2: Type: text/html, Size: 2760 bytes --]

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

On Wed, Nov 29, 2023 at 5:28 PM German Riano <griano@yahoo.com> wrote:
>
> Sorry, I mean '-r', not '-q'.  In other words, this seems to work: 'print -rn "'$ln' ". But (qq) looks better

Is there a reason $ln could not have any single-quotes in it?

The other thing puzzling me a little, is why this line (original source)

  print -n "'${default_target}:(Default target) ' "

has a space both inside and outside the closing single quote?  Why are
both needed?  Whereas this

  print -n "'$ln' "

has a space only following the single quote.  Does $ln always already
end in a space?

I don't have and have never used ant.

Re: PATCH: Improve quote handling in _ant completion

[German Riano]

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


$ln could have quotes in the description. For example "Run tests but don't do fail"Those are not properly handled today, since eval will complain about unmatched quotes

About the current code:

   print -n "'${default_target}:(Default target) ' "
The extra space after the ')' is no needed. It causes no big harm, but it's not needed.


Germán Riaño, Ph. D. http://GermanRiano.com Seattle, WA, USA 

    On Wednesday, November 29, 2023 at 06:41:03 PM PST, Bart Schaefer <schaefer@brasslantern.com> wrote:  
 
 On Wed, Nov 29, 2023 at 5:28 PM German Riano <griano@yahoo.com> wrote:
>
> Sorry, I mean '-r', not '-q'.  In other words, this seems to work: 'print -rn "'$ln' ". But (qq) looks better

Is there a reason $ln could not have any single-quotes in it?

The other thing puzzling me a little, is why this line (original source)

  print -n "'${default_target}:(Default target) ' "

has a space both inside and outside the closing single quote?  Why are
both needed?  Whereas this

  print -n "'$ln' "

has a space only following the single quote.  Does $ln always already
end in a space?

I don't have and have never used ant.
  

[-- Attachment #2: Type: text/html, Size: 2824 bytes --]

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

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

On Fri, Dec 1, 2023 at 11:20 AM German Riano <griano@yahoo.com> wrote:
>
> $ln could have quotes in the description. For example "Run tests but don't do fail"
> [...]
>
>    print -n "'${default_target}:(Default target) ' "
>
> The extra space after the ')' is no needed. It causes no big harm, but it's not needed.

Apologies again for accidentally squirting this into an unrelated thread.

[-- Attachment #2: targets-_ant.txt --]
[-- Type: text/plain, Size: 856 bytes --]

diff --git a/Completion/Unix/Command/_ant b/Completion/Unix/Command/_ant
index 36c7c0e89..91428b854 100644
--- a/Completion/Unix/Command/_ant
+++ b/Completion/Unix/Command/_ant
@@ -122,13 +122,13 @@ case $state in
                         read default_target junk
                     fi
                     # Output target again indicating its the default one.
-                    print -n "'${default_target}:(Default target) ' "
+                    print -rn -- "${(qq):-${default_target}:(Default target)} "
                 ;;
                 (Searching:*|Main:targets:|Subtargets:|BUILD:SUCCESSFUL|Total:time:*)
                 ;;
                 (*)
                     # Return target and description
-                    print -n "'$ln' "
+                    print -rn -- "${(qq)ln} "
                 ;;
             esac
           done

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

On Wed, Nov 29, 2023 at 11:56 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> >> Also I am proposing to shield 'find_targets' with '((
> >> $#functions[find_targets] )) ||' so that users can easily override the
> >> function.
> >
> > No particular objection to this, though I wonder if it should be tied
> > to the "targets" style and _call_program (Oliver?)
>
> It should probably also be renamed to _ant_find_targets or something
> similar, it's very unexpected that a completer pollutes the main
> function namespace.

I've lost track of whether Oliver originally wrote this completer or
merely committed it, but I'm going to leave this bit unchanged for him
to weigh in.

Re: PATCH: Improve quote handling in _ant completion

[Oliver Kiddle]

Bart Schaefer wrote:
> On Wed, Nov 29, 2023 at 11:56 AM Mikael Magnusson <mikachu@gmail.com> wrote:
> >
> > >> Also I am proposing to shield 'find_targets' with '((
> > >> $#functions[find_targets] )) ||' so that users can easily override the
> > >> function.
> > >
> > > No particular objection to this, though I wonder if it should be tied
> > > to the "targets" style and _call_program (Oliver?)

You could use _call_program but given that the default in this case
involves two separate invocations of sed and a loop, allowing the
function to be overridden is probably easier for users. I'm not overly
fond of shielding functions that way so tend to only do it for functions
that someone is likely to want to replace. But no objection from me in
this instance. Note that it is more efficient to use (( $+functions[...
and not $#functions[..

It definitely would be best renamed with _ant_ as a prefix. I would just
call it _ant_targets and include the compadd line in the function.
Perhaps whoever wrote it wanted to use it outside of completion. It
could also be rewritten to use fewer subshells.

> > It should probably also be renamed to _ant_find_targets or something
> > similar, it's very unexpected that a completer pollutes the main
> > function namespace.
>
> I've lost track of whether Oliver originally wrote this completer or
> merely committed it, but I'm going to leave this bit unchanged for him
> to weigh in.

I had to consult the git history myself to get the answer to that. And
while I did originally write it, there has been considerable change
since. The original had no separate function for targets. And it's a
long time since I used ant so it's not a completer I've ever had much
reason to look back at. I agree with everything you've said in the
feedback for this patch and haven't replied before because I didn't have
anything useful to add.

Oliver

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

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

On Sun, Dec 3, 2023 at 1:08 PM Oliver Kiddle <opk@zsh.org> wrote:
>
> It definitely would be best renamed with _ant_ as a prefix. I would just
> call it _ant_targets and include the compadd line in the function.

That last is a little tricky because it calls itself recursively.

> Perhaps whoever wrote it wanted to use it outside of completion.

Heh.

> It could also be rewritten to use fewer subshells.

I'm not certain enough of what's going on in there to try to do that.
One subshell is just to protect the parent against a "cd", the others
probably tail-exec themselves except for the one that has an "echo
$file | sed".

Rewritten to avoid "sed" you mean?  The s/// are mostly pretty gnarly.

[-- Attachment #2: more-_ant.txt --]
[-- Type: text/plain, Size: 1550 bytes --]

diff --git a/Completion/Unix/Command/_ant b/Completion/Unix/Command/_ant
index 91428b854..e5bfe7cf9 100644
--- a/Completion/Unix/Command/_ant
+++ b/Completion/Unix/Command/_ant
@@ -6,7 +6,8 @@ local curcontext="$curcontext" state line expl ret=1
 typeset -A opt_args
 local buildfile classpath cp userjars importedfiles target='*:target:->target' targets tmp
 
-find_targets() {
+(( $+functions[_ant_targets] )) ||
+_ant_targets() {
     importedfiles=( $(sed -n "s/ *<import[^>]* file=[\"']\([^\"']*\)[\"'].*/\1/p" < $1) )
     # Tweaked to omit targets beginning with "-" that can't
     # be invoked from the command line; see zsh-workers/24129.
@@ -14,12 +15,12 @@ find_targets() {
     if (( $#importedfiles )) ; then
         ( cd $1:h
             for file in $importedfiles ; do
-                expanded=( $(echo $file | sed -n "s|\${ant.home}|$ANT_HOME|p") )
-                if [[ ! "bla$expanded" = "bla" ]]; then
+                expanded=${file//\${ant.home}/$ANT_HOME}
+                if [[ $expanded" != "$file" ]]; then
                     file=$expanded
                 fi
                 if [[ -f $file ]]; then
-                    find_targets $file
+                    _ant_targets $file
                 fi
         done )
     fi
@@ -135,7 +136,7 @@ case $state in
         )//$'\015'}"
         _describe 'target' tmp && ret=0
       else
-        targets=( $(find_targets $buildfile) )
+        targets=( $(_ant_targets $buildfile) )
         _wanted targets expl target compadd -a targets && ret=0
       fi
     else

Re: PATCH: Improve quote handling in _ant completion

[Bart Schaefer]

> Rewritten to avoid "sed" you mean?  The s/// are mostly pretty gnarly.

Missed an opening double-quote in one line there -- should be

+                if [[ "$expanded" != "$file" ]]; then