PATCH: following -C option in make completion

PATCH: following -C option in make completion

[Oliver Kiddle]

In the make completion, there's some very old logic for picking out the
argument to -C. _arguments makes this rather easier if we just look in
$opt_args and has the advantage that we can handle -Cdir and --directory
forms (not just -C dir).

This patch also uses the computed $basedir value for our view of the GNU
make $(CURDIR) macro so it will handle include files referenced relative
to $(CURDIR).

Previously $basedir was forced into absolute form which I don't think
gains us anything - _files -W doesn't care. Neither does finding files
to include. But perhaps this breaks things for some form or another.

I'm also not sure what the (q) modifier achieved. I've used
${(Q)~opt_args... so it'll expand usernames and remove a level of
quoting. opt_args also does some extra quoting for colons which could
also be removed. We could do with some sort of safe-eval for where
completion functions make use of bits of the command line - expanding
variables and named directories is useful but command-substitutions not
so.

Oliver

diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make
index 3dcf479c3..06971f07a 100644
--- a/Completion/Unix/Command/_make
+++ b/Completion/Unix/Command/_make
@@ -118,35 +118,9 @@ _make-parseMakefile () {
   done
 }
 
-_make-findBasedir () {
-  local file index basedir
-  basedir=$PWD
-  for (( index=0; index < $#@; index++ ))
-  do
-    if [[ $@[index] == -C ]]
-    then
-      file=${~@[index+1]} 2>/dev/null
-      if [[ -z $file ]]
-      then
-        # make returns with an error if an empty arg is given
-        # even if the concatenated path is a valid directory
-        return
-      elif [[ $file == /* ]]
-      then
-        # Absolute path, replace base directory
-        basedir=$file
-      else
-        # Relative, concatenate path
-        basedir=$basedir/$file
-      fi
-    fi
-  done
-  print -- $basedir
-}
-
 _make() {
 
-  local prev="$words[CURRENT-1]" file expl tmp is_gnu incl match
+  local prev="$words[CURRENT-1]" file expl tmp is_gnu incl match basedir
   local context state state_descr line
   local -a option_specs
   local -A VARIABLES VAR_ARGS opt_args
@@ -214,15 +188,18 @@ _make() {
   _arguments -s $option_specs \
     '*:make target:->target' && ret=0
 
+  basedir=${(Q)~opt_args[-C]:-${opt_args[--directory]}}
+  VAR_ARGS[CURDIR]="${basedir:=$PWD}"
+
   case $state in
     (dir)
     _description directories expl "$state_descr"
-    _files "$expl[@]" -W ${(q)$(_make-findBasedir ${words[1,CURRENT-1]})} -/ && ret=0
+    _files "$expl[@]" -W $basedir -/ && ret=0
     ;;
 
     (file)
     _description files expl "$state_descr"
-    _files "$expl[@]" -W ${(q)$(_make-findBasedir $words)} && ret=0
+    _files "$expl[@]" -W $basedir && ret=0
     ;;
 
     (debug)
@@ -239,11 +216,9 @@ _make() {
     file=${(v)opt_args[(I)(-f|--file|--makefile)]}
     if [[ -n $file ]]
     then
-      [[ $file == [^/]* ]] && file=${(q)$(_make-findBasedir $words)}/$file
+      [[ $file == [^/]* ]] && file=$basedir/$file
       [[ -r $file ]] || file=
     else
-      local basedir
-      basedir=${$(_make-findBasedir $words)}
       if [[ $is_gnu == gnu && -r $basedir/GNUmakefile ]]
       then
         file=$basedir/GNUmakefile
@@ -287,7 +262,7 @@ _make() {
       _alternative \
         'targets:make target:compadd -Q -a TARGETS' \
         'variables:make variable:compadd -S = -F keys -k VARIABLES' \
-        '*:file:_files' && ret=0
+        '*:file:_files -W $basedir' && ret=0
     fi
   esac
 

Re: PATCH: following -C option in make completion

[Daniel Shahaf]

Good morning Oliver,

Oliver Kiddle wrote on Wed, Nov 06, 2019 at 22:51:09 +0100:
> In the make completion, there's some very old logic for picking out the
> argument to -C. _arguments makes this rather easier if we just look in
> $opt_args and has the advantage that we can handle -Cdir and --directory
> forms (not just -C dir).
> 
> This patch also uses the computed $basedir value for our view of the GNU
> make $(CURDIR) macro so it will handle include files referenced relative
> to $(CURDIR).
> 
> Previously $basedir was forced into absolute form which I don't think
> gains us anything - _files -W doesn't care. Neither does finding files
> to include. But perhaps this breaks things for some form or another.
> 
> I'm also not sure what the (q) modifier achieved. I've used
> ${(Q)~opt_args... so it'll expand usernames and remove a level of
> quoting. opt_args also does some extra quoting for colons which could
> also be removed. We could do with some sort of safe-eval for where
> completion functions make use of bits of the command line - expanding
> variables and named directories is useful but command-substitutions not
> so.

I've ran into a regression, and reverting this patch fixes it.

Using current master, and the following setup:

    % cd "$(mktemp -d)"
    % mkdir foo

The following two work:

    % make -C <TAB>
    [offers foo]

    % make -C ./fo<TAB>
    [expands to ./foo]

But the following does not:

    % make -C fo<TAB>
    [no matches]

It does work if I revert this patch.

WDYT?

Cheers,

Daniel

Re: PATCH: following -C option in make completion

[Oliver Kiddle]

Daniel Shahaf wrote:
>     % make -C fo<TAB>
>     [no matches]
>
> It does work if I revert this patch.

The easy quick fix would be to use _directories for -C. That's only
wrong if multiple -C options are used which is obscure enough that
it didn't occur to me when I wrote the patch. The state is used for
-C's argument itself because of that GNU specific feature of allowing
-C to be repeatable. But after make -C <tab>, $opt_args[-C] is set
to the empty string for the current -C option. Hence the breakage you
observed.

I think it is preferable to use $opt_args over scanning $words as
in the original code. That'll only be wrong if the user mixes -C
and --directory (the old code ignored --directory completely so I
don't feel bad about that).

The following patch changes the basedir assignment. The new expansion is
especially gnarly so I'd appreciate if you could give it some testing.
It has to:
  - (temporarily) convert quoted colons to nulls
  - remove one level of shell quoting
  - prepend $PWD: so the current directory is the default and to
    workaround problems with splitting giving a string not an array when
    there's no separator at all
  - split on colons and use (@) and double quotes to avoid losing a
    final empty element
  - expand named directories for each element
  - throw away initial elements if a later one is absolute
  - drop the last element for -C (but not -I)
  - turn nulls back into colons
  - join elements with /

Oliver

diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make
index 06971f07a..21ed56184 100644
--- a/Completion/Unix/Command/_make
+++ b/Completion/Unix/Command/_make
@@ -120,12 +120,12 @@ _make-parseMakefile () {
 
 _make() {
 
-  local prev="$words[CURRENT-1]" file expl tmp is_gnu incl match basedir
+  local prev="$words[CURRENT-1]" file expl tmp is_gnu incl match basedir nul=$'\0'
   local context state state_descr line
   local -a option_specs
   local -A VARIABLES VAR_ARGS opt_args
   local -aU TARGETS keys
-  local ret=1
+  local -i cdir=-1 ret=1
 
   # VAR=VAL on the current command line
   for tmp in $words; do
@@ -142,7 +142,7 @@ _make() {
     incl="(-|)include"
     option_specs=(
       '(-B --always-make)'{-B,--always-make}'[unconditionally make all targets]'
-      '*'{-C,--directory=}'[change directory first]:change to directory:->dir'
+      '*'{-C,--directory=}'[change directory first]:change to directory:->cdir'
       '-d[print lots of debug information]'
       '--debug=-[print various types of debug information]:debug options:->debug'
       '(-e --environment-overrides)'{-e,--environment-overrides}'[environment variables override makefiles]'
@@ -177,7 +177,7 @@ _make() {
     # Basic make options only.
     incl=.include
     option_specs=(
-      '-C[change directory first]:directory:->dir'
+      '-C[change directory first]:directory:->cdir'
       '-I[include directory for makefiles]:directory:->dir'
       '-f[specify makefile]:makefile:->file'
       '-o[specify file not to remake]:file not to remake:->file'
@@ -188,11 +188,12 @@ _make() {
   _arguments -s $option_specs \
     '*:make target:->target' && ret=0
 
-  basedir=${(Q)~opt_args[-C]:-${opt_args[--directory]}}
-  VAR_ARGS[CURDIR]="${basedir:=$PWD}"
+  [[ $state = cdir ]] && cdir=-2
+  basedir=${(j./.)${${~"${(@s.:.):-$PWD:${(Q)${opt_args[-C]:-$opt_args[--directory]}//\\:/$nul}}"}[(R)/*,cdir]}//$nul/:}
+  VAR_ARGS[CURDIR]="${basedir}"
 
   case $state in
-    (dir)
+    (*dir)
     _description directories expl "$state_descr"
     _files "$expl[@]" -W $basedir -/ && ret=0
     ;;

Re: PATCH: following -C option in make completion

[Daniel Shahaf]

Oliver Kiddle wrote on Thu, Jan 02, 2020 at 19:00:41 +0100:
> The following patch changes the basedir assignment. The new expansion is
> especially gnarly so I'd appreciate if you could give it some testing.

I've tested it and couldn't break it.  Thanks for the patch.