Broken specialsed file completion

Broken specialsed file completion

[Peter Stephenson]

For some reason the following, which has worked fine for many years, has
stopped working.

zle -C most-recent-file menu-complete _generic
zstyle ':completion:most-recent-file:*' completer _menu _path_files _match
zstyle ':completion:most-recent-file:*' file-sort modification
zstyle ':completion:most-recent-file:*' hidden all
bindkey '^X.' most-recent-file

Instead of completing most recently modified files, it reports

_path_files:compfiles:466: too few arguments

pws

Re: Broken specialsed file completion

[Frank Terbeck]

Hi,

Peter Stephenson wrote:
> For some reason the following, which has worked fine for many years, has
> stopped working.
>
> zle -C most-recent-file menu-complete _generic
> zstyle ':completion:most-recent-file:*' completer _menu _path_files _match
> zstyle ':completion:most-recent-file:*' file-sort modification
> zstyle ':completion:most-recent-file:*' hidden all
> bindkey '^X.' most-recent-file
>
> Instead of completing most recently modified files, it reports
>
> _path_files:compfiles:466: too few arguments


I had a bit of time on my hands, so I bisected this down to this commit:

    f7c3aa170b8c36b146ba7644cf3912cb1329a5d2 is the first bad commit
    commit f7c3aa170b8c36b146ba7644cf3912cb1329a5d2
    Author: Barton E. Schaefer <schaefer@zsh.org>
    Date:   Wed Aug 10 18:33:04 2016 -0700

    39019 (cf. PWS 39013): fix SHWORDSPLIT regression introduced by workers/29313

    Also add test cases for more join/split combinations


There is one hunk that changes code in that diff:

diff --git a/Src/subst.c b/Src/subst.c
index e3af156..ae3e4c4 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -3454,13 +3454,22 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
      * exception is that ${name:-word} and ${name:+word} will have already
      * done any requested splitting of the word value with quoting preserved.
      */
-    if (ssub || (spbreak && isarr >= 0) || spsep || sep) {
+    if (ssub || spbreak || spsep || sep) {
+       int force_split = !ssub && (spbreak || spsep);
        if (isarr) {
-           val = sepjoin(aval, sep, 1);
-           isarr = 0;
-           ms_flags = 0;
+           if (nojoin == 0) {
+               val = sepjoin(aval, sep, 1);
+               isarr = 0;
+               ms_flags = 0;
+           } else if (force_split && nojoin == 2) {
+               /* Hack to simulate splitting individual elements:
+                * first join on what we later use to split
+                */
+               val = sepjoin(aval, spsep, 1);
+               isarr = 0;
+           }
        }
-       if (!ssub && (spbreak || spsep)) {
+       if (force_split && !isarr) {
            aval = sepsplit(val, spsep, 0, 1);
            if (!aval || !aval[0])
                val = dupstring("");


Maybe that is useful to someone who knows that code.


Regards, Frank

Re: Broken specialsed file completion

[Bart Schaefer]

On Aug 29, 12:56am, Frank Terbeck wrote:
}
} > For some reason the following, which has worked fine for many years, has
} > stopped working.
} >
} > Instead of completing most recently modified files, it reports
} >
} > _path_files:compfiles:466: too few arguments
} 
} I had a bit of time on my hands, so I bisected this down to this commit:
} 
}     39019 (cf. PWS 39013): fix SHWORDSPLIT regression introduced by workers/29313

The difference is this expression (_path_files line 59):

sopt="-${(@j::M)${(@)tmp1#-}#?}"

Previously this would result in sopt='-' but now it appears to yield
sopt='' -- which doesn't seem to make any sense, because the "-" is
outside of the parameter expansion.  However, it's affected by the
setting of rcexpandparam in $_comp_options.

The expansion of ${(@j::M)${(@)tmp1#-}#?} used to result in a scalar
empty string, but now it results in an empty array, and rcexpandparam
discards the "-" because it has no array element with which to pair.

What's happened here is that given (@j::), the @ is forcing an array,
where previously the j would force a string.

I'm still not entirely sure what it should mean to use @ and j in the
same expansion, but clearly it's wrong for the j to be ignored.  Does
the D04 test case below look correct to everyone?


diff --git a/Src/subst.c b/Src/subst.c
index 15eb59b..4641b4b 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -3458,7 +3458,8 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
     if (ssub || spbreak || spsep || sep) {
 	int force_split = !ssub && (spbreak || spsep);
 	if (isarr) {
-	    if (nojoin == 0) {
+	    /* sep non-null here means F or j flag, force join */
+	    if (nojoin == 0 || sep) {
 		val = sepjoin(aval, sep, 1);
 		isarr = 0;
 		ms_flags = 0;
@@ -3467,7 +3468,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 		 * forced joining as previously determined, or
 		 * join on what we later use to forcibly split
 		 */
-		val = sepjoin(aval, (nojoin == 1 ? sep : spsep), 1);
+		val = sepjoin(aval, (nojoin == 1 ? NULL : spsep), 1);
 		isarr = 0;
 	    }
 	}
diff --git a/Test/D04parameter.ztst b/Test/D04parameter.ztst
index 37166fa..0630079 100644
--- a/Test/D04parameter.ztst
+++ b/Test/D04parameter.ztst
@@ -1986,3 +1986,12 @@
 >one
 >two-bucklemy
 >shoe
+
+  (
+  set -- "one two" "bucklemy shoe"
+  IFS=
+  setopt shwordsplit rcexpandparam
+  print -l "X${(@j.-.)*}"
+  )
+0:Use of @ does not prevent forced join with j
+>Xone two-bucklemy shoe

Re: Broken specialsed file completion

[Peter Stephenson]

On Sun, 28 Aug 2016 19:01:58 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> The difference is this expression (_path_files line 59):
> 
> sopt="-${(@j::M)${(@)tmp1#-}#?}"
> 
> Previously this would result in sopt='-' but now it appears to yield
> sopt='' -- which doesn't seem to make any sense, because the "-" is
> outside of the parameter expansion.  However, it's affected by the
> setting of rcexpandparam in $_comp_options.
> 
> The expansion of ${(@j::M)${(@)tmp1#-}#?} used to result in a scalar
> empty string, but now it results in an empty array, and rcexpandparam
> discards the "-" because it has no array element with which to pair.
> 
> What's happened here is that given (@j::), the @ is forcing an array,
> where previously the j would force a string.
> 
> I'm still not entirely sure what it should mean to use @ and j in the
> same ex/pansion, but clearly it's wrong for the j to be ignored.

It certainly seems right @ shouldn't be preventing joining.  There's
also the question of whether it should force splitting later.
Traditionally it hasn't, and I think that's correct, too.  However, the
latter isn't really relevant here, I suppose, as there'd be no split so
the empty expression would stay a scalar if already joined.

I'm not sure what the @ should be doing there, either, given that
joining happens before any splitting, and hence also before considering
multiple elements.  I suspect it's got something to do with affecting
the way the matching happens, but I'm not at all sure it actually does
that.

The use of it here is opaque enough that rewriting that expansion might
be a good idea anyway; it's relying on an obscurely different set of
behaviours if it is or isn't presented with an array.  But how to
rewrite it is another question.

> Does the D04 test case below look correct to everyone?
> +
> +  (
> +  set -- "one two" "bucklemy shoe"
> +  IFS=
> +  setopt shwordsplit rcexpandparam
> +  print -l "X${(@j.-.)*}"
> +  )
> +0:Use of @ does not prevent forced join with j
> +>Xone two-bucklemy shoe

Yes, I think that's right.

Thanks for working this out.
pws