completion: Use native ZSH array pattern matching

[3v1n0]

When clearing the builtin operations on re-sourcing in the ZSH case we
can use the native ${parameters} associative array keys values to get
the currently __gitcomp_builtin_* operations using pattern matching
instead of using sed.

[gitgitgadget]

Welcome to GitGitGadget

Hi @3v1n0, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget]

There are issues in commit d5a79a13f972d6176bd076bc5433b186aad9f057:
Prefixed commit message must be in lower case: completion: Use native ZSH array pattern matching
Commit not signed off

[gitgitgadget]

There is an issue in commit a8553951a482f14d70e45158c8ae70164f50b8f3:
Prefixed commit message must be in lower case: completion: Use native ZSH array pattern matching

[dscho]

/allow

[gitgitgadget]

User 3v1n0 is now allowed to use GitGitGadget.

[3v1n0]

/submit

[gitgitgadget]

Submitted as pull.645.git.1590520398132.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, "brian m. carlson" wrote (reply to this):

--FLPM4o+7JoHGki3m
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 2020-05-26 at 19:13:17, Marco Trevisan via GitGitGadget wrote:
> From: =3D?UTF-8?q?Marco=3D20Trevisan=3D20=3D28Trevi=3DC3=3DB1o=3D29?=3D <=
mail@3v1n0.net>
>=20
> When clearing the builtin operations on re-sourcing in the ZSH case we
> can use the native ${parameters} associative array keys values to get
> the currently `__gitcomp_builtin_*` operations using pattern matching
> instead of using sed.
>=20
> As also stated in commit 94408dc7, introducing this change the usage of
> sed has some overhead implications, while ZSH can do this check just
> using its native syntax.
>=20
> Signed-off-by: Marco Trevisan (Trevi=C3=B1o) <mail@3v1n0.net>
> ---
>     completion: Use native ZSH array pattern matching
>    =20
>     When clearing the builtin operations on re-sourcing in the ZSH case we
>     can use the native ${parameters} associative array keys values to get
>     the currently __gitcomp_builtin_* operations using pattern matching
>     instead of using sed.
>=20
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-645%2F3=
v1n0%2Fzsh-native-operation-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-645/3v1n0/=
zsh-native-operation-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/645
>=20
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>=20
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/=
git-completion.bash
> index 70ad04e1b2a..ad6934a3864 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -373,7 +373,7 @@ __gitcomp ()
>  # Clear the variables caching builtins' options when (re-)sourcing
>  # the completion script.
>  if [[ -n ${ZSH_VERSION-} ]]; then
> -	unset $(set |sed -ne 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\=
)=3D.*/\1/p') 2>/dev/null
> +	unset ${(M)${(k)parameters[@]}:#__gitcomp_builtin_*} 2>/dev/null
>  else
>  	unset $(compgen -v __gitcomp_builtin_)
>  fi

This file is necessarily used by both bash and zsh.  Does bash
(including the bash 3 used on macOS) happen to continue to work with
this syntax?

We've had cases in the past where despite some code running under shell
A, shell B, which also parsed file, choked on the data because it
had to parse it even though it didn't execute it.

If so, and this works there, can you mention that in your commit message
for future readers?
--=20
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

--FLPM4o+7JoHGki3m
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.2.20 (GNU/Linux)

iHUEABYKAB0WIQQILOaKnbxl+4PRw5F8DEliiIeigQUCXs2tKgAKCRB8DEliiIei
gczSAQDKHldIX3y4ut5qCAJ+W57gYfLttEmNlgVzDYJYWMkJ6QD+PIIm+xh9HxGw
8rv7sUKxfxvDY9JXh1/lgK2hSaQXmwM=
=+cXs
-----END PGP SIGNATURE-----

--FLPM4o+7JoHGki3m--

[gitgitgadget]

On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this):

On Tue, May 26, 2020 at 11:58:34PM +0000, brian m. carlson wrote:
> 
> This file is necessarily used by both bash and zsh.  Does bash
> (including the bash 3 used on macOS) happen to continue to work with
> this syntax?

if by that you meant t9902.[150-152] to succeed with macOS bash?:

$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)
Copyright (C) 2007 Free Software Foundation, Inc.

then I think we can add (on top of master):

Tested-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

Carlo

[gitgitgadget]

On the Git mailing list, "brian m. carlson" wrote (reply to this):

--wjoFZxbW4tu+iR6v
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 2020-05-27 at 06:13:59, Carlo Marcelo Arenas Bel=C3=B3n wrote:
> On Tue, May 26, 2020 at 11:58:34PM +0000, brian m. carlson wrote:
> >=20
> > This file is necessarily used by both bash and zsh.  Does bash
> > (including the bash 3 used on macOS) happen to continue to work with
> > this syntax?
>=20
> if by that you meant t9902.[150-152] to succeed with macOS bash?:
>=20
> $ bash --version
> GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)
> Copyright (C) 2007 Free Software Foundation, Inc.
>=20
> then I think we can add (on top of master):
>=20
> Tested-by: Carlo Marcelo Arenas Bel=C3=B3n <carenas@gmail.com>

Yes, that is what I meant.  I'm glad to know my question has been
answered and things work.  I'm okay with the patch as it is in that
case, although I'd give bonus points for mentioning that this syntax
doesn't regress bash.
--=20
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

--wjoFZxbW4tu+iR6v
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.2.20 (GNU/Linux)

iHUEABYKAB0WIQQILOaKnbxl+4PRw5F8DEliiIeigQUCXs8fNwAKCRB8DEliiIei
gUn/AP42s9udMj06960GtzRES0ofrfOrglkbDJY0oWRYgsCbpAD9H9t2BOL6iLLp
f0EKJgqBEf1Id9mEWlqKRa2ZAnUzSQw=
=Ml2k
-----END PGP SIGNATURE-----

--wjoFZxbW4tu+iR6v--

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2020-05-27 at 06:13:59, Carlo Marcelo Arenas Belón wrote:
>> On Tue, May 26, 2020 at 11:58:34PM +0000, brian m. carlson wrote:
>> > 
>> > This file is necessarily used by both bash and zsh.  Does bash
>> > (including the bash 3 used on macOS) happen to continue to work with
>> > this syntax?
>> 
>> if by that you meant t9902.[150-152] to succeed with macOS bash?:
>> 
>> $ bash --version
>> GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)
>> Copyright (C) 2007 Free Software Foundation, Inc.
>> 
>> then I think we can add (on top of master):
>> 
>> Tested-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>
> Yes, that is what I meant.  I'm glad to know my question has been
> answered and things work.  I'm okay with the patch as it is in that
> case, although I'd give bonus points for mentioning that this syntax
> doesn't regress bash.

True.  And we would want to also have tested-by on more recent
versions of bash, no?

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Marco Trevisan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <mail@3v1n0.net>
>
> When clearing the builtin operations on re-sourcing in the ZSH case we
> can use the native ${parameters} associative array keys values to get
> the currently `__gitcomp_builtin_*` operations using pattern matching
> instead of using sed.

"We can" may be a statement of fact, but by itself it does not make
a convincing argument why we should have this change in our
codebase.  

Is this change about correctness?  Is it about performance?
Or something else?

If it is about correctness, "the current code fails in this way
given this input, but with the new code the breakage is gone" would
be a good justification to give in the proposed log message.  If it
is about performance, of course a measured performance numbers would
make an objective justification that is hard to argue with,
especially if you can convince people that the patch does not change
the behaviour in any negative way.  If it is about something else,
well, there would be an appropriate way to justify the change,
depending on what it is ;-)

Thanks.

[gitgitgadget]

This branch is now known as mt/zsh-completion-optim.

[gitgitgadget]

This patch series was integrated into pu via git@a047d73.

[gitgitgadget]

This patch series was integrated into pu via git@7313ebd.

[gitgitgadget]

On the Git mailing list, "brian m. carlson" wrote (reply to this):

--KZLWU/9q3evlN4nQ
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 2020-05-28 at 15:54:49, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > Yes, that is what I meant.  I'm glad to know my question has been
> > answered and things work.  I'm okay with the patch as it is in that
> > case, although I'd give bonus points for mentioning that this syntax
> > doesn't regress bash.
>=20
> True.  And we would want to also have tested-by on more recent
> versions of bash, no?

Sure, such testing would be welcome, but I believe those are tested with
our tests on most platforms.  macOS is special because it uses the last
GPLv2 version of bash, which is less capable in some ways.  I assumed
that bash would not be more likely to break here in newer versions, but
perhaps I shouldn't make that assumption.
--=20
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

--KZLWU/9q3evlN4nQ
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.2.20 (GNU/Linux)

iHUEABYKAB0WIQQILOaKnbxl+4PRw5F8DEliiIeigQUCXtBB3AAKCRB8DEliiIei
gXWlAQCzMSJWA/YkyaNz5SDEA+UnEjL2TAK9I4eQ5MiytzprkAEArACuIbVhwep0
xaiiQ/f+LmzZ/cwbpp3jb+KYmC0tOA0=
=7snB
-----END PGP SIGNATURE-----

--KZLWU/9q3evlN4nQ--

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2020-05-28 at 15:54:49, Junio C Hamano wrote:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> > Yes, that is what I meant.  I'm glad to know my question has been
>> > answered and things work.  I'm okay with the patch as it is in that
>> > case, although I'd give bonus points for mentioning that this syntax
>> > doesn't regress bash.
>> 
>> True.  And we would want to also have tested-by on more recent
>> versions of bash, no?
>
> Sure, such testing would be welcome, but I believe those are tested with
> our tests on most platforms.  macOS is special because it uses the last
> GPLv2 version of bash, which is less capable in some ways.  I assumed
> that bash would not be more likely to break here in newer versions, but
> perhaps I shouldn't make that assumption.

In any case, a few integration into 'pu' with the patch have been
already made e.g. https://travis-ci.org/github/git/git/builds/692290894
so we should be good ;-)

[gitgitgadget]

This patch series was integrated into pu via git@df182f3.

[gitgitgadget]

This patch series was integrated into pu via git@0a61cd8.

[gitgitgadget]

This patch series was integrated into pu via git@b4eccd0.

[gitgitgadget]

This patch series was integrated into pu via git@9805eed.

[gitgitgadget]

This patch series was integrated into next via git@f5d02a5.

[gitgitgadget]

This patch series was integrated into pu via git@ecfa642.

[gitgitgadget]

This patch series was integrated into pu via git@a0ba2bb.

[gitgitgadget]

This patch series was integrated into next via git@a0ba2bb.

[gitgitgadget]

This patch series was integrated into master via git@a0ba2bb.

[gitgitgadget]

Closed via a0ba2bb.