Patchwork [4,of,5,osx] zsh_completion: install as _hg not hg

login
register
mail settings
Submitter Augie Fackler
Date May 27, 2017, 12:27 a.m.
Message ID <ffca36020b1032817a45.1495844851@augie-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/20951/
State Accepted
Headers show

Comments

Augie Fackler - May 27, 2017, 12:27 a.m.
# HG changeset patch
# User Kyle Lippincott <spectral@google.com>
# Date 1495830247 25200
#      Fri May 26 13:24:07 2017 -0700
# Node ID ffca36020b1032817a451e0fb752b80bbf55d84a
# Parent  599d022d2d51320b2d593152e6d87650ceaf1c06
zsh_completion: install as _hg not hg

The contrib/zsh_completion file itself says to name it _hg.

With a name like `hg`, if the user has a line like `autoload ${^fpath}/*(N-.:t)`
in their zshrc, it will create a shell function named `hg` that will hide the
actual hg command and make hg unusable.

Separately from that though, the underscore prefix makes it actually work. The
zsh man page states:

    The convention for autoloaded functions used in completion is that they
    start with an underscore

This does not seem to just be a "convention", though. With the ill-advised line
removed from my zshrc and the file named
`/usr/local/share/zsh/site-functions/hg` (without the underscore), these
completions did not seem to get loaded and the ones from the zsh installation
were loaded instead.  If I renamed them to be
`/usr/local/share/zsh/site-functions/_hg`, however, they were loaded.

I manually tested the above statement by starting a new zsh instance with the
file in `/usr/local/share/zsh/site-functions` with the following names:
- As `hg`, `which _hg_labels` did not show anything
- As `_hg`, `which _hg_labels` showed the expected function.
Kyle Lippincott - May 31, 2017, 12:44 a.m.
Would it make sense to put this (and the required pkgbuild change) on
'stable' to avoid the issue from the first paragraph of the description?

With a name like `hg`, if the user has a line like `autoload
${^fpath}/*(N-.:t)`
in their zshrc, it will create a shell function named `hg` that will hide
the
actual hg command and make hg unusable.

I don't know how long this was out there as 'hg' instead of '_hg', so it
might be only 4.2 that was affected, and if we have a patch release it
would be nice to have less time where packagers might take the potentially
very-broken version, instead of waiting for 4.3 to get it out there?

On Fri, May 26, 2017 at 5:27 PM, Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Kyle Lippincott <spectral@google.com>
> # Date 1495830247 25200
> #      Fri May 26 13:24:07 2017 -0700
> # Node ID ffca36020b1032817a451e0fb752b80bbf55d84a
> # Parent  599d022d2d51320b2d593152e6d87650ceaf1c06
> zsh_completion: install as _hg not hg
>
> The contrib/zsh_completion file itself says to name it _hg.
>
> With a name like `hg`, if the user has a line like `autoload
> ${^fpath}/*(N-.:t)`
> in their zshrc, it will create a shell function named `hg` that will hide
> the
> actual hg command and make hg unusable.
>
> Separately from that though, the underscore prefix makes it actually work.
> The
> zsh man page states:
>
>     The convention for autoloaded functions used in completion is that they
>     start with an underscore
>
> This does not seem to just be a "convention", though. With the ill-advised
> line
> removed from my zshrc and the file named
> `/usr/local/share/zsh/site-functions/hg` (without the underscore), these
> completions did not seem to get loaded and the ones from the zsh
> installation
> were loaded instead.  If I renamed them to be
> `/usr/local/share/zsh/site-functions/_hg`, however, they were loaded.
>
> I manually tested the above statement by starting a new zsh instance with
> the
> file in `/usr/local/share/zsh/site-functions` with the following names:
> - As `hg`, `which _hg_labels` did not show anything
> - As `_hg`, `which _hg_labels` showed the expected function.
>
> diff --git a/Makefile b/Makefile
> --- a/Makefile
> +++ b/Makefile
> @@ -169,7 +169,7 @@ osx:
>          # install zsh completions - this location appears to be
>          # searched by default as of macOS Sierra.
>         install -d build/mercurial/usr/local/share/zsh/site-functions/
> -       install -m 0644 contrib/zsh_completion build/mercurial/usr/local/
> share/zsh/site-functions/hg
> +       install -m 0644 contrib/zsh_completion build/mercurial/usr/local/
> share/zsh/site-functions/_hg
>          # install bash completions - there doesn't appear to be a
>          # place that's searched by default for bash, so we'll follow
>          # the lead of Apple's git install and just put it in a
> diff --git a/tests/test-mac-packages.t b/tests/test-mac-packages.t
> --- a/tests/test-mac-packages.t
> +++ b/tests/test-mac-packages.t
> @@ -29,8 +29,8 @@ Spot-check some randomly selected files:
>    ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.py   100644
> 0/0
>    ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.pyc  100644
> 0/0
>    ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.pyo  100644
> 0/0
> -  $ grep zsh/site-functions/hg boms.txt | cut -d '     ' -f 1,2,3
> -  ./usr/local/share/zsh/site-functions/hg      100644  0/0
> +  $ grep zsh/site-functions/_hg boms.txt | cut -d '    ' -f 1,2,3
> +  ./usr/local/share/zsh/site-functions/_hg     100644  0/0
>    $ grep hg-completion.bash boms.txt | cut -d '        ' -f 1,2,3
>    ./usr/local/hg/contrib/hg-completion.bash    100644  0/0
>    $ egrep 'man[15]' boms.txt | cut -d '        ' -f 1,2,3
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - May 31, 2017, 6:16 p.m.
On Tue, May 30, 2017 at 8:44 PM, Kyle Lippincott <spectral@pewpew.net> wrote:
> Would it make sense to put this (and the required pkgbuild change) on
> 'stable' to avoid the issue from the first paragraph of the description?
>
> With a name like `hg`, if the user has a line like `autoload
> ${^fpath}/*(N-.:t)`
> in their zshrc, it will create a shell function named `hg` that will hide
> the
> actual hg command and make hg unusable.
>
> I don't know how long this was out there as 'hg' instead of '_hg', so it
> might be only 4.2 that was affected, and if we have a patch release it would
> be nice to have less time where packagers might take the potentially
> very-broken version, instead of waiting for 4.3 to get it out there?

Sounds good to me. Anyone object? We'd need to graft this patch and
the OS X installer fix for the .hg ignore in pkgbuild to stable.

>
> On Fri, May 26, 2017 at 5:27 PM, Augie Fackler <raf@durin42.com> wrote:
>>
>> # HG changeset patch
>> # User Kyle Lippincott <spectral@google.com>
>> # Date 1495830247 25200
>> #      Fri May 26 13:24:07 2017 -0700
>> # Node ID ffca36020b1032817a451e0fb752b80bbf55d84a
>> # Parent  599d022d2d51320b2d593152e6d87650ceaf1c06
>> zsh_completion: install as _hg not hg
>>
>> The contrib/zsh_completion file itself says to name it _hg.
>>
>> With a name like `hg`, if the user has a line like `autoload
>> ${^fpath}/*(N-.:t)`
>> in their zshrc, it will create a shell function named `hg` that will hide
>> the
>> actual hg command and make hg unusable.
>>
>> Separately from that though, the underscore prefix makes it actually work.
>> The
>> zsh man page states:
>>
>>     The convention for autoloaded functions used in completion is that
>> they
>>     start with an underscore
>>
>> This does not seem to just be a "convention", though. With the ill-advised
>> line
>> removed from my zshrc and the file named
>> `/usr/local/share/zsh/site-functions/hg` (without the underscore), these
>> completions did not seem to get loaded and the ones from the zsh
>> installation
>> were loaded instead.  If I renamed them to be
>> `/usr/local/share/zsh/site-functions/_hg`, however, they were loaded.
>>
>> I manually tested the above statement by starting a new zsh instance with
>> the
>> file in `/usr/local/share/zsh/site-functions` with the following names:
>> - As `hg`, `which _hg_labels` did not show anything
>> - As `_hg`, `which _hg_labels` showed the expected function.
>>
>> diff --git a/Makefile b/Makefile
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -169,7 +169,7 @@ osx:
>>          # install zsh completions - this location appears to be
>>          # searched by default as of macOS Sierra.
>>         install -d build/mercurial/usr/local/share/zsh/site-functions/
>> -       install -m 0644 contrib/zsh_completion
>> build/mercurial/usr/local/share/zsh/site-functions/hg
>> +       install -m 0644 contrib/zsh_completion
>> build/mercurial/usr/local/share/zsh/site-functions/_hg
>>          # install bash completions - there doesn't appear to be a
>>          # place that's searched by default for bash, so we'll follow
>>          # the lead of Apple's git install and just put it in a
>> diff --git a/tests/test-mac-packages.t b/tests/test-mac-packages.t
>> --- a/tests/test-mac-packages.t
>> +++ b/tests/test-mac-packages.t
>> @@ -29,8 +29,8 @@ Spot-check some randomly selected files:
>>    ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.py   100644
>> 0/0
>>    ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.pyc  100644
>> 0/0
>>    ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.pyo  100644
>> 0/0
>> -  $ grep zsh/site-functions/hg boms.txt | cut -d '     ' -f 1,2,3
>> -  ./usr/local/share/zsh/site-functions/hg      100644  0/0
>> +  $ grep zsh/site-functions/_hg boms.txt | cut -d '    ' -f 1,2,3
>> +  ./usr/local/share/zsh/site-functions/_hg     100644  0/0
>>    $ grep hg-completion.bash boms.txt | cut -d '        ' -f 1,2,3
>>    ./usr/local/hg/contrib/hg-completion.bash    100644  0/0
>>    $ egrep 'man[15]' boms.txt | cut -d '        ' -f 1,2,3
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
Yuya Nishihara - June 1, 2017, 2:19 p.m.
On Wed, 31 May 2017 14:16:30 -0400, Augie Fackler wrote:
> On Tue, May 30, 2017 at 8:44 PM, Kyle Lippincott <spectral@pewpew.net> wrote:
> > Would it make sense to put this (and the required pkgbuild change) on
> > 'stable' to avoid the issue from the first paragraph of the description?
> >
> > With a name like `hg`, if the user has a line like `autoload
> > ${^fpath}/*(N-.:t)`
> > in their zshrc, it will create a shell function named `hg` that will hide
> > the
> > actual hg command and make hg unusable.
> >
> > I don't know how long this was out there as 'hg' instead of '_hg', so it
> > might be only 4.2 that was affected, and if we have a patch release it would
> > be nice to have less time where packagers might take the potentially
> > very-broken version, instead of waiting for 4.3 to get it out there?
> 
> Sounds good to me. Anyone object? We'd need to graft this patch and
> the OS X installer fix for the .hg ignore in pkgbuild to stable.

+1
Augie Fackler - June 1, 2017, 2:59 p.m.
On Thu, Jun 1, 2017 at 10:19 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Wed, 31 May 2017 14:16:30 -0400, Augie Fackler wrote:
>> On Tue, May 30, 2017 at 8:44 PM, Kyle Lippincott <spectral@pewpew.net> wrote:
>> > Would it make sense to put this (and the required pkgbuild change) on
>> > 'stable' to avoid the issue from the first paragraph of the description?
>> >
>> > With a name like `hg`, if the user has a line like `autoload
>> > ${^fpath}/*(N-.:t)`
>> > in their zshrc, it will create a shell function named `hg` that will hide
>> > the
>> > actual hg command and make hg unusable.
>> >
>> > I don't know how long this was out there as 'hg' instead of '_hg', so it
>> > might be only 4.2 that was affected, and if we have a patch release it would
>> > be nice to have less time where packagers might take the potentially
>> > very-broken version, instead of waiting for 4.3 to get it out there?
>>
>> Sounds good to me. Anyone object? We'd need to graft this patch and
>> the OS X installer fix for the .hg ignore in pkgbuild to stable.
>
> +1

This is now done. Thanks!

Patch

diff --git a/Makefile b/Makefile
--- a/Makefile
+++ b/Makefile
@@ -169,7 +169,7 @@  osx:
         # install zsh completions - this location appears to be
         # searched by default as of macOS Sierra.
 	install -d build/mercurial/usr/local/share/zsh/site-functions/
-	install -m 0644 contrib/zsh_completion build/mercurial/usr/local/share/zsh/site-functions/hg
+	install -m 0644 contrib/zsh_completion build/mercurial/usr/local/share/zsh/site-functions/_hg
         # install bash completions - there doesn't appear to be a
         # place that's searched by default for bash, so we'll follow
         # the lead of Apple's git install and just put it in a
diff --git a/tests/test-mac-packages.t b/tests/test-mac-packages.t
--- a/tests/test-mac-packages.t
+++ b/tests/test-mac-packages.t
@@ -29,8 +29,8 @@  Spot-check some randomly selected files:
   ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.py	100644	0/0
   ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.pyc	100644	0/0
   ./Library/Python/2.7/site-packages/mercurial/pure/bdiff.pyo	100644	0/0
-  $ grep zsh/site-functions/hg boms.txt | cut -d '	' -f 1,2,3
-  ./usr/local/share/zsh/site-functions/hg	100644	0/0
+  $ grep zsh/site-functions/_hg boms.txt | cut -d '	' -f 1,2,3
+  ./usr/local/share/zsh/site-functions/_hg	100644	0/0
   $ grep hg-completion.bash boms.txt | cut -d '	' -f 1,2,3
   ./usr/local/hg/contrib/hg-completion.bash	100644	0/0
   $ egrep 'man[15]' boms.txt | cut -d '	' -f 1,2,3