Re: [PATCH] Revert "completion: fix shell expansion of items"
On Wed, Sep 26, 2012 at 12:37:25AM +0200, SZEDER Gábor wrote: > I looked into this issue, but quickly got lost in quoting-escaping > hell. Ideally we could do some quoting in __gitcomp_nl(), so it would > work for all kinds of input, but I couldn't come up with anything > working. Alternatively, we could modify __gitcomp_nl()'s callers, or > rather the helper functions supplying input to __gitcomp_nl() to do > the quoting or escaping themselves. Actually, that's quite easy for > local refs, at least, because for-each-ref's builtin quoting support > does the trick: I feel like insanity lies that way, because every caller is going to have to do its own quoting. On the other hand, I think it would be the only way to handle completion of entries with embedded newlines (as it is now, we pass in a newline-delimited list with no opportunity for quoting). Here's a simple patch series that fixes the problem and adds a few basic sanity checks. [1/3]: t9902: add a few basic completion tests [2/3]: t9902: add completion tests for "odd" filenames [3/3]: completion: improve shell expansion of items -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] Revert "completion: fix shell expansion of items"
SZEDER Gábor writes:
> On Tue, Sep 25, 2012 at 12:31:19AM -0400, Jeff King wrote:
>> Yeah, doing "git checkout jk/" is not working at all, and I noticed
>> the buggy commit is on the maint track, but has not yet been released.
>> I'm not sure of the solution, but I think we should do this in the
>> meantime:
>>
>> -- >8 --
>> Subject: Revert "completion: fix shell expansion of items"
>
> I agree with the revert, too.
>
> I looked into this issue, but quickly got lost in quoting-escaping
> hell. Ideally we could do some quoting in __gitcomp_nl(), so it would
> work for all kinds of input, but I couldn't come up with anything
> working. Alternatively, we could modify __gitcomp_nl()'s callers, or
> rather the helper functions supplying input to __gitcomp_nl() to do
> the quoting or escaping themselves. Actually, that's quite easy for
> local refs, at least, because for-each-ref's builtin quoting support
> does the trick:
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index c48cd19f..3dc1ec8c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -313,7 +313,7 @@ __git_refs ()
> refs="refs/tags refs/heads refs/remotes"
> ;;
> esac
> - git --git-dir="$dir" for-each-ref --format="%($format)" \
> + git --git-dir="$dir" for-each-ref --shell --format="%($format)"
> \
> $refs
> if [ -n "$track" ]; then
> # employ the heuristic used by git checkout
>
> With this change completion of local refs works well, even in the
> presence of branches ${foo.bar} and foo'bar.
>
> Unfortunately, there are many callsites for __gitcomp_nl(), and it is
> invoked with refs from remote repos, heads, tags, refspecs, remotes,
> config variables, symbols from ctags, or output from stash or
> ls-tree... although some of these are OK as they are now (remotes,
> config variables).
Whatever you do, please make the first step of that endeavour an
addition to the t/t9902 to prevent the same breakage from happening
again.
Thanks.
--
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] Revert "completion: fix shell expansion of items"
On Tue, Sep 25, 2012 at 12:31:19AM -0400, Jeff King wrote:
> Yeah, doing "git checkout jk/" is not working at all, and I noticed
> the buggy commit is on the maint track, but has not yet been released.
> I'm not sure of the solution, but I think we should do this in the
> meantime:
>
> -- >8 --
> Subject: Revert "completion: fix shell expansion of items"
I agree with the revert, too.
I looked into this issue, but quickly got lost in quoting-escaping
hell. Ideally we could do some quoting in __gitcomp_nl(), so it would
work for all kinds of input, but I couldn't come up with anything
working. Alternatively, we could modify __gitcomp_nl()'s callers, or
rather the helper functions supplying input to __gitcomp_nl() to do
the quoting or escaping themselves. Actually, that's quite easy for
local refs, at least, because for-each-ref's builtin quoting support
does the trick:
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index c48cd19f..3dc1ec8c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -313,7 +313,7 @@ __git_refs ()
refs="refs/tags refs/heads refs/remotes"
;;
esac
- git --git-dir="$dir" for-each-ref --format="%($format)" \
+ git --git-dir="$dir" for-each-ref --shell --format="%($format)"
\
$refs
if [ -n "$track" ]; then
# employ the heuristic used by git checkout
With this change completion of local refs works well, even in the
presence of branches ${foo.bar} and foo'bar.
Unfortunately, there are many callsites for __gitcomp_nl(), and it is
invoked with refs from remote repos, heads, tags, refspecs, remotes,
config variables, symbols from ctags, or output from stash or
ls-tree... although some of these are OK as they are now (remotes,
config variables).
Best,
Gábor
--
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] Revert "completion: fix shell expansion of items"
Jeff King writes:
> On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote:
>
> Yeah, doing "git checkout jk/" is not working at all, and I noticed
> the buggy commit is on the maint track, but has not yet been released.
> I'm not sure of the solution, but I think we should do this in the
> meantime:
Thanks. Reverting is the right solution; it will leave a note in
the history why a seemingly simple patch was a bad idea to remind
us.
It is sad that existing test did not cover something fundamental
like this, though. Anybody who's worked on completion wants to make
the test coverage better?
> -- >8 --
> Subject: Revert "completion: fix shell expansion of items"
>
> This reverts commit 25ae7cfd19c8f21721363c64163cd5d9d1135b20.
>
> That patch does fix expansion of weird variables in some
> simple tests, but it also seems to break other things, like
> expansion of refs by "git checkout".
>
> While we're sorting out the correct solution, we are much
> better with the original bug (people with metacharacters in
> their completions occasionally see an error message) than
> the current bug (ref completion does not work at all).
>
> Signed-off-by: Jeff King
> ---
> contrib/completion/git-completion.bash | 9 +
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 5a5b5a0..d743e56 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -225,13 +225,6 @@ fi
> fi
> fi
>
> -# Quotes the argument for shell reuse
> -__git_quote()
> -{
> - local quoted=${1//\'/\'\\\'\'}
> - printf "'%s'" "$quoted"
> -}
> -
> # Generates completion reply with compgen, appending a space to possible
> # completion words, if necessary.
> # It accepts 1 to 4 arguments:
> @@ -268,7 +261,7 @@ __gitcomp_nl ()
> __gitcomp_nl ()
> {
> local IFS=$'\n'
> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" --
> "${3-$cur}"))
> + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> }
>
> __git_heads ()
--
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] Revert "completion: fix shell expansion of items"
On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote:
> > > Solves the original problem for me.
> >
> > Me too. Thanks.
>
> While it solves the original problem, it seems to break refs
> completion, as demonstrated by the following POC test:
>
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 92d7eb47..fab63b95 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' '
> test_completion "git --no-replace-objects check" "checkout "
> '
>
> +test_expect_success 'basic refs completion' '
> + touch file &&
> + git add file &&
> + git commit -m initial &&
> + test_completion "git branch m" "master "
> +'
> +
Yeah, doing "git checkout jk/" is not working at all, and I noticed
the buggy commit is on the maint track, but has not yet been released.
I'm not sure of the solution, but I think we should do this in the
meantime:
-- >8 --
Subject: Revert "completion: fix shell expansion of items"
This reverts commit 25ae7cfd19c8f21721363c64163cd5d9d1135b20.
That patch does fix expansion of weird variables in some
simple tests, but it also seems to break other things, like
expansion of refs by "git checkout".
While we're sorting out the correct solution, we are much
better with the original bug (people with metacharacters in
their completions occasionally see an error message) than
the current bug (ref completion does not work at all).
Signed-off-by: Jeff King
---
contrib/completion/git-completion.bash | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 5a5b5a0..d743e56 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -225,13 +225,6 @@ fi
fi
fi
-# Quotes the argument for shell reuse
-__git_quote()
-{
- local quoted=${1//\'/\'\\\'\'}
- printf "'%s'" "$quoted"
-}
-
# Generates completion reply with compgen, appending a space to possible
# completion words, if necessary.
# It accepts 1 to 4 arguments:
@@ -268,7 +261,7 @@ __gitcomp_nl ()
__gitcomp_nl ()
{
local IFS=$'\n'
- COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" --
"${3-$cur}"))
+ COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
}
__git_heads ()
--
1.7.12.1.17.g7286916
--
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
