minor niggle with svn completion of sub-commands

minor niggle with svn completion of sub-commands

[Greg Klanderman]

Can someone explain what's causing the completion of the svn
sub-command to only work when there are no words to the right of
the cursor?

For example,

% svn lo<tab>

completes to "svn log ", but

% svn lo<tab> foo

gives no completions.

I don't really understand _arguments too well, but it looks like the
use in _subversion is attempting to complete commands in the first
position.. why doesn't that always work?

thanks,
Greg

Re: minor niggle with svn completion of sub-commands

[Greg Klanderman]

>>>>> On November 9, 2009 Greg Klanderman <gak@klanderman.net> wrote:

> I don't really understand _arguments too well, but it looks like the
> use in _subversion is attempting to complete commands in the first
> position.. why doesn't that always work?

Hmmm does this look like a bug in _arguments?  _svn is calling
_arguments like this:

  _arguments -C \
    '(-)--help[print help information]' \
    '(- *)--version[print client version information]' \
    '1: :->cmds' \
    '*:: :->args' && ret=0

and state is returned set to 'args' when <tab> is pressed for
completion in the position indicated:

% svn lo<tab> foo

>From reading the documentation on _arguments, I would think the
'1: :->cmds' spec would match, since we are in fact completing
the first argument, even though there are subsequent arguments.

thanks,
Greg

Re: minor niggle with svn completion of sub-commands

[Greg Klanderman]

>>>>> On November 9, 2009 Greg Klanderman <gak@klanderman.net> wrote:

> From reading the documentation on _arguments, I would think the
> '1: :->cmds' spec would match, since we are in fact completing
> the first argument, even though there are subsequent arguments.

Would anyone mind taking a quick look and validating for me whether
you agree that it looks like _arguments is doing the wrong thing here
before I dive into trying to debug that?

thanks,
Greg

Re: minor niggle with svn completion of sub-commands

[Peter Stephenson]

On Thu, 12 Nov 2009 12:53:35 -0500
Greg Klanderman <gak@klanderman.net> wrote:
> >>>>> On November 9, 2009 Greg Klanderman <gak@klanderman.net> wrote:
> 
> > From reading the documentation on _arguments, I would think the
> > '1: :->cmds' spec would match, since we are in fact completing
> > the first argument, even though there are subsequent arguments.
> 
> Would anyone mind taking a quick look and validating for me whether
> you agree that it looks like _arguments is doing the wrong thing here
> before I dive into trying to debug that?

I would have thought so, but as usual there are no real experts on this
stuff.  It might be an unexpected interaction with something else in the
completion system (which wouldn't stop it being a bug).

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

Re: minor niggle with svn completion of sub-commands

[Greg Klanderman]

>>>>> On November 12, 2009 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:

> I would have thought so, but as usual there are no real experts on this
> stuff.  It might be an unexpected interaction with something else in the
> completion system (which wouldn't stop it being a bug).

I traced the problem a bit further into the comparguments C code, and
it got pretty hopeless at that point just trying to read the code.  If
I could easily locate the code that's matching the parsed definitions
against the word to be completed, maybe that would help.  It seems
like maybe it's in ca_parse_line.. I guess next step is to attach gdb
and step through that.

Greg

Re: minor niggle with svn completion of sub-commands

[Greg Klanderman]

Here's a simple test showing that the '*::...' type argument
descriptions are not handled correctly by _arguments:

compdef _foo foo

_foo () { _arguments '1: :_foo_arg1' '*: :_foo_rest' }
% foo a<tab> b  => correctly prints "command not found: _foo_arg1"

_foo () { _arguments '1: :_foo_arg1' '*:: :_foo_rest' }
% foo a<tab> b  => incorrectly prints "command not found: _foo_rest"

cheers,
Greg

Re: minor niggle with svn completion of sub-commands

[Bart Schaefer]

On Nov 15,  2:47am, Greg Klanderman wrote:
} 
} Here's a simple test showing that the '*::...' type argument
} descriptions are not handled correctly by _arguments:

It all goes awry at computil.c line 2133:

	    if ((adef = state.def = ca_get_arg(d, state.nth)) &&
		(state.def->type == CAA_RREST ||
		 state.def->type == CAA_RARGS)) {

for the '*::' case, state.def->type == CAA_RREST so we enter the block
at 2136 and exit the entire loop at the "break" on 2151.  Line 2133 is
the only place that RREST and RARGS are treated differently than REST
(line 2173 doesn't count as that's for the case of an argument of an
option ("optspec"), not a "normal argument").

The problem seems to be that we blindly clobber ca_laststate upon
discovering that we've reached the "rest" description.  The patch
below changes this behavior but I'm not certain it's complete; it
may need an "else" clause, or maybe we should be breaking out of
the entire loop elsewhere as soon as ca_laststate.def is non-NULL.

This patch may claim it applies with an offset, it's a diff against
my own repository rather than the official one.

Index: computil.c
===================================================================
RCS file: /extra/cvsroot/zsh/zsh-4.0/Src/Zle/computil.c,v
retrieving revision 1.32
diff -c -r1.32 computil.c
--- computil.c	23 Nov 2008 18:26:28 -0000	1.32
+++ computil.c	16 Nov 2009 00:10:26 -0000
@@ -2130,10 +2144,12 @@
 		for (; line; line = compwords[cur++])
 		    zaddlinknode(state.args, ztrdup(line));
 
-		memcpy(&ca_laststate, &state, sizeof(state));
-		ca_laststate.ddef = NULL;
-		ca_laststate.dopt = NULL;
-		ca_laststate.doff = 0;
+		if (!ca_laststate.def) {
+		    memcpy(&ca_laststate, &state, sizeof(state));
+		    ca_laststate.ddef = NULL;
+		    ca_laststate.dopt = NULL;
+		    ca_laststate.doff = 0;
+		}
 		break;
 	    }
 	    zaddlinknode(state.args, ztrdup(line));

Re: minor niggle with svn completion of sub-commands

[Greg Klanderman]

>>>>> On November 15, 2009 Bart Schaefer <schaefer@brasslantern.com> wrote:

> It all goes awry at computil.c line 2133:

> 	    if ((adef = state.def = ca_get_arg(d, state.nth)) &&
> 		(state.def->type == CAA_RREST ||
>                state.def-> type == CAA_RARGS)) {

Thanks for looking into this Bart!  I spent way too much time over the
weekend staring at that code trying to make sense of it, and had
actually spent a bunch of time looking at that 'Otherwise it's a
normal argument.' section of code without much enlightenment.

> for the '*::' case, state.def->type == CAA_RREST so we enter the block

Actually, I think '*::' is CAA_RARGS and '*:::' is CAA_RREST, but it
doesn't matter to this discussion.

> at 2136 and exit the entire loop at the "break" on 2151.  Line 2133 is
> the only place that RREST and RARGS are treated differently than REST
> (line 2173 doesn't count as that's for the case of an argument of an
> option ("optspec"), not a "normal argument").

> The problem seems to be that we blindly clobber ca_laststate upon
> discovering that we've reached the "rest" description.  The patch
> below changes this behavior but I'm not certain it's complete; it

Now looking at this section of code again, I notice that in most cases
where ca_laststate is set, it is conditioned on a predicate involving
'cur' (index of current word being parsed) and 'compcurrent' (index of
word being completed) which makes some sense (though I don't
understand why sometimes it's 'cur < compcurrent' vs 'cur + 1 ==
compcurrent' vs 'cur == compcurrent'...) so that leads me to wonder if
the condition you want to add should be some function of those.

There's one other place that changes ca_laststate inside the loop over
words (why is the local variable named 'line'?!) in the line, around
line 1977:

|	    if (state.def->type == CAA_REST || state.def->type == CAA_RARGS ||
|		state.def->type == CAA_RREST) {
|		if (state.def->end && pattry(endpat, line)) {
|		    state.def = NULL;
|		    state.curopt = NULL;
|		    state.opt = state.arg = 1;
|		    state.argend = ca_laststate.argend = cur - 1;
|		    goto cont;
|		}

and it seems fishy because it is continuing the loop, not breaking out
of it, and the 'cont' label logic seems to be responsible for when
state data gets copied to ca_laststate.  Also, why is it setting just
that one field in ca_laststate?

It also seems strange that CAA_REST is not included in the conditions
leading to the block of code you changed.  Why should CAA_REST be
treated any differently here?  Seems like the distinction should only
be in ca_set_data() where restrict_range() is called differently
depending on which rest variant was used.

> may need an "else" clause, or maybe we should be breaking out of
> the entire loop elsewhere as soon as ca_laststate.def is non-NULL.

Maybe under the 'cont' label?  Is there any reason to parse the line
past the current word being completed?

> This patch may claim it applies with an offset, it's a diff against
> my own repository rather than the official one.

I'll give it a try.. thanks!

Greg

Re: minor niggle with svn completion of sub-commands

[Bart Schaefer]

On Nov 16,  2:45pm, Greg Klanderman wrote:
} Subject: Re: minor niggle with svn completion of sub-commands
}
} > The problem seems to be that we blindly clobber ca_laststate upon
} > discovering that we've reached the "rest" description.  The patch
} > below changes this behavior but I'm not certain it's complete; it
} 
} Now looking at this section of code again, I notice that in most cases
} where ca_laststate is set, it is conditioned on a predicate involving
} 'cur' (index of current word being parsed) and 'compcurrent' (index of
} word being completed) which makes some sense

Elswhere it matters whether the index of the current "definition"
being examined matches the index of the word being completed.  Here
the code has reached the "rest" of the arguments so neither index is
of consequence.

The problem is that the code appears to assume this condition won't be
reached if an earlier pair of indices matched up.  This is why the
code in parse_cadef inserts the definitions in sorted order.  This is
also why I'm increasingly convinced that the entire loop should end
as soon as any definition has been found for the index of the word
being completed.

} There's one other place that changes ca_laststate inside the loop over
} words (why is the local variable named 'line'?!)

Because it's an array of all the words that make up the entire line
from the editor (i.e., the L in ZLE).  This is just the way Sven W's
mind works.  (Now if only we had it back working for us ...)

} in the line, around line 1977:
} 
} |	    if (state.def->type == CAA_REST || state.def->type == CAA_RARGS ||
} |		state.def->type == CAA_RREST) {
} |		if (state.def->end && pattry(endpat, line)) {
} |		    state.def = NULL;
} |		    state.curopt = NULL;
} |		    state.opt = state.arg = 1;
} |		    state.argend = ca_laststate.argend = cur - 1;
} |		    goto cont;
} |		}
} 
} and it seems fishy because it is continuing the loop, not breaking out
} of it, and the 'cont' label logic seems to be responsible for when
} state data gets copied to ca_laststate.  Also, why is it setting just
} that one field in ca_laststate?

In this case AFAICT ca_laststate is being kept up to date in case NONE
of the conditions in the "if" just below "cont:" turn out to be true.
That is, ca_laststate should have something sensible in it even if we
loop through all possible definitions without finding any that apply
to the current word.
 
} It also seems strange that CAA_REST is not included in the conditions
} leading to the block of code you changed.  Why should CAA_REST be
} treated any differently here?

I'm not entirely sure but I think CAA_REST is omitted here because
the later code implements both normal-argument and argument-following-
an-option cases.

} > the entire loop elsewhere as soon as ca_laststate.def is non-NULL.
} 
} Maybe under the 'cont' label?  Is there any reason to parse the line
} past the current word being completed?

I begin to think the test really belongs in the "for" statement loop
conditions at the top.  "line && !ca_laststate.def" rather than just
"line".

comparguments (was Re: minor niggle with svn completion of sub-commands)

[Bart Schaefer]

On Nov 16,  8:01pm, Bart Schaefer wrote:
}
} I begin to think the test really belongs in the "for" statement loop
} conditions at the top.  "line && !ca_laststate.def" rather than just
} "line".

So it turns out that breaks :*PATTERN:::MESSAGE:ACTION in optspecs.
Fortunately there's a check for that one in the test suite.

Here's a patch I think is worth committing.

--- computil.c.~1.116.~	2009-08-31 07:24:38.000000000 -0700
+++ computil.c	2009-11-17 07:53:43.000000000 -0800
@@ -2133,6 +2133,23 @@
 	    if ((adef = state.def = ca_get_arg(d, state.nth)) &&
 		(state.def->type == CAA_RREST ||
 		 state.def->type == CAA_RARGS)) {
+
+		/* Bart 2009/11/17:
+		 * We've reached the "rest" definition.  If at this point
+		 * we already found another definition that describes the
+		 * current word, use that instead.  If not, prep for the
+		 * "narrowing" of scope to only the remaining words.
+		 *
+		 * We can't test ca_laststate.def in the loop conditions
+		 * at the top because this same loop also handles the
+		 * ':*PATTERN:MESSAGE:ACTION' form for multiple arguments
+		 * after an option, which may need to continue scanning.
+		 * There might be an earlier point at which this test can
+		 * be made but tracking it down is not worth the effort.
+		 */
+		if (ca_laststate.def)
+		    break;
+
 		state.inrest = 0;
 		state.opt = (cur == state.nargbeg + 1 &&
 			     (!multi || !*line || 

Re: comparguments (was Re: minor niggle with svn completion of sub-commands)

[Greg Klanderman]

>>>>> On November 17, 2009 Bart Schaefer <schaefer@brasslantern.com> wrote:

> Here's a patch I think is worth committing.

Thank you again for tracking this down, Bart.

greg