From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28323 invoked by alias); 17 Nov 2009 04:02:15 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 27414 Received: (qmail 17838 invoked from network); 17 Nov 2009 04:02:03 -0000 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.2.5 Received-SPF: none (ns1.primenet.com.au: domain at closedmail.com does not designate permitted sender hosts) From: Bart Schaefer Message-id: <091116200143.ZM31202@torch.brasslantern.com> Date: Mon, 16 Nov 2009 20:01:43 -0800 In-reply-to: Comments: In reply to Greg Klanderman "Re: minor niggle with svn completion of sub-commands" (Nov 16, 2:45pm) References: <19192.17676.966894.118536@gargle.gargle.HOWL> <20091112192553.2d76ce1d@pws-pc> <091115161750.ZM29484@torch.brasslantern.com> X-Mailer: OpenZMail Classic (0.9.2 24April2005) To: zsh-workers@zsh.org Subject: Re: minor niggle with svn completion of sub-commands MIME-version: 1.0 Content-type: text/plain; charset=us-ascii 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".