Bug 475229

Summary: Implement catch-all programmable completion handler
Product: [Fedora] Fedora Reporter: Behdad Esfahbod <behdad>
Component: bashAssignee: Roman Rakus <rrakus>
Status: CLOSED UPSTREAM QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fred, fvulto, guillomovitch, rhughes, rrakus, rstrode, siegfried, tsmetana, twaugh, ville.skytta
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-04-27 09:07:52 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Attachments:
Description Flags
The patch
none
Updated patch
none
One last time today
none
Updated patch none

Description Behdad Esfahbod 2008-12-08 10:50:31 EST
bash-completion is the niftiest thing since Fedora itself.  The implementation sucks though: Every interactive shell sources 200kb of shell scripts and keeps it in memory, consuming lots of memory, slowing startup down, and cluttering "set" output.

I sat down to fix this.  In fact have wanted to do for 5 years.  I had lots of ideas about how the actual function contents can be sourced when needed from inside a function stub, etc, but they all had the problem that the actual "complete -F funcname cmdname" still had to be read by the shell upfront for every command.

So, to fix that, I implemented a catch-all handler.  The patch that I'm going to attach does the following:

  - A handler installed for the empty string will be a catch all handler.  That is, something like:

    complete -F __loader ''

  - When trying to complete for a command, we do:

    * Try the completion set for command
    * Try the completion set for `basename command`
    * Try catch-all completion
    * Do default bash completion

So, step three is new.

  - Also new that function handlers can now communicate a "failed, retry" mode.  That happens when they return 127.  When a function handler return 127, this is what we do:

    * Check to see if the installed handler for the current command has changed.  If yes, retry with the new handler.
    * If not, fail and fall back to the next method


This way, a package like bash completion can do something as simple as:

function __bash_completion_loader() {
  source "/etc/bash_completion.d/$1.sh" &>/dev/null
  return 127
}
complete -F __bash_completion_loader ''

That's all!  If a bash-completion spec for the command exists in /etc/bash_completion.sh, that will install a new handler.  The return 127 causes bash to retry with the new handler, and silently pass no to the default bash completion otherwise.  Problem solved!

As a side-effect of these, if you set a complete handler to a non-existent function, after warning, bash now will fall back to the next method (catch-all handler, or default bash completion).


So, do you see any chance of getting this in rawhide and pushing it upward?

Thanks,
Comment 1 Behdad Esfahbod 2008-12-08 10:51:47 EST
Created attachment 326136 [details]
The patch
Comment 2 Behdad Esfahbod 2008-12-08 11:37:34 EST
Created attachment 326142 [details]
Updated patch

The old one didn't correctly do the retry phase.  This one does.
Comment 3 Behdad Esfahbod 2008-12-08 12:04:55 EST
Created attachment 326147 [details]
One last time today

Ok, cleaned up retries.  Now we retry from the beginning of the sequence.  Stop retrying if none of the three cases had changed handlers since last round.
Comment 4 Roman Rakus 2008-12-10 09:00:15 EST
So if I understand it well:
1) completions are loaded onwork as needed?
2) This change speedup system startup, but slowdown bash completion itself. Do you agree?

I don't see any other effects.
Will be startup speedup notable? Or slowdown will be much higher?
Comment 5 Behdad Esfahbod 2008-12-10 11:53:50 EST
Thanks Roman for the reply.

> So if I understand it well:
> 1) completions are loaded onwork as needed?

Yes.

> 2) This change speedup system startup, but slowdown bash completion itself. Do
> you agree?

It doesn't affect system startup as bash-completion only loads on interactive shells.  However, interactive shell startup becomes more than twice faster.  Currently with bash-completion installed:

behdad:~ 0$ time bash -c :

real	0m0.006s
user	0m0.002s
sys	0m0.004s
behdad:~ 0$ time bash -i -c :

real	0m1.247s
user	0m0.722s
sys	0m0.135s

If I remove bash-completion:

behdad:~ 0$ time bash -i -c :

real	0m0.571s
user	0m0.397s
sys	0m0.092s


One should also look at what other craziness is happening in the interactive startup sequence, but bash-completion is taking more time than everything else combined.

> I don't see any other effects.
> Will be startup speedup notable?

The speedup is notable as noted above.


> Or slowdown will be much higher?

A first pass would be to source the current bash-completion implementation the first time that the catch-all handler is called.  That way the first tab will be slow.  However, I plan to refactor bash-completion and chop it into tens or hundreds of files, such that in the catch-all handler we can only load the function needed to complete the command at hand.  That's a win-win scenario, and we won't waste hundreds of kilobytes of memory per login shell either.  (The bash-completion scripts are 200k.  No idea how much memory bash takes to keep them in memory.)

I have already created a git repo for bash-completion on fedorahosted.org.  I expect to start the refactoring this weekend.  Will report here.
Comment 6 Behdad Esfahbod 2008-12-10 11:58:04 EST
One point about the implementation.  Currently I abuse the empty string to install the catch-all handler.  That is:

  complete -F __handler ''

I chose this because it minimized code change, and is already a valid but useless parameter.

I personally would have preferred to use the no-parameter case as default handler.  That is:

  complete -F __handler

That's much more logical and expected.  However, one can't query or remove it as "complete -p" and "complete -r" are documented as listing/removing all installed handlers.
Comment 7 Behdad Esfahbod 2008-12-15 17:30:28 EST
Created attachment 327033 [details]
Updated patch

"complete -p ''" was not quoting its argument correctly.  Fixed that.  This now can be used to detect at run time whether bash supports the catch-all completion, so I can make bash-completion work with both versions of bash with this feature and without.
Comment 8 Freddy Vulto 2009-01-09 03:33:57 EST
Refactoring bash-completion and chopping it into tens or hundreds of files has already been done.  See: http://fvue.nl/wiki/Bash_completion_library

Writing a catch-all handler was the next thing on my list :-), so maybe we can join the efforts?

Regards, Freddy Vulto
Comment 9 Behdad Esfahbod 2009-01-09 08:21:13 EST
(In reply to comment #8)
> Refactoring bash-completion and chopping it into tens or hundreds of files has
> already been done.  See: http://fvue.nl/wiki/Bash_completion_library
> 
> Writing a catch-all handler was the next thing on my list :-), so maybe we can
> join the efforts?
> 
> Regards, Freddy Vulto

Eh, that's awesome!  I'm so happy I had not started the refactoring yet.  I'll study your wiki and contact you with my ideas.

Cheers,
behdad
Comment 10 Guillaume Rousse 2009-02-09 17:56:57 EST
I just found a huge problem with this patch: it apparently changes 'complete -p command' output, by inserting spaces and carriage returns, breaking _command function.

Here is a debug trace of completing on 'sudo chown <TAB>' on a host with the patch applied:
...
+ cmd=chown
+ [[ chown == -* ]]
+ [[ chown == -* ]]
+ done=1
+ '[' -z 1 ']'
+ [[ 1 -eq 0 ]]
+ complete -p chown
++ complete -p chown
+ cspec=' complete -o filenames -F _chown chown
 '
+ '[' '_chown chown
 ' '!=' ' complete -o filenames -F _chown chown
 ' ']'
...

Here is the same trace, on a host without the patch:
...
+ cmd=chown
+ [[ chown == -* ]]
+ [[ chown == -* ]]
+ done=1
+ '[' -z 1 ']'
+ [[ 1 -eq 0 ]]
+ complete -p chown
++ complete -p chown
+ cspec='complete -o filenames -F _chown chown'
+ '[' '_chown chown' '!=' 'complete -o filenames -F _chown chown' ']'
Comment 11 Bug Zapper 2009-06-09 06:10:14 EDT
This bug appears to have been reported against 'rawhide' during the Fedora 11 development cycle.
Changing version to '11'.

More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping
Comment 12 Bug Zapper 2010-04-27 08:29:09 EDT
This message is a reminder that Fedora 11 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 11.  It is Fedora's policy to close all
bug reports from releases that are no longer maintained.  At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '11'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 11's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that 
we may not be able to fix it before Fedora 11 is end of life.  If you 
would still like to see this bug fixed and are able to reproduce it 
against a later version of Fedora please change the 'version' of this 
bug to the applicable version.  If you are unable to change the version, 
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events.  Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

The process we are following is described here: 
http://fedoraproject.org/wiki/BugZappers/HouseKeeping
Comment 13 Roman Rakus 2010-04-27 09:07:52 EDT
Still not fixed in rawhide, but the patch sent upstream.