Re: minor niggle with svn completion of sub-commands

[Greg Klanderman <gak@xxxxxxxxxxxxxx>]

>>>>> On November 15, 2009 Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> 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