On Wed, Oct 12, 2011 at 05:50:38PM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder@xxxxxxxxxx> writes: > > > After a unique command or option is completed, in most cases it is a > > good thing to add a trailing a space, but sometimes it doesn't makes > > s/makes/make/; > > > __gitcomp() therefore iterates over all possible completion words it > > got as argument, and checks each word whether a trailing space is > > necessary or not. This is ok for commands, options, etc., i.e. when > > the number of words is relatively small, but can be noticeably slow > > for large number of refs. However, while options might or might not > > need that trailing space, refs are always handled uniformly and always > > get that trailing space (or a trailing '.' for 'git config > > branch.<head>.'). > > ... > > So, add a specialized variant of __gitcomp() that only deals with > > possible completion words separated by a newline and uniformly appends > > the trailing space to all words using 'compgen -S' (or any other > > suffix, if specified), so no iteration over all words is done. > > s/is done./is needed./; > > I think I followed your logic (very well written ;-) Thanks; learned it around here ;) > but feel somewhat > dirty, as you are conflating the "These things are separated with newlines" > with "These things do not need inspection --- they all need suffix", which > has one obvious drawback --- you may find other class of words that always > want a SP after each of them but the source that generates such a class of > words may not separate the list elements with a newline. Yes, there are a couple of other places where SP is uniformly needed, for example completion of subcommands for bisect, notes, stash, etc., merge strategies, whitespace options, which are all separated by SP, or help topics, which are separated by SP, TAB, and NL. However, it really is necessary that no SP is used to separate those words, see below, so we can't use this optimization in these cases. And since the number of possible completion words in these cases is usually low, it doesn't worth the effort to restructure those words to not use SP separator, because it doesn't really make a performance difference anyway. > Because a ref cannot have $IFS whitespace in its name anyway, I think you > can rename __gitcomp_nl to a name that conveys more clearly what it does > (i.e. "complete and always append suffix"), drop the IFS fiddling from the > function, and get the same optimization, no? Unfortunately, this optimization depends on the IFS fiddling, because we want to append a SP. The same IFS trick is done in __gitcomp(), too. If we use the default IFS containing an SP and append a SP to possible completion words by 'compgen -S " "' (or by word="$word ", as in __gitcomp_1()), then that SP will be promply stripped off when compgen's output is stored in the COMPREPLY array. Using an IFS without SP keeps those SP suffixes. Perhaps I should've mentioned this explicitly in the commit message, but didn't do so because one of the referenced commit messages (72e5e989 (bash: Add space after unique command name is completed., 2007-02-04)) already mentioned it briefly. But when we use an IFS without SP, that also implies that we can't pass words separated by SP to __gitcomp_nl(), because those words won't be split at SPs anymore. Since refs & co. are separated by NL, it was the obvious choice for this special-purpose IFS. So this optimization can't work with the class of words mentioned above. So I thought that it's important to stress that this function can only deal with NL separated words, hence I named it __gitcomp_nl(). But I see your point about naming it after what it actually does, so I'm fine with __gitcomp_add_suffix() or whatever else that indicates "complete and always append suffix". Will resend in a day or two, to leave some time for other suggestions. Best, Gábor -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html