Regression affecting completion with NO_CASE_GLOB

Regression affecting completion with NO_CASE_GLOB

[John Heatherington]

After bisecting, I found a regression originating in the commit:
d82604843bf2b743e04666d4644dd109831252f7

This was merged after https://zsh.org/workers/49661

The off-by-one fix appears to have traded one for another. I have a
folder structure on my machine similar to the following:

~/Projects
├── Private
├── Public
└── QT

It can be simplified further for the purposes of testing:

/tmp/zsh_test
├── a
└── b

'a' and 'b' can be files or folders; I noticed with folders when
attempting to change directory. The key is that the names need to be
one character in separation, and CASE_GLOB must be turned off. Steps
are as follows:

$ zsh -f
$ setopt NO_CASE_GLOB
$ cd /tmp/zsh_test     (press tab to trigger completion)
$ cd /tmp/zsh_test/B   (press tab again)
$ cd /tmp/zsh_test/b/

I would expect the normal completion behavior to be:

$ cd /tmp/zsh_test/
a/  b/

It looks like the change has caused case-insensitive, adjacent letters
to be considered equivalent, so it goes ahead and starts to fill it in
when completing a command. I've included a partial trace at the bottom
for reference.

I tried taking a look at the code, but I'm very unfamiliar with how
things work, conceptually, so I thought I'd check here.

In the old version, bld_line relies on (in the case of an equivalence)
pattern_match1 returning (1 + the index), which would be later
decremented by one in pattern_match_equivalence. This makes sense to
me, since the return value of pattern_match1 is also being used to
indicate success/failure for an equivalence class.

The change was originally introduced to correct cfp_matcher_range,
which doesn't call bld_line, so it seems like this could be fixed by
putting the burden of adjusting the index on bld_line instead. I also
noticed pattern_match_restrict uses a similar pattern, so it may need
the same adjustment. Any thoughts?

---

Breakpoint 1, pattern_match_equivalence (lp=0x5555557eff40, wind=27,
wmtp=21, wchr=97) at compmatch.c:1322
1322        if (!PATMATCHINDEX(lp->u.str, wind, &lchr, &lmtp)) {
(gdb) bt 15
#0  pattern_match_equivalence (lp=0x5555557eff40, wind=27, wmtp=21,
wchr=97) at compmatch.c:1322
#1  0x000055555566626b in bld_line (mp=0x55555587cee0,
line=0x7ffff7fbdfd8 L"", mword=0x7ffff7fbd941 "", word=0x7ffff7fbdc99
"", wlen=1, sfx=0) at compmatch.c:1805
#2  0x00005555556672f1 in join_sub (md=0x7ffffffe9ef0,
str=0x7ffff7fbd940 "a", len=1, mlen=0x7ffffffe9ea0, sfx=0, join=1) at
compmatch.c:2267
#3  0x0000555555667b96 in join_psfx (ot=0x7ffff7fbd948,
nt=0x7ffff7fbdca0, orest=0x0, nrest=0x0, sfx=0) at compmatch.c:2509
#4  0x0000555555669187 in join_clines (o=0x7ffff7fbd948,
n=0x7ffff7fbdca0) at compmatch.c:2952
#5  0x0000555555660081 in add_match_data (alt=0, str=0x7ffff7fbdd50
"b", orig=0x55555587cca0 "b", line=0x7ffff7fbdd58, ipre=0x7ffff7fbd7d8
"", ripre=0x0, isuf=0x7ffff7fbd7e0 "", pre=0x0, prpre=0x7ffff7fbd920
"/tmp/zsh_test/",
    ppre=0x7ffff7fbd800 "/tmp/zsh_test/", pline=0x7ffff7fbd490,
psuf=0x7ffff7fbd810 "", sline=0x0, suf=0x0, flags=1, exact=0) at
compcore.c:2997
#6  0x000055555565e194 in addmatches (dat=0x7ffffffea6f0,
argv=0x555555878038) at compcore.c:2550
#7  0x0000555555654952 in bin_compadd (name=0x7ffff7f93ee8 "compadd",
argv=0x7ffff7e2cd78, ops=0x7ffffffea870, func=0) at complete.c:846
#8  0x000055555556edaf in execbuiltin (args=0x7ffff7f93fc0,
assigns=0x0, bn=0x5555556f5e20 <bintab>) at builtin.c:506
#9  0x000055555559e4fd in execcmd_exec (state=0x7ffffffedd00,
eparams=0x7ffffffeadb0, input=0, output=0, how=18, last1=2,
close_if_forked=-1) at exec.c:4148
#10 0x000055555559770f in execpline2 (state=0x7ffffffedd00,
pcode=45955, how=18, input=0, output=0, last1=0) at exec.c:1960
#11 0x0000555555595e2a in execpline (state=0x7ffffffedd00,
slcode=17410, how=18, last1=0) at exec.c:1689
#12 0x0000555555594ebe in execlist (state=0x7ffffffedd00,
dont_change_job=1, exiting=0) at exec.c:1444
#13 0x00005555555d446a in execif (state=0x7ffffffedd00, do_exec=0) at loop.c:582
#14 0x000055555559daf0 in execcmd_exec (state=0x7ffffffedd00,
eparams=0x7ffffffeb640, input=0, output=0, how=18, last1=2,
close_if_forked=-1) at exec.c:3968
(More stack frames follow...)

Re: Regression affecting completion with NO_CASE_GLOB

[Bart Schaefer]

On Thu, Jun 2, 2022 at 5:28 PM John Heatherington
<jheatherington@gmail.com> wrote:
>
> After bisecting, I found a regression originating in the commit:
> d82604843bf2b743e04666d4644dd109831252f7
>
[...]
>
> The change was originally introduced to correct cfp_matcher_range,
> which doesn't call bld_line, so it seems like this could be fixed by
> putting the burden of adjusting the index on bld_line instead.

Indeed, it makes sense that if the semantics of an argument to
pattern_match_equivalence() has changed, then all callers need to
adapt to that.

There are two ways to go here:  Revert the pattern_match_equivalence()
change and alter cfp_matcher_range() to add one to the index that it
passes, or change the other two callers to decrement their indices by
one.  I tested the latter and it works as expected, but I think the
former might be the more sensible approach.

Re: Regression affecting completion with NO_CASE_GLOB

[Bart Schaefer]

On Thu, Jun 2, 2022 at 7:34 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> There are two ways to go here:  Revert the pattern_match_equivalence()
> change and alter cfp_matcher_range() to add one to the index that it
> passes, or change the other two callers to decrement their indices by
> one.  I tested the latter and it works as expected, but I think the
> former might be the more sensible approach.

In fact pattern_match1() calls PATMATCHRANGE() and then returns ind+1,
whereas cfp_matcher_range() calls PATMATCHRANGE() and then uses ind
directly, so it's pretty clear that reverting and fixing
cfp_matcher_range() is the right way to go.

diff --git a/Src/Zle/compmatch.c b/Src/Zle/compmatch.c
index bb8359f1d..56e5509a4 100644
--- a/Src/Zle/compmatch.c
+++ b/Src/Zle/compmatch.c
@@ -1319,7 +1319,7 @@ pattern_match_equivalence(Cpattern lp,
convchar_t wind, int wmtp,
     convchar_t lchr;
     int lmtp;

-    if (!PATMATCHINDEX(lp->u.str, wind, &lchr, &lmtp)) {
+    if (!PATMATCHINDEX(lp->u.str, wind-1, &lchr, &lmtp)) {
        /*
         * No equivalent.  No possible match; give up.
         */
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index 59abb4cc4..77ccdebf7 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -4383,7 +4383,7 @@ cfp_matcher_range(Cmatcher *ms, char *add)
                         * word pattern.
                         */
                        if ((ind = pattern_match_equivalence
-                            (m->word, ind, mt, addc)) != CHR_INVALID) {
+                            (m->word, ind+1, mt, addc)) != CHR_INVALID) {
                            if (ret) {
                                if (imeta(ind)) {
                                    *p++ = Meta;

Re: Regression affecting completion with NO_CASE_GLOB

[John Heatherington]

On Thu, Jun 2, 2022 at 10:42 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> In fact pattern_match1() calls PATMATCHRANGE() and then returns ind+1,
> whereas cfp_matcher_range() calls PATMATCHRANGE() and then uses ind
> directly, so it's pretty clear that reverting and fixing
> cfp_matcher_range() is the right way to go.

That's definitely more consistent. Thanks for the quick response!