From: "SZEDER Gábor" <szeder@ira.uka.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Shawn O. Pearce" <spearce@spearce.org>,
Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH 2/9] completion: optimize refs completion
Date: Thu, 13 Oct 2011 12:40:47 +0200 [thread overview]
Message-ID: <20111013104047.GA15379@goldbirke> (raw)
In-Reply-To: <7v7h497m01.fsf@alter.siamese.dyndns.org>
On Wed, Oct 12, 2011 at 05:50:38PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder@ira.uka.de> 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
next prev parent reply other threads:[~2011-10-13 10:40 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
2011-10-08 14:54 ` [PATCH 1/9] completion: document __gitcomp() SZEDER Gábor
2011-10-08 14:54 ` [PATCH 2/9] completion: optimize refs completion SZEDER Gábor
2011-10-13 0:50 ` Junio C Hamano
2011-10-13 10:40 ` SZEDER Gábor [this message]
2011-10-13 17:28 ` Junio C Hamano
2011-10-14 12:16 ` SZEDER Gábor
2011-10-15 12:57 ` [PATCH 2/9 v2] " SZEDER Gábor
2011-10-16 3:29 ` Junio C Hamano
2011-10-08 14:54 ` [PATCH 3/9] completion: make refs completion consistent for local and remote repos SZEDER Gábor
2011-10-08 14:54 ` [PATCH 4/9] completion: improve ls-remote output filtering in __git_refs() SZEDER Gábor
2011-10-08 14:54 ` [PATCH 5/9] completion: support full refs from remote repositories SZEDER Gábor
2011-10-08 14:54 ` [PATCH 6/9] completion: query only refs/heads/ in __git_refs_remotes() SZEDER Gábor
2011-10-08 14:54 ` [PATCH 7/9] completion: improve ls-remote output filtering " SZEDER Gábor
2011-10-08 14:54 ` [PATCH 8/9] completion: fast initial completion for config 'remote.*.fetch' value SZEDER Gábor
2011-10-08 14:54 ` [PATCH 9/9] completion: remove broken dead code from __git_heads() and __git_tags() SZEDER Gábor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20111013104047.GA15379@goldbirke \
--to=szeder@ira.uka.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=spearce@spearce.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).