Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

2012-11-17 Thread Felipe Contreras 
On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor  wrote:

>  __gitcomp_nl ()
>  {
> local IFS=$'\n'
> -   COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> +   COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
> +   BEGIN {
> +   FS="\n";
> +   len=length(cur);
> +   }
> +   {
> +   if (cur == substr($1, 1, len))
> +   print pfx$1sfx;
> +   }' <<< "$1" ))
>  }

This version is simpler and faster:

local IFS=$'\n'
COMPREPLY=($(awk -v cur="${3-$cur}" -v pre="${2-}" -v suf="${4- }"
'$0 ~ cur { print pre$0suf }' <<< "$1" ))

== 1 ==
awk 1:
real0m0.067s
user0m0.066s
sys 0m0.001s
awk 2:
real0m0.057s
user0m0.055s
sys 0m0.002s

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

2012-11-17 Thread Felipe Contreras 
On Sat, Nov 17, 2012 at 8:28 PM, Felipe Contreras
 wrote:
> On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras
>  wrote:
>> On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor  wrote:
>>> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
 On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor  wrote:

 >  __gitcomp_nl ()
 >  {
 > local IFS=$'\n'
 > -   COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- 
 > "${3-$cur}"))
 > +   COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v 
 > cur="${3-$cur}" '
 > +   BEGIN {
 > +   FS="\n";
 > +   len=length(cur);
 > +   }
 > +   {
 > +   if (cur == substr($1, 1, len))
 > +   print pfx$1sfx;
 > +   }' <<< "$1" ))
 >  }

 Does this really perform better than my alternative?

 +   for x in $1; do
 +   if [[ "$x" = "$3"* ]]; then
 +   COMPREPLY+=("$2$x$4")
 +   fi
 +   done
>>>
>>> It does:
>>>
>>>   My version:
>>>
>>> $ refs="$(for i in {0..} ; do echo branch$i ; done)"
>>> $ time __gitcomp_nl "$refs"
>>>
>>> real0m0.109s
>>> user0m0.096s
>>> sys 0m0.012s
>>>
>>>   Yours:
>>>
>>> $ time __gitcomp_nl "$refs"
>>>
>>> real0m0.321s
>>> user0m0.312s
>>> sys 0m0.008s
>>
>> Yeah, for 1 refs, which is not the common case:
>
> And this beats both in every case:
>
> while read -r x; do
> if [[ "$x" == "$3"* ]]; then
> COMPREPLY+=("$2$x$4")
> fi
> done <<< $1

Nevermind that.

Here's another:

local IFS=$'\n'
COMPREPLY=($(printf -- "$2%s$4\n" $1 | grep "^$2$3"))

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

2012-11-17 Thread Felipe Contreras 
On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras
 wrote:
> On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor  wrote:
>> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
>>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor  wrote:
>>>
>>> >  __gitcomp_nl ()
>>> >  {
>>> > local IFS=$'\n'
>>> > -   COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- 
>>> > "${3-$cur}"))
>>> > +   COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v 
>>> > cur="${3-$cur}" '
>>> > +   BEGIN {
>>> > +   FS="\n";
>>> > +   len=length(cur);
>>> > +   }
>>> > +   {
>>> > +   if (cur == substr($1, 1, len))
>>> > +   print pfx$1sfx;
>>> > +   }' <<< "$1" ))
>>> >  }
>>>
>>> Does this really perform better than my alternative?
>>>
>>> +   for x in $1; do
>>> +   if [[ "$x" = "$3"* ]]; then
>>> +   COMPREPLY+=("$2$x$4")
>>> +   fi
>>> +   done
>>
>> It does:
>>
>>   My version:
>>
>> $ refs="$(for i in {0..} ; do echo branch$i ; done)"
>> $ time __gitcomp_nl "$refs"
>>
>> real0m0.109s
>> user0m0.096s
>> sys 0m0.012s
>>
>>   Yours:
>>
>> $ time __gitcomp_nl "$refs"
>>
>> real0m0.321s
>> user0m0.312s
>> sys 0m0.008s
>
> Yeah, for 1 refs, which is not the common case:

And this beats both in every case:

while read -r x; do
if [[ "$x" == "$3"* ]]; then
COMPREPLY+=("$2$x$4")
fi
done <<< $1

== 1 ==
one:
real0m0.004s
user0m0.003s
sys 0m0.000s
two:
real0m0.000s
user0m0.000s
sys 0m0.000s
== 10 ==
one:
real0m0.005s
user0m0.002s
sys 0m0.002s
two:
real0m0.000s
user0m0.000s
sys 0m0.000s
== 100 ==
one:
real0m0.005s
user0m0.004s
sys 0m0.000s
two:
real0m0.001s
user0m0.000s
sys 0m0.000s
== 1000 ==
one:
real0m0.010s
user0m0.008s
sys 0m0.001s
two:
real0m0.004s
user0m0.004s
sys 0m0.000s
== 1 ==
one:
real0m0.061s
user0m0.057s
sys 0m0.005s
two:
real0m0.045s
user0m0.044s
sys 0m0.000s
== 10 ==
one:
real0m0.582s
user0m0.575s
sys 0m0.022s
two:
real0m0.487s
user0m0.482s
sys 0m0.004s
== 100 ==
one:
real0m6.305s
user0m6.284s
sys 0m0.216s
two:
real0m5.268s
user0m5.194s
sys 0m0.065s

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

2012-11-17 Thread Felipe Contreras 
On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor  wrote:
> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor  wrote:
>>
>> >  __gitcomp_nl ()
>> >  {
>> > local IFS=$'\n'
>> > -   COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- 
>> > "${3-$cur}"))
>> > +   COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" 
>> > '
>> > +   BEGIN {
>> > +   FS="\n";
>> > +   len=length(cur);
>> > +   }
>> > +   {
>> > +   if (cur == substr($1, 1, len))
>> > +   print pfx$1sfx;
>> > +   }' <<< "$1" ))
>> >  }
>>
>> Does this really perform better than my alternative?
>>
>> +   for x in $1; do
>> +   if [[ "$x" = "$3"* ]]; then
>> +   COMPREPLY+=("$2$x$4")
>> +   fi
>> +   done
>
> It does:
>
>   My version:
>
> $ refs="$(for i in {0..} ; do echo branch$i ; done)"
> $ time __gitcomp_nl "$refs"
>
> real0m0.109s
> user0m0.096s
> sys 0m0.012s
>
>   Yours:
>
> $ time __gitcomp_nl "$refs"
>
> real0m0.321s
> user0m0.312s
> sys 0m0.008s

Yeah, for 1 refs, which is not the common case:

== 1 ==
SZEDER:
real0m0.007s
user0m0.003s
sys 0m0.000s
felipec:
real0m0.000s
user0m0.000s
sys 0m0.000s
== 10 ==
SZEDER:
real0m0.004s
user0m0.003s
sys 0m0.001s
felipec:
real0m0.000s
user0m0.000s
sys 0m0.000s
== 100 ==
SZEDER:
real0m0.005s
user0m0.002s
sys 0m0.002s
felipec:
real0m0.002s
user0m0.002s
sys 0m0.000s
== 1000 ==
SZEDER:
real0m0.010s
user0m0.008s
sys 0m0.001s
felipec:
real0m0.018s
user0m0.017s
sys 0m0.001s
== 1 ==
SZEDER:
real0m0.062s
user0m0.060s
sys 0m0.003s
felipec:
real0m0.175s
user0m0.174s
sys 0m0.000s
== 10 ==
SZEDER:
real0m0.595s
user0m0.593s
sys 0m0.021s
felipec:
real0m1.848s
user0m1.843s
sys 0m0.003s
== 100 ==
SZEDER:
real0m6.258s
user0m6.241s
sys 0m0.215s
felipec:
real0m18.191s
user0m18.115s
sys 0m0.045s

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

2012-11-17 Thread SZEDER Gábor 
On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor  wrote:
> 
> >  __gitcomp_nl ()
> >  {
> > local IFS=$'\n'
> > -   COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> > +   COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
> > +   BEGIN {
> > +   FS="\n";
> > +   len=length(cur);
> > +   }
> > +   {
> > +   if (cur == substr($1, 1, len))
> > +   print pfx$1sfx;
> > +   }' <<< "$1" ))
> >  }
> 
> Does this really perform better than my alternative?
> 
> +   for x in $1; do
> +   if [[ "$x" = "$3"* ]]; then
> +   COMPREPLY+=("$2$x$4")
> +   fi
> +   done

It does:

  My version:

$ refs="$(for i in {0..} ; do echo branch$i ; done)"
$ time __gitcomp_nl "$refs"

real0m0.109s
user0m0.096s
sys 0m0.012s

  Yours:

$ time __gitcomp_nl "$refs"

real0m0.321s
user0m0.312s
sys 0m0.008s

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

2012-11-17 Thread Felipe Contreras 
On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor  wrote:

>  __gitcomp_nl ()
>  {
> local IFS=$'\n'
> -   COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> +   COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
> +   BEGIN {
> +   FS="\n";
> +   len=length(cur);
> +   }
> +   {
> +   if (cur == substr($1, 1, len))
> +   print pfx$1sfx;
> +   }' <<< "$1" ))
>  }

Does this really perform better than my alternative?

+   for x in $1; do
+   if [[ "$x" = "$3"* ]]; then
+   COMPREPLY+=("$2$x$4")
+   fi
+   done

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

2012-11-17 Thread SZEDER Gábor 
The compgen Bash-builtin performs expansion on all words in the
wordlist given to its -W option, breaking Git's completion script when
refs or filenames passed to __gitcomp_nl() contain expandable
substrings.  At least one user can't use ref completion at all in a
repository, which contains tags with metacharacters like

  Build%20V%20${bamboo.custom.jiraversion.name}%20Build%20721

Such a ref causes a failure in compgen as it tries to expand the
variable with invalid name.

Unfortunately, compgen has no option to turn off this expansion.
However, in __gitcomp_nl() we use only a small subset of compgen's
functionality, namely the filtering of matching possible completion
words and adding a prefix and/or suffix to each of those words.  So,
to avoid compgen's problematic expansion we get rid of compgen, and
implement the equivalent functionality in a small awk script.  The
reason for using awk instead of sed is that awk allows plain (i.e.
non-regexp) string comparison, making quoting of regexp characters in
the current word unnecessary.

Note, that such expandable words need quoting to be of any use on the
command line.  This patch doesn't perform that quoting, but it could
be implemented later on top by extending the awk script to unquote
$cur and to quote matching words.  Nonetheless, this patch still a
step forward, because at least refs can be completed even in the
presence of one with metacharacters.

Also update the function's description a bit.

Reported-by: Jeroen Meijer 
Signed-off-by: SZEDER Gábor 
---

awk doesn't have a prefixcmp().  I'm no awk wizard, so I'm not sure this
is the right way to do it.

 contrib/completion/git-completion.bash | 17 +
 t/t9902-completion.sh  |  4 ++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index bc0657a2..65196ddd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -249,19 +249,28 @@ __gitcomp ()
esac
 }
 
-# Generates completion reply with compgen from newline-separated possible
-# completion words by appending a space to all of them.
+# Generates completion reply for the word in $cur from newline-separated
+# possible completion words, appending a space to all of them.
 # It accepts 1 to 4 arguments:
 # 1: List of possible completion words, separated by a single newline.
 # 2: A prefix to be added to each possible completion word (optional).
-# 3: Generate possible completion matches for this word (optional).
+# 3: Generate possible completion matches for this word instead of $cur
+#(optional).
 # 4: A suffix to be appended to each possible completion word instead of
 #the default space (optional).  If specified but empty, nothing is
 #appended.
 __gitcomp_nl ()
 {
local IFS=$'\n'
-   COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
+   COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
+   BEGIN {
+   FS="\n";
+   len=length(cur);
+   }
+   {
+   if (cur == substr($1, 1, len))
+   print pfx$1sfx;
+   }' <<< "$1" ))
 }
 
 __git_heads ()
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a108ec1c..fa289324 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -246,7 +246,7 @@ test_expect_success '__gitcomp_nl - no suffix' '
test_cmp expected out
 '
 
-test_expect_failure '__gitcomp_nl - doesnt fail because of invalid variable 
name' '
+test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable 
name' '
(
__gitcomp_nl "$invalid_variable_name"
)
@@ -383,7 +383,7 @@ test_expect_success 'complete tree filename with spaces' '
EOF
 '
 
-test_expect_failure 'complete tree filename with metacharacters' '
+test_expect_success 'complete tree filename with metacharacters' '
echo content >"name with \${meta}" &&
git add . &&
git commit -m meta &&
-- 
1.8.0.220.g4d14ece

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html