threads / patch / 30383

patchcompletion: simplify __git_complete_revlist_file

Subject: [PATCH] completion: simplify __git_complete_revlist_file

## tl;dr

6 messages between May 1, 2012 and May 7, 2012. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Felipe Contreras· May 1, 2012, 18:42 UTC · lore

Use new __gitcomp_nl; this is the last place that uses COMPREPLY and compgen directly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Show changes to contrib/completion/git-completion.bash +2 −4

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 31f714d..d877a23 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -676,9 +676,7 @@ __git_complete_revlist_file ()
 		*)   pfx="$ref:$pfx" ;;
 		esac
 
-		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "$pfx" \
-			-W "$(git --git-dir="$(__gitdir)" ls-tree "$ls" \
+		__gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" \
 				| sed '/^100... blob /{
 				           s,^.*	,,
 				           s,$, ,
@@ -692,7 +690,7 @@ __git_complete_revlist_file ()
 				           s,$,/,
 				       }
 				       s/^.*	//')" \
-			-- "$cur_"))
+			"$pfx" "$cur_" ""
 		;;
 	*...*)
 		pfx="${cur_%...*}..."

-- 
1.7.10

Junio C Hamano· May 1, 2012, 23:39 UTC · re: Felipe Contreras · lore

Re: [PATCH] completion: simplify __git_complete_revlist_file

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 10 quoted lines

> Use new __gitcomp_nl; this is the last place that uses COMPREPLY and
> compgen directly.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 31f714d..d877a23 100755

This seems to be based on 'maint', which has 31f714d, but I see two hits with "git grep 'COMPREPLY.*compgen' contrib/completion" there, so I am not sure if that is where you wanted it to apply.

I see this was not addressed to me (neither was the other zsh one), so I take it that the patch is still for discussion, at least for the purpose of today's integration round. Ping me when it is ready for either 'pu' or 'next'.

Thanks.

Felipe Contreras· May 1, 2012, 23:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion: simplify __git_complete_revlist_file

On Wed, May 2, 2012 at 1:39 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Use new __gitcomp_nl; this is the last place that uses COMPREPLY and
>> compgen directly.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  contrib/completion/git-completion.bash |    6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 31f714d..d877a23 100755
>
> This seems to be based on 'maint', which has 31f714d, but I see two hits
> with "git grep 'COMPREPLY.*compgen' contrib/completion" there, so I am not
> sure if that is where you wanted it to apply.

The last place that is not the __gitcomp* functions, of course.

> I see this was not addressed to me (neither was the other zsh one), so I
> take it that the patch is still for discussion, at least for the purpose
> of today's integration round.  Ping me when it is ready for either 'pu' or
> 'next'.

I'd say at least one person should say LGTM before asking you to integrate it.

Cheers.

-- 
Felipe Contreras

Felipe Contreras· May 5, 2012, 15:19 UTC · re: Felipe Contreras · lore

Re: [PATCH] completion: simplify __git_complete_revlist_file

On Wed, May 2, 2012 at 1:46 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Wed, May 2, 2012 at 1:39 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>> I see this was not addressed to me (neither was the other zsh one), so I
>> take it that the patch is still for discussion, at least for the purpose
>> of today's integration round.  Ping me when it is ready for either 'pu' or
>> 'next'.
>
> I'd say at least one person should say LGTM before asking you to integrate it.

Nobody has raised any issues with this, so, please merge it to 'pu'. This is so straight-forward that it might make sense to move to 'next' through.

Cheers.

-- 
Felipe Contreras

SZEDER Gábor· May 5, 2012, 22:40 UTC · re: Felipe Contreras · lore

Re: [PATCH] completion: simplify __git_complete_revlist_file

Hi,

On Wed, May 02, 2012 at 01:46:32AM +0200, Felipe Contreras wrote:

Show 5 quoted lines

> On Wed, May 2, 2012 at 1:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> >> Use new __gitcomp_nl; this is the last place that uses COMPREPLY and
> >> compgen directly.

I didn't dare to touch that code back then while doing the __gitcomp_nl() changes; that sed command looked way too scary ;) But all it does is to process the output of 'git ls-tree' and removes the cruft and appends a space to filenames and a '/' to directories (without a trailing space). So invoking __gitcomp_nl() with an empty suffix parameter is OK, as it doesn't change the current behavior.

Show 20 quoted lines

> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> ---
> >>  contrib/completion/git-completion.bash |    6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> >> index 31f714d..d877a23 100755
> >
> > This seems to be based on 'maint', which has 31f714d, but I see two hits
> > with "git grep 'COMPREPLY.*compgen' contrib/completion" there, so I am not
> > sure if that is where you wanted it to apply.
> 
> The last place that is not the __gitcomp* functions, of course.
> 
> > I see this was not addressed to me (neither was the other zsh one), so I
> > take it that the patch is still for discussion, at least for the purpose
> > of today's integration round.  Ping me when it is ready for either 'pu' or
> > 'next'.
> 
> I'd say at least one person should say LGTM before asking you to integrate it.

Yeah, it's good to go.

Best, Gábor

Junio C Hamano· May 7, 2012, 17:15 UTC · re: Felipe Contreras · lore

Re: [PATCH] completion: simplify __git_complete_revlist_file

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 7 quoted lines

> On Wed, May 2, 2012 at 1:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> This seems to be based on 'maint', which has 31f714d, but I see two hits
>> with "git grep 'COMPREPLY.*compgen' contrib/completion" there, so I am not
>> sure if that is where you wanted it to apply.
>
> The last place that is not the __gitcomp* functions, of course.

OK, that is what you meant by "uses ... _directly_". With SG's Ack, will queue.