need help debugging cvs completion problem

need help debugging cvs completion problem

[Greg Klanderman]

Hi,

The ztrftime patch I just sent fixes one problem with cvs completion,
in the _cvs_modified_entries function (used by 'cvs diff' completions),
but there appears to be another problem in _cvs_existing_entries (used
by 'cvs annotate' completions, for example).  The variable 'pat' is
being populated with the correct set of completions, however, the
final line:

  (( ${#pat} )) && _wanted files expl file _path_files -a pat

then results in all files (even those not under CVS control) being
considered for completion.

I assume this is some problem with _path_files, so am hoping someone
might be able to at least tell me what the '-a' argument is supposed
to be doing.  At a higher level, if we already know the completions,
why are we using _path_files in the first place here?

thanks,
Greg

Re: need help debugging cvs completion problem

[Peter Stephenson]

On Fri, 26 Jun 2009 18:19:18 -0400
Greg Klanderman <gak@klanderman.net> wrote:
> The ztrftime patch I just sent fixes one problem with cvs completion,
> in the _cvs_modified_entries function (used by 'cvs diff' completions),
> but there appears to be another problem in _cvs_existing_entries (used
> by 'cvs annotate' completions, for example).  The variable 'pat' is
> being populated with the correct set of completions, however, the
> final line:
> 
>   (( ${#pat} )) && _wanted files expl file _path_files -a pat
> 
> then results in all files (even those not under CVS control) being
> considered for completion.

It looks to me like that should probably be "compadd" rather than
"_path_files".

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: need help debugging cvs completion problem

[Greg Klanderman]

>>>>> Peter Stephenson <p.w.stephenson@ntlworld.com> writes:

> It looks to me like that should probably be "compadd" rather than
> "_path_files".

Yeah, that was one thought I had.. let me try that out and get back to you..

thanks
Greg

Re: need help debugging cvs completion problem

[Greg Klanderman]

>>>>> Peter Stephenson <p.w.stephenson@ntlworld.com> writes:

>> It looks to me like that should probably be "compadd" rather than
>> "_path_files".

Of course that seems to work.. that line in _cvs_existing_entries was
changed here:

| revision 1.20
| date: 2003/07/03 11:34:57;  author: pws;  state: Exp;  lines: +2 -2
| 19696: quoting of existing CVS-managed files was wrong

from something that looked very much like _cvs_modified_entries still
does, and I had already predicted that this function would have
quoting problems with the pattern constructed.  I will make the
analogous change, with the correction to compadd, to
_cvs_modified_entries, rename the 'pat' variable which is no longer a
pattern in both functions, and submit the patch.

'19696' is a reference to a mailing list message right?  How do I look
that up?  I tried the 'Look up message by official number' at

http://www.zsh.org/mla/

but it doesn't get me to a message that seems relevant..

thanks,
Greg

Re: need help debugging cvs completion problem

[Geoff Wing]

On Saturday 2009-06-27 21:11 -0400, Greg Klanderman output:
:>>>>> Peter Stephenson <p.w.stephenson@ntlworld.com> writes:
:>> It looks to me like that should probably be "compadd" rather than
:>> "_path_files".
:
:Of course that seems to work.. that line in _cvs_existing_entries was
:changed here:
:| revision 1.20
:| date: 2003/07/03 11:34:57;  author: pws;  state: Exp;  lines: +2 -2
:| 19696: quoting of existing CVS-managed files was wrong
[...stuff deleted...]
:'19696' is a reference to a mailing list message right?  How do I look
:that up?  I tried the 'Look up message by official number' at
:http://www.zsh.org/mla/
:but it doesn't get me to a message that seems relevant..

It's incorrectly listed.  It's really 18796 (on zsh-workers).

Regards,
Geoff

Re: need help debugging cvs completion problem

[Greg Klanderman]

>>>>> Geoff Wing <gcw@zsh.org> writes:

> It's incorrectly listed.  It's really 18796 (on zsh-workers).

Thank you Geoff!

Peter,

I now see why _path_files was being used prior to your change to
_cvs_existing_entries back in 2003 - your form (with compadd, since
otherwise it doesn't do anything) does not handle completing files in
a subdirectory of the current directory, i.e.

% cvs annotate foo/bar<tab>

The directory prefix is getting removed in _cvs_existing_entries, is
it safe to just paste '$linedir' back on the front of the resulting
filenames?

Once I figure that out, I'll resume my plan to make the analogous fix
to _cvs_modified_entries.  Would anyone be opposed to my starting by
using Bart's equivalent series of substitutions (in his followup to
18796) rather than the mother-of-all substitutions currently found
there?  I presume we're not striving to unmaintainable obfuscation, is
there some efficiency reason for the single substitution?

thanks,
Greg

Re: need help debugging cvs completion problem

[Greg Klanderman]

>>>>> Greg Klanderman <gak@klanderman.net> writes:

> The directory prefix is getting removed in _cvs_existing_entries, is
> it safe to just paste '$linedir' back on the front of the resulting
> filenames?

this seems to work:

_cvs_existing_entries() {
  local expl match linedir realdir files disp
  match=()
  : ${PREFIX:#(#b)(*/)(*)}
  linedir="$match[1]"
  realdir=${(e)~linedir}
  [[ -f "$realdir"CVS/Entries ]] &&
  disp=(${(@)${(@)${(@M)${(@f)"$(<"$realdir"CVS/Entries)"}:#/*}#/}%%/*})
  files=( ${linedir}${(@)^disp} )
  (( ${#disp} )) && _wanted files expl file compadd -d disp -a files
}

except it now breaks when 'linedir' contains a parameter substitution,
presumably the whole reason for the 'realdir' logic above, because the
'$' in the parameter substitution gets escaped with '\'.  But
presumably adding '-Q' to compadd would break completing files that
contain characters that need to be escaped.  Should I do that, and
escape characters that need it in $files?

Otherwise I'm running out of ideas; maybe I should look into why
whitespace in file names was not handled before Peter's change in
2003.

greg

Re: need help debugging cvs completion problem

[Peter Stephenson]

On Sun, 28 Jun 2009 14:52:55 -0400
Greg Klanderman <gak@klanderman.net> wrote:
> this seems to work:
> 
> _cvs_existing_entries() {
>   local expl match linedir realdir files disp
>   match=()
>   : ${PREFIX:#(#b)(*/)(*)}
>   linedir="$match[1]"
>   realdir=${(e)~linedir}
>   [[ -f "$realdir"CVS/Entries ]] &&
>   disp=(${(@)${(@)${(@M)${(@f)"$(<"$realdir"CVS/Entries)"}:#/*}#/}%%/*})
>   files=( ${linedir}${(@)^disp} )
>   (( ${#disp} )) && _wanted files expl file compadd -d disp -a files
> }
> 
> except it now breaks when 'linedir' contains a parameter substitution,
> presumably the whole reason for the 'realdir' logic above, because the
> '$' in the parameter substitution gets escaped with '\'.  But
> presumably adding '-Q' to compadd would break completing files that
> contain characters that need to be escaped.  Should I do that, and
> escape characters that need it in $files?

That sounds plausible---I think you're on the right lines.  You might
need a -U argument to compadd in that case, too.

Basically, you are (anybody is) on your (their) own in this type of
thing---lots of us have been somewhere similar once or twice and it was
so complicated we've forgotten and won't remember again without devoting
hours to remembering why we thought it was better to forget in the first
place.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: need help debugging cvs completion problem

[Peter Stephenson]

Peter Stephenson wrote:
> You might need a -U argument to compadd in that case, too.

Thinking more: in principle, you shouldn't.  It should be possible to
get what you're adding to have the same quoting as what's one the line
already, so that the matches you add have the correct prefix and suffix.
It may not be completely trivial to enforce that in all cases, however.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: need help debugging cvs completion problem

[Greg Klanderman]

>>>>> Peter Stephenson <p.w.stephenson@ntlworld.com> writes:

> That sounds plausible---I think you're on the right lines.  You might
> need a -U argument to compadd in that case, too.

> Basically, you are (anybody is) on your (their) own in this type of
> thing---lots of us have been somewhere similar once or twice and it was
> so complicated we've forgotten and won't remember again without devoting
> hours to remembering why we thought it was better to forget in the first
> place.

Hi Peter, sorry I had to put this aside for a bit, and am still not
quite ready to dig back into it.  Seems like to fix this right, I'm
headed down a path where I have to reinvent much of the cruft that's
already in _path_files, which is probably why the logic had been
trying to leverage that, albeit in a somewhat hacky way.  My next step
is to understand what was actually broken that you fixed in 2003, and
if there's not some way to go back to using _path_files.

Unfortunately, it seems that when the new completion system was
designed, it did not abstract stuff like handling parameter
references, quoting, etc like the compctl stuff did.  So if you're
doing file completion, _path_files has all that logic (at the expense
of being completely incomprehensible) and it mostly works, but other
sorts of completion either have to reimplement that logic (sometimes
partially and/or buggily), or just not handle these complications.

cheers,
Greg

Re: need help debugging cvs completion problem

[Peter Stephenson]

Greg Klanderman wrote:
> Unfortunately, it seems that when the new completion system was
> designed, it did not abstract stuff like handling parameter
> references, quoting, etc like the compctl stuff did.  So if you're
> doing file completion, _path_files has all that logic (at the expense
> of being completely incomprehensible) and it mostly works, but other
> sorts of completion either have to reimplement that logic (sometimes
> partially and/or buggily), or just not handle these complications.

Right, it's the same problem with generating the files for git
completion---if you're not using _path_files, you're starting from
scratch, and you don't have the useful stuff for handling directories,
hence you're doing too much work.

It would be really nice to have this all generic---pass in a directory
to a context-defined function, pass back a list of files, however
generated---even if it was in the first instance separate from
_path_files.  The combination of _path_files, compfiles, and the basic
completion system is as obfuscated as anything I know.

compctl was no better---but as the whole thing was hidden from you from
start to end it didn't matter.

-- 
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