[pacman-dev] [PATCH] [bash_completion] Undeclared local vars/filenames with spaces/other
Fix:
3 Undeclared local vars with common enough names to warrant breakage
Performance issues with _pacman trying to replicate /usr/bin/pacman
with find and other slow tools.
Performance issues with expanding an array (with sometimes hundreds of
items) over three times.
Expanding said array to remove already completed entries had the side
effect of braking filenames with spaces and or \n.
The full description of fixes are already posted at:
http://bugs.archlinux.org/task/16630#comment55779
along with time diffs.
Signed-off-by: Andres P
On Tue, Jan 19, 2010 at 11:26 PM, Andres Perera
Fix: 3 Undeclared local vars with common enough names to warrant breakage
Performance issues with _pacman trying to replicate /usr/bin/pacman with find and other slow tools.
Performance issues with expanding an array (with sometimes hundreds of items) over three times.
Expanding said array to remove already completed entries had the side effect of braking filenames with spaces and or \n.
The full description of fixes are already posted at: http://bugs.archlinux.org/task/16630#comment55779 along with time diffs.
Signed-off-by: Andres P
--- First off, thanks. These fixes sound much needed.
contrib/bash_completion | 535 ++++++++++++++++++----------------------------- 1 files changed, 202 insertions(+), 333 deletions(-)
diff --git a/contrib/bash_completion b/contrib/bash_completion index 62e5bc9..65051f5 100644 --- a/contrib/bash_completion +++ b/contrib/bash_completion @@ -1,365 +1,234 @@ -# vim: set ft=sh ts=2 sw=2 et: -# file: /etc/bash_completion.d/pacman - -# Bash completion for pacman -# Original: Manolis Tzanidakis
+# pacman/makepkg completion by Andres Perera <andres87p gmail> # -# Distributed under the terms of the GNU General Public License, v2 or later. +# Distributed under the terms of the GNU General Public License v3 or +# later. Is this strictly necessary? You are changing one piece of code in the entire codebase to require v3 or later and as weird as it sounds, I am not going to let that fly. I won't be merging this unless it is v2 or you give some darn good reasons why we should move to v3.
For the rest of the patch, I think I am just going to test it out locally and then I'll get back to you if I don't see anything that blows up as you are right in it being a total revision of the original. -Dan
On Wed, Jan 20, 2010 at 1:07 AM, Dan McGee
Is this strictly necessary? You are changing one piece of code in the entire codebase to require v3 or later and as weird as it sounds, I am not going to let that fly. I won't be merging this unless it is v2 or you give some darn good reasons why we should move to v3.
Not really necessary. You can change it to v2, or I can do that if it requires a commit from me.
On Tue, Jan 19, 2010 at 11:48 PM, Andres Perera
On Wed, Jan 20, 2010 at 1:07 AM, Dan McGee
wrote: Is this strictly necessary? You are changing one piece of code in the entire codebase to require v3 or later and as weird as it sounds, I am not going to let that fly. I won't be merging this unless it is v2 or you give some darn good reasons why we should move to v3.
Not really necessary. You can change it to v2, or I can do that if it requires a commit from me.
Nah that's good enough for me. I'll try to give this whole thing a workout in the next day or two and see if I can come up with anything else that we might want to tweak, however. -Dan
Uh, I broke the spaces with git's 72 textwidth. Same patch attached...
On Wed, Jan 20, 2010 at 9:27 AM, Andres Perera
Uh, I broke the spaces with git's 72 textwidth.
Same patch attached...
I don't understand what you mean, this is the diff between original and new patch : diff --git a/contrib/bash_completion b/contrib/bash_completion index 2f6cd06..b1162ad 100644 --- a/contrib/bash_completion +++ b/contrib/bash_completion @@ -1,6 +1,6 @@ # pacman/makepkg completion by Andres Perera <andres87p gmail> # -# Distributed under the terms of the GNU General Public License v3 or +# Distributed under the terms of the GNU General Public License v2 or # later. # # Local variables: common core cur glob list long m o prev query r @@ -56,7 +56,7 @@ _makepkg_count_words() { _makepkg() { COMPREPLY=() - local prev cur short long parse glob + local prev cur short long prev=${COMP_WORDS[COMP_CWORD-1]} cur=`_get_cword` I see you reverted the license and made another change, but no space change. I am not sure what you mean with git's 72 textwidth , isnt that just commit log ? Your original commit log was fine. Also it's much easier for us if you keep submitting git patches with the log. And finally the patch now only applies against maint, not master, but maybe that's our fault of not applying it earlier :)
On 06/08/10 at 09:54:16 CEST, Xavier Chantry <shiningxc at gmail.com> wrote:
On Wed, Jan 20, 2010 at 9:27 AM, Andres Perera
wrote: Uh, I broke the spaces with git's 72 textwidth.
Same patch attached...
I don't understand what you mean, this is the diff between original and new patch :
diff --git a/contrib/bash_completion b/contrib/bash_completion index 2f6cd06..b1162ad 100644 --- a/contrib/bash_completion +++ b/contrib/bash_completion @@ -1,6 +1,6 @@ # pacman/makepkg completion by Andres Perera <andres87p gmail> # -# Distributed under the terms of the GNU General Public License v3 or +# Distributed under the terms of the GNU General Public License v2 or # later. # # Local variables: common core cur glob list long m o prev query r @@ -56,7 +56,7 @@ _makepkg_count_words() { _makepkg() { COMPREPLY=()
- local prev cur short long parse glob + local prev cur short long prev=${COMP_WORDS[COMP_CWORD-1]} cur=`_get_cword`
I see you reverted the license and made another change, but no space change. I am not sure what you mean with git's 72 textwidth , isnt that just commit log ? Your original commit log was fine. Also it's much easier for us if you keep submitting git patches with
Oh, I was under the impression I broke spaces. Maybe it was the gmail client playing tricks on me the log.
And finally the patch now only applies against maint, not master, but maybe that's our fault of not applying it earlier :)
To be honest I'm not really satisfied with that patch anymore, since it could be made even more simple and faster. I also had put an option that made dirs show twice (+plusdirs), ie a new bug. I'll try and see if I can get something clean against master, Andres
participants (4)
-
Andres P
-
Andres Perera
-
Dan McGee
-
Xavier Chantry