Re: [PATCH 3/3] completion: improve shell expansion of items
On Thu, Sep 27, 2012 at 02:43:38AM -0400, Jeff King wrote:
> Here are the numbers using sed[1]
> instead:
> -# Quotes each element of an IFS-delimited list for shell reuse
> +# Quotes each element of a newline-delimited list for shell reuse
> __git_quote()
> {
> - local i
> - local delim
> - for i in $1; do
> - local quoted=${i//\'/\'\\\'\'}
> - printf "${delim:+$IFS}'%s'" "$quoted"
> - delim=t
> - done
> + echo "$1" |
> + sed "
> + s/'/'''/g
> + s/^/'/
> + s/$/'/
> + "
> }
>
> # Generates completion reply with compgen, appending a space to possible
Your usage of sed got me thinking and come up with this.
The first two fix benign bugs in completion tests, and the third adds
tests for __gitcomp_nl(). These are good to go.
The fourth adds __gitcomp() and __gitcomp_nl() tests for this
expansion breakage. The expected results might need some adjustments,
because they contain special characters unquoted, but I'm tempted to
say that a branch called $foo is so rare in practice, that we
shouldn't bother.
The final one is a proof of concept for inspiration. It gets rid of
compgen and its crazy additional expansion replacing it with a small
sed script. It needs further work to handle words with whitespaces
and special characters.
SZEDER Gábor (5):
completion: fix non-critical bugs in __gitcomp() tests
completion: fix args of run_completion() test helper
completion: add tests for the __gitcomp_nl() completion helper
function
completion: test __gitcomp() and __gitcomp_nl() with expandable words
completion: avoid compgen to fix expansion issues in __gitcomp_nl()
contrib/completion/git-completion.bash | 6 ++-
t/t9902-completion.sh | 91 +++---
2 files changed, 90 insertions(+), 7 deletions(-)
--
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 3/3] completion: improve shell expansion of items
On Thu, Sep 27, 2012 at 02:43:38AM -0400, Jeff King wrote:
> Ah. The problem is that most of the load comes from my patch 4/3, which
> does a separate iteration. Here are the numbers after just patch 3:
>
> $ time __gitcomp_nl "$refs"
> real0m0.344s
> user0m0.392s
> sys 0m0.040s
>
> Slower, but not nearly as painful. Here are the numbers using sed[1]
> instead:
>
> $ time __gitcomp_nl "$refs"
> real0m0.100s
> user0m0.084s
> sys 0m0.016s
>
> So a little slower, but probably acceptable. We could maybe do the same
> trick on the output side (although it is a little trickier there,
> because we need it in a bash array). Of course, this is Linux; the fork
> for sed is way more expensive on some systems.
So something like the patch below does the quoting correctly (try
committing a file like "name with spaces" and doing "git show
HEAD:"), and isn't too much slower:
real0m0.114s
user0m0.108s
sys 0m0.004s
That's almost double the time without handling quoting, but keep in mind
this is on 10,000 entries (and the real sluggishness we are trying to
avoid is an order of magnitude). So it might be acceptable.
This is just a proof-of-concept patch. We'd probably want to replace the
perl below with a more complicated sed invocation for portability (the
trickiness is that the output is shown to the user, so we very much
don't want to quote anything that does not have to be).
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index be800e0..20c09ef 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -225,6 +225,17 @@ fi
fi
fi
+# Quotes each element of a newline-delimited list for shell reuse
+__git_quote()
+{
+ echo "$1" |
+ sed "
+ s/'/'''/g
+ s/^/'/
+ s/$/'/
+ "
+}
+
# Generates completion reply with compgen, appending a space to possible
# completion words, if necessary.
# It accepts 1 to 4 arguments:
@@ -261,7 +272,10 @@ __gitcomp_nl ()
__gitcomp_nl ()
{
local IFS=$'\n'
- COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
+ COMPREPLY=($(
+ compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" --
"${3-$cur}" |
+ perl -lne '/(.*?)( ?)$/; print quotemeta($1), $2'
+ ))
}
__git_heads ()
-Peff
--
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 3/3] completion: improve shell expansion of items
On Thu, Sep 27, 2012 at 02:28:55AM -0400, Jeff King wrote:
> Thanks for reminding me to time. I noticed your a31e626 while digging in
> the history, but forgot that I wanted to do a timing test. Sadly, the
> results are very discouraging. Doing a similar test to your 10,000-refs,
> I get:
>
> $ refs=$(seq 1 1)
>
> $ . git-completion.bash.old
> $ time __gitcomp_nl "$refs"
> real0m0.065s
> user0m0.064s
> sys 0m0.004s
>
> $ . git-completion.bash.new
> $ time __gitcomp_nl "$refs"
> real0m1.799s
> user0m1.828s
> sys 0m0.036s
>
> So, a 2700% slowdown. Yuck.
>
> I also tried running it through sed instead of iterating in bash. I know
> that some people will not like the fork, but curiously enough, it was
> not that much faster. Which makes me wonder if part of the slowdown is
> actually bash unquoting the result in compgen.
Ah. The problem is that most of the load comes from my patch 4/3, which
does a separate iteration. Here are the numbers after just patch 3:
$ time __gitcomp_nl "$refs"
real0m0.344s
user0m0.392s
sys 0m0.040s
Slower, but not nearly as painful. Here are the numbers using sed[1]
instead:
$ time __gitcomp_nl "$refs"
real0m0.100s
user0m0.084s
sys 0m0.016s
So a little slower, but probably acceptable. We could maybe do the same
trick on the output side (although it is a little trickier there,
because we need it in a bash array). Of course, this is Linux; the fork
for sed is way more expensive on some systems.
Still, I'd be curious to see it with the callers (e.g., __git_refs)
doing their own quoting. I'd worry that it would become a maintenance
headache, but perhaps we don't have that many lists we feed (there are a
lot of calls to gitcomp_nl, but they are mostly feeding __git_refs).
-Peff
[1] For reference, that patch is:
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 1fc43f7..5ff3742 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -225,16 +225,15 @@ __git_quote()
fi
fi
-# Quotes each element of an IFS-delimited list for shell reuse
+# Quotes each element of a newline-delimited list for shell reuse
__git_quote()
{
- local i
- local delim
- for i in $1; do
- local quoted=${i//\'/\'\\\'\'}
- printf "${delim:+$IFS}'%s'" "$quoted"
- delim=t
- done
+ echo "$1" |
+ sed "
+ s/'/'''/g
+ s/^/'/
+ s/$/'/
+ "
}
# Generates completion reply with compgen, appending a space to possible
--
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 3/3] completion: improve shell expansion of items
On Thu, Sep 27, 2012 at 02:37:51AM +0200, SZEDER Gábor wrote:
> > +# Quotes each element of an IFS-delimited list for shell reuse
> > +__git_quote()
> > +{
> > + local i
> > + local delim
> > + for i in $1; do
> > + local quoted=${i//\'/\'\\\'\'}
> > + printf "${delim:+$IFS}'%s'" "$quoted"
> > + delim=t
> > + done
> > +}
> [...]
>
> Iterating through all possible completion words undermines the main
> reason for __gitcomp_nl()'s existence: to handle potentially large
> number of possible completion words faster than the old __gitcomp().
> If we really have to iterate in a subshell, then it would perhaps be
> better to drop __gitcomp_nl(), go back to using __gitcomp(), and
> modify that instead.
Thanks for reminding me to time. I noticed your a31e626 while digging in
the history, but forgot that I wanted to do a timing test. Sadly, the
results are very discouraging. Doing a similar test to your 10,000-refs,
I get:
$ refs=$(seq 1 1)
$ . git-completion.bash.old
$ time __gitcomp_nl "$refs"
real0m0.065s
user0m0.064s
sys 0m0.004s
$ . git-completion.bash.new
$ time __gitcomp_nl "$refs"
real0m1.799s
user0m1.828s
sys 0m0.036s
So, a 2700% slowdown. Yuck.
I also tried running it through sed instead of iterating in bash. I know
that some people will not like the fork, but curiously enough, it was
not that much faster. Which makes me wonder if part of the slowdown is
actually bash unquoting the result in compgen.
> After all, anyone could drop a file called git-cmd-${meta} on his
> $PATH, and then get cmd- offered, because completion of git commands
> still goes through __gitcomp().
Yeah. I wasn't sure if __gitcomp() actually got used on any arbitrary
data, but that's a good example.
I'm not sure where to go next. I guess we could try pre-quoting via git
when generating the list of refs (or files, or whatever) and hope that
is faster.
With this patch as it is, I'm not sure the slowdown is worth it. Yes,
it's more correct in the face of metacharacters, but those are the
uncommon case by a large margin. I'd hate to have a performance
regression in the common case just for that.
-Peff
--
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 3/3] completion: improve shell expansion of items
Hi,
On Wed, Sep 26, 2012 at 05:51:19PM -0400, Jeff King wrote:
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index be800e0..b0416ea 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -225,6 +225,18 @@ fi
> fi
> fi
>
> +# Quotes each element of an IFS-delimited list for shell reuse
> +__git_quote()
> +{
> + local i
> + local delim
> + for i in $1; do
> + local quoted=${i//\'/\'\\\'\'}
> + printf "${delim:+$IFS}'%s'" "$quoted"
> + delim=t
> + done
> +}
> +
> # Generates completion reply with compgen, appending a space to possible
> # completion words, if necessary.
> # It accepts 1 to 4 arguments:
> @@ -261,7 +273,7 @@ __gitcomp_nl ()
> __gitcomp_nl ()
> {
> local IFS=$'\n'
> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" --
> "${3-$cur}"))
Iterating through all possible completion words undermines the main
reason for __gitcomp_nl()'s existence: to handle potentially large
number of possible completion words faster than the old __gitcomp().
If we really have to iterate in a subshell, then it would perhaps be
better to drop __gitcomp_nl(), go back to using __gitcomp(), and
modify that instead.
After all, anyone could drop a file called git-cmd-${meta} on his
$PATH, and then get cmd- offered, because completion of git commands
still goes through __gitcomp().
--
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 3/3] completion: improve shell expansion of items
The current completion code doesn't deal properly with items
(tags, branches, etc.) that have ${} in them because they
get expanded by bash while using compgen.
This patch is a rewrite of Felipe Contreras's 25ae7cf, which
attempted to fix the problem by quoting the values we pass
to __gitcomp_nl. However, that patch ended up quoting the
whole list as a single item, which broke other completions.
Instead, we need to split the newline-delimited list into
elements and quote each one individually.
This is not a complete fix, as the completion result does
will still contain metacharacters, so it would need extra
quoting to actually be used on a command line. But it's
still a step in the right direction, because:
1. The current code's expansion means that things that are
broken expansions (e.g., "${foo:bar}") will actually
cause the completion function to barf, breaking all
completion (even if you weren't going to complete that
item). This patch fixes that so you can at least
complete "foo" when "${foo:bar}" exists.
2. We don't know yet what the final fix will look like,
but this is probably the first step towards it. It
handles quoting on the input side of compgen; the next
step will likely be handling the quoting on the output
side of compgen to yield a usable string for the
command line.
Signed-off-by: Jeff King
---
contrib/completion/git-completion.bash | 14 +-
t/t9902-completion.sh | 2 +-
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index be800e0..b0416ea 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -225,6 +225,18 @@ fi
fi
fi
+# Quotes each element of an IFS-delimited list for shell reuse
+__git_quote()
+{
+ local i
+ local delim
+ for i in $1; do
+ local quoted=${i//\'/\'\\\'\'}
+ printf "${delim:+$IFS}'%s'" "$quoted"
+ delim=t
+ done
+}
+
# Generates completion reply with compgen, appending a space to possible
# completion words, if necessary.
# It accepts 1 to 4 arguments:
@@ -261,7 +273,7 @@ __gitcomp_nl ()
__gitcomp_nl ()
{
local IFS=$'\n'
- COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
+ COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" --
"${3-$cur}"))
}
__git_heads ()
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cbd0fb6..da67705 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -278,7 +278,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.7.12.10.g31da6dd
--
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
