* Regression in braces completion @ 2008-10-12 2:32 Vin Shelton 2008-10-12 4:32 ` Bart Schaefer 0 siblings, 1 reply; 12+ messages in thread From: Vin Shelton @ 2008-10-12 2:32 UTC (permalink / raw) To: Zsh Hackers' List I just noticed a problem with completion using braces that has apparently been around since at least June. Here's the recipe to reproduce: zsh -f $ touch abcdef abcdefg abcghi $ ls abc{d<TAB> ==> $ ls abc{def as it should. autoload -U compinit compinit $ ls abc{d<TAB> ==> $ ls abcdef{ which is not correct. This is under cygwin on windows. I can reproduce this problem with a zsh built on 2008-06-17, but a zsh built on 2008-01-02 does not exhibit this problem. Let me know if you need any more info to reproduce this problem. - Vin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Regression in braces completion 2008-10-12 2:32 Regression in braces completion Vin Shelton @ 2008-10-12 4:32 ` Bart Schaefer 2008-10-12 7:10 ` Bart Schaefer 0 siblings, 1 reply; 12+ messages in thread From: Bart Schaefer @ 2008-10-12 4:32 UTC (permalink / raw) To: Zsh Hackers' List On Oct 11, 10:32pm, Vin Shelton wrote: } } $ ls abc{d<TAB> ==> } $ ls abcdef{ } } which is not correct. Hmm. Looking at _complete_debug output, "abc{d" has already become "abcdef" before the first completer is even called. In fact, it has become that before _setup is even called. Here we are, barely into _main_complete: : _main_complete:49:if; [[ automenu-unambiguous == tab* ]] : _main_complete:59:if; [[ -z '' ]] : _main_complete:60:then if; [[ -o equals ]] : _main_complete:60:then if cmdand; compset -P 1 '=' : _main_complete:62:then elif; [[ abcdef != */* && a == \~ ]] : _main_complete:70; _setup default That's $PREFIX on line 62 and the curly brace is already missing. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Regression in braces completion 2008-10-12 4:32 ` Bart Schaefer @ 2008-10-12 7:10 ` Bart Schaefer 2008-10-12 19:42 ` Peter Stephenson 0 siblings, 1 reply; 12+ messages in thread From: Bart Schaefer @ 2008-10-12 7:10 UTC (permalink / raw) To: Zsh Hackers' List On Oct 11, 9:32pm, Bart Schaefer wrote: } } On Oct 11, 10:32pm, Vin Shelton wrote: } } } } $ ls abc{d<TAB> ==> } } $ ls abcdef{ } } } } which is not correct. } } : _main_complete:62:then elif; [[ abcdef != */* && a == \~ ]] } } That's $PREFIX on line 62 and the curly brace is already missing. Turns out that's a red herring, it was that way in the older version as well. As nearly as I can track it down, the problem is with: * 25130 slightly tweaked for typos: Completion/Unix/Type/_path_files: changes to use -U flag to compadd so that spelling corrections in non-final path segments are accepted. If I remove the -U option from all the calls to compadd in _path_files, the old behavior is restored. (That took *way* longer to track down than it should have, because I didn't realize just how *many* calls to "compadd -U" there are, and I missed the crucial one the first time and ended up chasing a different red herring for an hour. Oy.) Removing the -U *only* from the compadd on line 634 of _path_files is sufficient to fix this particular example, but I wonder if it should be removed from all three of the compadd calls in the "we are listing, or something" branch (see comment line 605), or if it needs to be omitted based on some other condition ... or if we need to know when spelling correction has been done so we can only add -U when we have to, or some even more complicated set of conditions. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Regression in braces completion 2008-10-12 7:10 ` Bart Schaefer @ 2008-10-12 19:42 ` Peter Stephenson 2008-10-12 22:47 ` Bart Schaefer 0 siblings, 1 reply; 12+ messages in thread From: Peter Stephenson @ 2008-10-12 19:42 UTC (permalink / raw) To: Zsh Hackers' List On Sun, 12 Oct 2008 00:10:51 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > (That took *way* longer to track down than it should have, because I > didn't realize just how *many* calls to "compadd -U" there are, and I > missed the crucial one the first time and ended up chasing a different > red herring for an hour. Oy.) I don't know what they're all doing, either. > Removing the -U *only* from the compadd on line 634 of _path_files is > sufficient to fix this particular example, but I wonder if it should be > removed from all three of the compadd calls in the "we are listing, or > something" branch (see comment line 605), or if it needs to be omitted > based on some other condition ... or if we need to know when spelling > correction has been done so we can only add -U when we have to, or some > even more complicated set of conditions. There will be an underlying reason why this particular case inserts the brace in the wrong place. It will take many hours of work to find out. -- Peter Stephenson <p.w.stephenson@ntlworld.com> Web page now at http://homepage.ntlworld.com/p.w.stephenson/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Regression in braces completion 2008-10-12 19:42 ` Peter Stephenson @ 2008-10-12 22:47 ` Bart Schaefer 2008-10-13 9:04 ` Peter Stephenson 0 siblings, 1 reply; 12+ messages in thread From: Bart Schaefer @ 2008-10-12 22:47 UTC (permalink / raw) To: Zsh Hackers' List On Oct 12, 8:42pm, Peter Stephenson wrote: } } There will be an underlying reason why this particular case inserts the } brace in the wrong place. It will take many hours of work to find out. It's not really "this particular case". The brace ends up in the wrong place in the following examples too: schaefer<508> ls abc{g,d<TAB> schaefer<508> ls abcdef{g, schaefer<516> cd .. schaefer<517> ls dta/abc{d<TAB> schaefer<517> ls dta/abcdef{ Seems to happen any time there's a longer unambiguous prefix possible. In order to find the possible completions of the rightmost expansion of the brace expression, the C code removes everything between the brace and the comma, inclusive, and then asks the completer (which happens to be _path_files) for the matches. It's not expecting the completer to modify the command line as a side-effect, but that's what happens when "compadd -U" is called. Is there a reason you think it's deeper than that? This one is interesting, though: schaefer<518> ls dat/abc{d<TAB> schaefer<518> ls dta/abc{def/ Completing corrections abcdef/ abcdefg/ Completing original dat/abcd Note that it's wrong about what the "original" string is, and sort of wrong about what the corrections are, because really the correction is "dat" --> "dta" ... but in this case the brace is in the right place, and menu completion cycles through: schaefer<518> ls dta/abc{def/ schaefer<518> ls dta/abc{defg/ schaefer<518> ls dat/abc{d Looking at _complete_debug output, this seems in part to be because with the misspelt "dat" in front, "dta/abcghi" remains a candidate match until later in the process. It helps, when testing this stuff, to remove _expand and _oldlist from the completer style. -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Regression in braces completion 2008-10-12 22:47 ` Bart Schaefer @ 2008-10-13 9:04 ` Peter Stephenson 2008-10-13 15:43 ` Bart Schaefer 0 siblings, 1 reply; 12+ messages in thread From: Peter Stephenson @ 2008-10-13 9:04 UTC (permalink / raw) To: Zsh Hackers' List On Sun, 12 Oct 2008 15:47:19 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > In order to find the possible completions of the rightmost expansion > of the brace expression, the C code removes everything between the > brace and the comma, inclusive, and then asks the completer (which > happens to be _path_files) for the matches. It's not expecting the > completer to modify the command line as a side-effect, but that's > what happens when "compadd -U" is called. No, that's not how "compadd -U" works. It simply changes the rules of how completions added by the command are treated. The actual modification of the command line happens later in any case. It's very roughly like this: - completion system kicks off a set of functions - somewhere inside those compadd is called - within the C code some internal variables are updated to add lists of things grouped together in horribly obscure ways, storing the fact that "-U" was present - eventually the completion function exits - the code in compresult.c uses the previous results to decide how to manipulate the command line. As the compresult code is called a lot later than the "compadd" and there's no clear grouping of variables used by it, it's extremely difficult to pin changes in the result onto changes in the command that added the completion, particularly since the whole thing is sensitive (via what's visible as compstate etc.) to context (although different context shouldn't be an issue here). One of the things that's taken account of at various places along the line, including in the compresult code that adds ambiguous or unambiguous completions, is whether -U was present (except that it's called something entirely different internally that I've forgotten). It's certainly possible that the code that takes account of braces in the case of compadd -U isn't present because it's never been tested before, but that would be bug: the C code takes pains to hide the existence of the braces from the rest of the system, so there's no way you can treat this as a special case anywhere else, and if you did it would arbitrarily limit the completions you could do on expressions with braces. pws ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Regression in braces completion 2008-10-13 9:04 ` Peter Stephenson @ 2008-10-13 15:43 ` Bart Schaefer 2008-10-13 16:27 ` Peter Stephenson 0 siblings, 1 reply; 12+ messages in thread From: Bart Schaefer @ 2008-10-13 15:43 UTC (permalink / raw) To: Zsh Hackers' List On Oct 13, 10:04am, Peter Stephenson wrote: } Subject: Re: Regression in braces completion } } It's certainly possible that the code that takes account of braces in } the case of compadd -U isn't present because it's never been tested } before, but that would be bug: the C code takes pains to hide the } existence of the braces from the rest of the system The direct effect of -U (if the doc is remotely descriptive of the code) is to cause matching of the returned words against the command line to be skipped, so perhaps the code that handles braces is embedded in the compresult code that performs matching. The usual indirect effect of -U is that the the word on the command line is thrown away and replaced entirely with the result from compadd, which does look like what's happening here. Maybe all that's been lost is the position at which to re-insert the brace, but I'm betting that this comment above cline_str() is significant: /* This builds the unambiguous string. If ins is one, it is immediately * inserted into the line. Otherwise csp is used to return the relative * cursor position in the string returned and posl contains all * positions with missing or ambiguous characters. If ins is two, csp * and posl contain real command line positions (including braces). */ Comparing walkthroughs of cline_str() when ins==1 for the correct case (-U removed from compadd) and the incorrect case, the difference is at line 240, where in the correct case li==3 and in the incorrect li==0. The value for li comes from l->prefix->llen, which must be the result of the call to cut_cline() on line 175. I've run out of time to spend on this, but I suspect that the effect of the -U option is to cause cut_cline() to decide that nothing that was already on the line is worth keeping, when really in this case we want to keep the first three characters up to the brace. However, I'm not sure the C code is actually wrong here. Postulate a completer that given "abcd" as the thing to match, returns "zyxw" and uses compadd -U to say "accept this as a completion even though it does not look like what you started with." Now call that completer for a line with "abc{d". Is the right answer really "zyx{w" ? } so there's no way you can treat this as a special case anywhere else I wasn't suggesting treating this as a special case, I was suggesting that _path_files treat a spelling correction in the path prefix as a special case. That's the stated reason why _path_files is using -U. In fact _path_files probably ought to bail out in the case where it has corrected the path prefix and make the user confirm that the correction is, well, correct, before it goes on to guessing about stuff further along in the path. The trouble most likely is that we're trying to do too many things in one go. Of course if we change that, someone will complain that what used to need only one tab now needs two, but this may be a case of lesser of evils. Or there may be an entirely other way of solving this, such as adding the entire paths including the correction as separate matches so that the point of ambiguity is much farther to the left. In any case if you agree that the C code behavior for -U is correct, then the right fix is something that eliminates the -U from _path_files by using another approach for the original auto-correction problem. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Regression in braces completion 2008-10-13 15:43 ` Bart Schaefer @ 2008-10-13 16:27 ` Peter Stephenson 2008-10-13 16:56 ` Bart Schaefer 2008-10-14 4:37 ` PATCH (?) " Bart Schaefer 0 siblings, 2 replies; 12+ messages in thread From: Peter Stephenson @ 2008-10-13 16:27 UTC (permalink / raw) To: Zsh Hackers' List Bart Schaefer wrote: > However, I'm not sure the C code is actually wrong here. Postulate a > completer that given "abcd" as the thing to match, returns "zyxw" and > uses compadd -U to say "accept this as a completion even though it does > not look like what you started with." Now call that completer for a > line with "abc{d". Is the right answer really "zyx{w" ? Yes, if the C code has (as it does) stripped all the stuff to do with the parts of the brace expansion it doesn't think you want to see away before you ever got to look at the word to be completed. Otherwise you get a meaningless hybrid---you're adding back braces you've never had any information about in the first place. (I can see how you could argue it differently: you can infer the braces by looking at the raw command line and tell the completion system that you've decided you don't like what's done and decide to do it yourself. However, that's overloading -U with multiple meanings: not just "use my completion as it is in your results table" but "forget everything you've done so far and insert this the way I want". I'm not convinced it even works---the system is now thoroughly confused about what it's actually completing, so working out what it's replacing with your string isn't easy. What -U does and doesn't do is already rather obscure, so this is all probably moot.) I think to do it consistently with the other interpretation, you would need completion internally to ignore the presence of braces right from the start and allow the function system to decide whether to handle them. That's not going to happen any time soon---though it's actually possible it's a more elegant solution. > I wasn't suggesting treating this as a special case, I was suggesting > that _path_files treat a spelling correction in the path prefix as a > special case. That's the stated reason why _path_files is using -U. As you previously noted, there are already far too many ways of running compadd within _path_files as it is. I'm not at all happy about doubling the number to get sets with and without -U --- which would leave spelling correction broken in the case where braces are present (perhaps depending which way the pinball decides to roll down _path_files in that case, but I presume it would still miss the flippers in the same way). -- Peter Stephenson <pws@csr.com> Software Engineer CSR PLC, Churchill House, Cambridge Business Park, Cowley Road Cambridge, CB4 0WZ, UK Tel: +44 (0)1223 692070 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Regression in braces completion 2008-10-13 16:27 ` Peter Stephenson @ 2008-10-13 16:56 ` Bart Schaefer 2008-10-14 4:37 ` PATCH (?) " Bart Schaefer 1 sibling, 0 replies; 12+ messages in thread From: Bart Schaefer @ 2008-10-13 16:56 UTC (permalink / raw) To: Zsh Hackers' List On Oct 13, 5:27pm, Peter Stephenson wrote: } } However, that's overloading -U with multiple meanings: not just "use my } completion as it is in your results table" but "forget everything you've } done so far and insert this the way I want". I think if you look around at the other uses of -U in the existing completion functions, you'll find that it's employed in the latter way in a significant number of cases. In other words, it's too late, -U has already become overloaded. -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* PATCH (?) Re: Regression in braces completion 2008-10-13 16:27 ` Peter Stephenson 2008-10-13 16:56 ` Bart Schaefer @ 2008-10-14 4:37 ` Bart Schaefer 2008-10-14 8:45 ` Peter Stephenson 1 sibling, 1 reply; 12+ messages in thread From: Bart Schaefer @ 2008-10-14 4:37 UTC (permalink / raw) To: Zsh Hackers' List On Oct 13, 5:27pm, Peter Stephenson wrote: } } > I wasn't suggesting treating this as a special case, I was suggesting } > that _path_files treat a spelling correction in the path prefix as a } > special case. That's the stated reason why _path_files is using -U. } } As you previously noted, there are already far too many ways of running } compadd within _path_files as it is. I'm not at all happy about } doubling the number to get sets with and without -U --- which would } leave spelling correction broken in the case where braces are present Somebody tell me what's wrong with this. Doesn't seem to break spelling correction in the path prefix when braces are present, and doesn't seem to break braces when spelling correction isn't present, and doesn't add any new calls to compadd; just changes whether the ones that are there get -U passed in. What am I missing? --- ../zsh-forge/current/Completion/Unix/Type/_path_files 2008-09-02 20:09:04.000000000 -0700 +++ Completion/Unix/Type/_path_files 2008-10-13 21:31:53.000000000 -0700 @@ -7,7 +7,7 @@ local tmp1 tmp2 tmp3 tmp4 i orig eorig pre suf tpre tsuf opre osuf cpre local pats haspats ignore pfx pfxsfx sopt gopt opt sdirs ignpar cfopt listsfx local nm=$compstate[nmatches] menu matcher mopts sort mid accex fake -local listfiles listopts tmpdisp origtmp1 +local listfiles listopts tmpdisp origtmp1 Uopt integer npathcheck local -a match mbegin mend @@ -209,7 +209,7 @@ [[ $compstate[insert] = (*menu|[0-9]*) || -n "$_comp_correct" || ( -n "$compstate[pattern_match]" && "${orig#\~}" != (|*[^\\])[][*?#~^\|\<\>]* ) ]] && menu=yes -[[ -n "$_comp_correct" ]] && cfopt=- +[[ -n "$_comp_correct" ]] && cfopt=- Uopt=-U # Now let's have a closer look at the string to complete. @@ -612,7 +612,7 @@ # back up the path. tmp1=("${(@)tmp1%%/*}") _list_files tmp1 "$prepath$realpath$testpath" - compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \ + compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \ -s "/${tmp3#*/}$ISUFFIX" \ -W "$prepath$realpath$testpath" \ "$pfxsfx[@]" \ @@ -622,7 +622,7 @@ # Same with a non-empty suffix tmp1=("${(@)^tmp1%%/*}/${tmp3#*/}") _list_files tmp1 "$prepath$realpath$testpath" - compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \ + compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \ -s "$ISUFFIX" \ -W "$prepath$realpath$testpath" \ "$pfxsfx[@]" \ @@ -631,7 +631,7 @@ fi else _list_files tmp1 "$prepath$realpath$testpath" - compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \ + compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \ -s "$ISUFFIX" \ -W "$prepath$realpath$testpath" \ "$pfxsfx[@]" \ @@ -661,7 +661,7 @@ fi else _list_files tmp1 "$prepath$realpath$testpath" - compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \ + compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \ -s "$ISUFFIX" \ -W "$prepath$realpath$testpath" \ "$pfxsfx[@]" \ @@ -730,7 +730,7 @@ compquote tmp4 tmp2 tmp1 for i in "$tmp1[@]"; do _list_files tmp2 "$prepath$realpath${mid%/*/}" - compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp3/" \ + compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp3/" \ -s "/$tmp4$i$ISUFFIX" \ -W "$prepath$realpath${mid%/*/}/" \ "$pfxsfx[@]" $listopts - "$tmp2" @@ -761,7 +761,7 @@ else # Not a pattern match _list_files tmp1 "$prepath$realpath$testpath" - compadd -U -Qf -p "$IPREFIX$linepath$tmp4" \ + compadd $Uopt -Qf -p "$IPREFIX$linepath$tmp4" \ -s "$ISUFFIX" \ -W "$prepath$realpath$testpath" \ "$pfxsfx[@]" "$mopts[@]" $listopts -a tmp1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH (?) Re: Regression in braces completion 2008-10-14 4:37 ` PATCH (?) " Bart Schaefer @ 2008-10-14 8:45 ` Peter Stephenson 2008-10-15 1:34 ` Vin Shelton 0 siblings, 1 reply; 12+ messages in thread From: Peter Stephenson @ 2008-10-14 8:45 UTC (permalink / raw) To: Zsh Hackers' List Bart Schaefer wrote: > Somebody tell me what's wrong with this. Doesn't seem to break spelling > correction in the path prefix when braces are present, and doesn't seem > to break braces when spelling correction isn't present, and doesn't add > any new calls to compadd; just changes whether the ones that are there > get -U passed in. > > What am I missing? Presumably we won't know for months until somebody tries the completion in question, but if this seems to fix all the obvious problems feel free to commit it, since I wasn't going to have much time to look at compresult.c anyway. (It might, for example, break spelling correction where the prefix itself is in braces, but I don't feel particularly moved to experiment.) -- Peter Stephenson <pws@csr.com> Software Engineer CSR PLC, Churchill House, Cambridge Business Park, Cowley Road Cambridge, CB4 0WZ, UK Tel: +44 (0)1223 692070 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH (?) Re: Regression in braces completion 2008-10-14 8:45 ` Peter Stephenson @ 2008-10-15 1:34 ` Vin Shelton 0 siblings, 0 replies; 12+ messages in thread From: Vin Shelton @ 2008-10-15 1:34 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh Hackers' List On Tue, Oct 14, 2008 at 4:45 AM, Peter Stephenson <pws@csr.com> wrote: > Bart Schaefer wrote: >> Somebody tell me what's wrong with this. Doesn't seem to break spelling >> correction in the path prefix when braces are present, and doesn't seem >> to break braces when spelling correction isn't present, and doesn't add >> any new calls to compadd; just changes whether the ones that are there >> get -U passed in. >> >> What am I missing? > > Presumably we won't know for months until somebody tries the completion > in question, but if this seems to fix all the obvious problems feel free > to commit it, since I wasn't going to have much time to look at > compresult.c anyway. (It might, for example, break spelling correction > where the prefix itself is in braces, but I don't feel particularly > moved to experiment.) > Bart - +1 Your patch worked for my case, but you probably already guessed that. - Vin ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2008-10-15 1:35 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-10-12 2:32 Regression in braces completion Vin Shelton 2008-10-12 4:32 ` Bart Schaefer 2008-10-12 7:10 ` Bart Schaefer 2008-10-12 19:42 ` Peter Stephenson 2008-10-12 22:47 ` Bart Schaefer 2008-10-13 9:04 ` Peter Stephenson 2008-10-13 15:43 ` Bart Schaefer 2008-10-13 16:27 ` Peter Stephenson 2008-10-13 16:56 ` Bart Schaefer 2008-10-14 4:37 ` PATCH (?) " Bart Schaefer 2008-10-14 8:45 ` Peter Stephenson 2008-10-15 1:34 ` Vin Shelton
Code repositories for project(s) associated with this public 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).