Re: [PATCH] completion: fix shell expansion of items

2012-09-20 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote:

 As reported by Jeroen Meijer[1]; the current code doesn't deal properly
 with items (tags, branches, etc.) that have ${} in them because they get
 expaned by bash while using compgen.
 
 A simple solution is to quote the items so they get expanded properly
 (\$\{\}).
 
 In order to achieve that I took bash-completion's quote() function,
 which is rather simple, and renamed it to __git_quote() as per Jeff
 King's suggestion.
 
 Solves the original problem for me.

 Me too. Thanks.

Thanks, both.  Will queue.
--
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] completion: fix shell expansion of items

2012-09-20 Thread SZEDER Gábor 
Hi,

On Wed, Sep 19, 2012 at 09:46:08PM -0400, Jeff King wrote:
 On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote:
 
  As reported by Jeroen Meijer[1]; the current code doesn't deal properly
  with items (tags, branches, etc.) that have ${} in them because they get
  expaned by bash while using compgen.
  
  A simple solution is to quote the items so they get expanded properly
  (\$\{\}).
  
  In order to achieve that I took bash-completion's quote() function,
  which is rather simple, and renamed it to __git_quote() as per Jeff
  King's suggestion.
  
  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 
+'
+
 test_done
-- 
1.7.12.1.438.g7dfa67b


which fails with:

--- expected2012-09-20 18:05:23.857752925 +
+++ out 2012-09-20 18:05:23.877752925 +
@@ -1 +1 @@
-master 
+

--
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] completion: fix shell expansion of items

2012-09-20 Thread Jeff King 
On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote:

   In order to achieve that I took bash-completion's quote() function,
   which is rather simple, and renamed it to __git_quote() as per Jeff
   King's suggestion.
   
   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 
 +'

Hmm.  I notice that Felipe's patch wraps the _whole_ input to
__gitcomp_nl in single quotes. So if there are multiple completions we
would end up with:

  'one
   two
   quo\'ted
   three'

I wonder if that is OK to feed to compgen -W, or if it wants to expand
it line-by-line. Just guessing at this point, though.

-Peff

 +
  test_done
 -- 
 1.7.12.1.438.g7dfa67b
 
 
 which fails with:
 
 --- expected2012-09-20 18:05:23.857752925 +
 +++ out 2012-09-20 18:05:23.877752925 +
 @@ -1 +1 @@
 -master 
 +
 
--
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] completion: fix shell expansion of items

2012-09-20 Thread Felipe Contreras 
On Thu, Sep 20, 2012 at 8:21 PM, Jeff King p...@peff.net wrote:
 On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote:

   In order to achieve that I took bash-completion's quote() function,
   which is rather simple, and renamed it to __git_quote() as per Jeff
   King's suggestion.
  
   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 
 +'

 Hmm.  I notice that Felipe's patch wraps the _whole_ input to
 __gitcomp_nl in single quotes.

Wasn't there a patch series that added tests for __gitcomp_nl to catch
these issues?

-- 
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