threads / patch / 32362

patchgit-completion.bash: update obsolete code.

Subject: [PATCH] git-completion.bash: update obsolete code.

## tl;dr

5 messages between Dec 16, 2012 and Dec 18, 2012. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Manlio Perillo· Dec 16, 2012, 21:50 UTC · lore

The git-completion.bash script was using the git ls-tree command without the --name-only option, with a sed filter to parse path names; use the --name-only option, instead.

Signed-off-by: Manlio Perillo <manlio.perillo@gmail.com>
---
 contrib/completion/git-completion.bash | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

Show changes to contrib/completion/git-completion.bash +1 −14

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0b77eb1..85d9051 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -397,20 +397,7 @@ __git_complete_revlist_file ()
 		*)   pfx="$ref:$pfx" ;;
 		esac
 
-		__gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" \
-				| sed '/^100... blob /{
-				           s,^.*	,,
-				           s,$, ,
-				       }
-				       /^120000 blob /{
-				           s,^.*	,,
-				           s,$, ,
-				       }
-				       /^040000 tree /{
-				           s,^.*	,,
-				           s,$,/,
-				       }
-				       s/^.*	//')" \
+		__gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree --name-only "$ls")" \
 			"$pfx" "$cur_" ""
 		;;
 	*...*)

-- 
1.8.1.rc1.18.g9db0d25

Junio C Hamano· Dec 17, 2012, 04:54 UTC · re: Manlio Perillo · lore

Re: [PATCH] git-completion.bash: update obsolete code.

Manlio Perillo <manlio.perillo@gmail.com> writes:

Show 6 quoted lines

> The git-completion.bash script was using the git ls-tree command
> without the --name-only option, with a sed filter to parse path names;
> use the --name-only option, instead.
>
> Signed-off-by: Manlio Perillo <manlio.perillo@gmail.com>
> ---

Did you miss the different handling between blobs and trees the latter gets trailing slash in the completion)?

Show 29 quoted lines

>  contrib/completion/git-completion.bash | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 0b77eb1..85d9051 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -397,20 +397,7 @@ __git_complete_revlist_file ()
>  		*)   pfx="$ref:$pfx" ;;
>  		esac
>  
> -		__gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" \
> -				| sed '/^100... blob /{
> -				           s,^.*	,,
> -				           s,$, ,
> -				       }
> -				       /^120000 blob /{
> -				           s,^.*	,,
> -				           s,$, ,
> -				       }
> -				       /^040000 tree /{
> -				           s,^.*	,,
> -				           s,$,/,
> -				       }
> -				       s/^.*	//')" \
> +		__gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree --name-only "$ls")" \
>  			"$pfx" "$cur_" ""
>  		;;
>  	*...*)

Manlio Perillo· Dec 17, 2012, 16:53 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-completion.bash: update obsolete code.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 17/12/2012 05:54, Junio C Hamano ha scritto:

Show 12 quoted lines

> Manlio Perillo <manlio.perillo@gmail.com> writes:
> 
>> The git-completion.bash script was using the git ls-tree command
>> without the --name-only option, with a sed filter to parse path names;
>> use the --name-only option, instead.
>>
>> Signed-off-by: Manlio Perillo <manlio.perillo@gmail.com>
>> ---
> 
> Did you miss the different handling between blobs and trees the
> latter gets trailing slash in the completion)?
>

Yes, I totally missed it, sorry. I was assuming the bash completion script was written before --name-only option was added to ls-tree.

By the way, IMHO there should be an option for adding a slash to directory names in ls-tree.

> [...]

Regards  Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEUEARECAAYFAlDPTgUACgkQscQJ24LbaUSkKgCePH2NFHf/qp2ZrgI9eD9D0D3G zOwAmL8Dc8r9DevyV1ZhaCb2G9MPZxU= =QJEy -----END PGP SIGNATURE-----

Junio C Hamano· Dec 17, 2012, 20:29 UTC · re: Manlio Perillo · lore

Re: [PATCH] git-completion.bash: update obsolete code.

Manlio Perillo <manlio.perillo@gmail.com> writes:

> By the way, IMHO there should be an option for adding a slash to
> directory names in ls-tree.

I am not sure about that; ls-tree is meant to be used by scripts that are capable of doing that kind of thing themselves.

If we were to add an option to add a slash, I think it should be modeled after "ls -F", whose output is meant for humans and no sane scripts would read from it. It is very likely that such an option should add @ at the end of the name for a symbolic link, so it won't be useful for your patch.

Junio C Hamano· Dec 18, 2012, 01:37 UTC · re: Manlio Perillo · lore

Re: [PATCH] git-completion.bash: update obsolete code.

Manlio Perillo <manlio.perillo@gmail.com> writes:

Show 17 quoted lines

> Il 17/12/2012 05:54, Junio C Hamano ha scritto:
>> Manlio Perillo <manlio.perillo@gmail.com> writes:
>> 
>>> The git-completion.bash script was using the git ls-tree command
>>> without the --name-only option, with a sed filter to parse path names;
>>> use the --name-only option, instead.
>>>
>>> Signed-off-by: Manlio Perillo <manlio.perillo@gmail.com>
>>> ---
>> 
>> Did you miss the different handling between blobs and trees the
>> latter gets trailing slash in the completion)?
>> 
>
> Yes, I totally missed it, sorry.
> I was assuming the bash completion script was written before --name-only
> option was added to ls-tree.

There is no need to say "sorry" here; catching this kind of mistake is what we have the patch review process for, after all. If anything, "thanks" is more appropriate ;-)

And the way you stated the reasoning behind this change in the proposed commit log message was really good. It showed clearly that the patch was an attempt to clean up the code, not was an attempt to change the behaviour to drop the trailing slash or space. If it said "let's use ls-tree --name-only" without such an explanation on "why", future readers of "git log" would have been left scratching their heads, wondering "why" the change was made.

Thanks.