Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] Remove state (@a) claim from perldiag #15511

Closed
p5pRT opened this issue Aug 12, 2016 · 14 comments
Closed

[PATCH] Remove state (@a) claim from perldiag #15511

p5pRT opened this issue Aug 12, 2016 · 14 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 12, 2016

Migrated from rt.perl.org#128910 (status was 'resolved')

Searchable as RT128910$

@p5pRT
Copy link
Author

p5pRT commented Aug 12, 2016

From @Smylers

This is a bug report for perl from smylers@​stripey.com,
generated with the help of perlbug 1.40 running under perl 5.25.2.


I was surprised to see perldiag making a claim about a feature being
added to a future version of Perl, namely​:

  Constructions such as "state (@​a) = foo()" will be supported in a
  future perl release.

That's in the description for ‘Initialization of state variables in list
context currently forbidden’.

perldiag doesn't seem like a good place to announce forthcoming
features, especially if they don't actually turn out to be forthcoming​:
the above claim was added in 2009.

Implementing the feature is RT# 114932, most recently updated in 2013,
and without agreement on the desired behaviour​:
https://rt.perl.org/Public/Bug/Display.html?id=114932

So I think it's better to remove the claim about a future feature,
simply documenting the situation as it is now.

I also tweaked the wording to make it more obvious that state can only
be applied to a single scalar variable at at time, and gave a suggestion
as to what to do if you're trying to use state with an array or hash.

----------------------------------------------------------------- ---
Flags​: category=docs severity=low Type=Patch PatchStatus=HasPatch ---
Site configuration information for perl 5.25.2​:

Configured by smylers at Tue May 31 13​:40​:14 BST 2016.

Summary of my perl5 (revision 5 version 25 subversion 2) configuration​:
  Commit id​: bdc905d
  Platform​:
  osname=linux, osvers=3.13.0-85-generic, archname=x86_64-linux
  uname='linux fozzie 3.13.0-85-generic #129-ubuntu smp thu mar 17 20​:50​:15 utc 2016 x86_64 x86_64 x86_64 gnulinux '
  config_args='-des -Dusedevel'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  use64bitint=define, use64bitall=define, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2',
  cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
  ccversion='', gccversion='4.8.4', gccosandvers=''
  intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3
  ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.8/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
  libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.19.so, so=so, useshrplib=false, libperl=libperl.a
  gnulibc_version='2.19'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'


@​INC for perl 5.25.2​:
  lib
  /usr/local/lib/perl5/site_perl/5.25.2/x86_64-linux
  /usr/local/lib/perl5/site_perl/5.25.2
  /usr/local/lib/perl5/5.25.2/x86_64-linux
  /usr/local/lib/perl5/5.25.2
  .


Environment for perl 5.25.2​:
  HOME=/home/smylers
  LANG=en_GB.utf8
  LANGUAGE=en_GB​:en
  LC_COLLATE=C
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/home/smylers/bin​:/usr/local/sbin​:/usr/local/bin​:/sbin​:/bin​:/usr/sbin​:/usr/bin​:/usr/X11R6/bin​:/usr/games
  PERL_BADLANG (unset)
  PERL_CPANM_OPT=--sudo --prompt
  SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Aug 12, 2016

From @Smylers

0001-Remove-claim-that-state-a-will-be-supported.patch
From 43bb7ac1d0b6cb6bcfe3c45e8a0afa6ba58e9855 Mon Sep 17 00:00:00 2001
From: Smylers <Smylers@stripey.com>
Date: Fri, 12 Aug 2016 16:34:17 +0100
Subject: [PATCH] Remove claim that state @a will be supported

perldiag doesn't seem to be the right place to announce future changes to
Perl, especially since this claim was made 9 years ago, in 6dbe9451, and
doesn't show any sign of becoming true.
---
 pod/perldiag.pod | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git pod/perldiag.pod pod/perldiag.pod
index bc086521..bd5039a8 100644
--- pod/perldiag.pod
+++ pod/perldiag.pod
@@ -2741,11 +2741,10 @@ either consume text or fail.
 
 =item Initialization of state variables in list context currently forbidden
 
-(F) Currently the implementation of "state" only permits the
-initialization of scalar variables in scalar context.  Re-write
-C<state ($a) = 42> as C<state $a = 42> to change from list to scalar
-context.  Constructions such as C<state (@a) = foo()> will be
-supported in a future perl release.
+(F) C<state> only permits initializing a single scalar variable, in scalar
+context. So C<state $a = 42> is allowed, but not C<state ($a) = 42>. To apply
+state semantics to a hash or array, store a hash or array reference in a scalar
+variable.
 
 =item %%s[%s] in scalar context better written as $%s[%s]
 
-- 
1.9.1

@p5pRT
Copy link
Author

p5pRT commented Aug 13, 2016

From @cpansprout

On Fri Aug 12 08​:47​:22 2016, smylers@​stripey.com wrote​:

So I think it's better to remove the claim about a future feature,
simply documenting the situation as it is now.

I also tweaked the wording to make it more obvious that state can only
be applied to a single scalar variable at at time, and gave a
suggestion
as to what to do if you're trying to use state with an array or hash.

Thank you. Applied as dca6023.

BTW, ‘git am’ version 1.7 wouldn’t accept the patch, which is missing the a/ and b/ file path prefixes that git used to produce (you seem to be using 1.9)​:

Inline Patch
diff --git pod/perldiag.pod pod/perldiag.pod
index bc086521..bd5039a8 100644
--- pod/perldiag.pod
+++ pod/perldiag.pod

Don’t git developers care about backward-compatibility? All these cosmetic changes do is make it harder for people to apply patches.

Is there an option in 1.9 to put the prefixes back?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 13, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Aug 13, 2016

@cpansprout - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented Aug 13, 2016

From @Smylers

Father Chrysostomos via RT writes​:

BTW, ‘git am’ version 1.7 wouldn’t accept the patch, which is missing
the a/ and b/ file path prefixes that git used to produce (you seem to
be using 1.9)​:

diff --git pod/perldiag.pod pod/perldiag.pod
index bc086521..bd5039a8 100644
--- pod/perldiag.pod
+++ pod/perldiag.pod

Ah, sorry. I have diff.noprefix set in my .gitconfig, so that I can
double-click a filename in git diff output to put it in the X selection.
(Without that it requires either more careful drag-selecting just the
filename without the prefix, or editing the prefix out after pasting it
somewhere.)

It's unhelpful (and arguably undocumented) that git format-patch takes
notice of that setting, too.

Don’t git developers care about backward-compatibility? All these
cosmetic changes do is make it harder for people to apply patches.

diff.noprefix was added in 1.7.2​:
https://www.kernel.org/pub/software/scm/git/docs/RelNotes-1.7.2.txt

Are you on that, or an earlier 1.7.x? Obviously it's disappointing that
git format-patch changed in non-backwards-compatible way, but it's even
worse if git am from the same version won't accept its output.

Is there an option in 1.9 to put the prefixes back?

I shall attempt to remember to temporarily comment-out that line in my
config the next time I'm submitting a patch. Thanks for alerting me to
this issue.

I can't immediately see I way of making the config setting only apply to
diff when directly invoked by me as git diff and not when used
internally by another git tool.

I also note that git format-patch has a --no-prefix switch (for people
who don't normally want to break their patches but do just this one
time?), but not a --prefix switch for overriding diff.noprefix if set.

Smylers

@p5pRT
Copy link
Author

p5pRT commented Aug 13, 2016

From @cpansprout

On Sat Aug 13 11​:46​:44 2016, smylers@​stripey.com wrote​:

Father Chrysostomos via RT writes​:

BTW, ‘git am’ version 1.7 wouldn’t accept the patch, which is missing
the a/ and b/ file path prefixes that git used to produce (you seem to
be using 1.9)​:

diff --git pod/perldiag.pod pod/perldiag.pod
index bc086521..bd5039a8 100644
--- pod/perldiag.pod
+++ pod/perldiag.pod

Ah, sorry. I have diff.noprefix set in my .gitconfig, so that I can
double-click a filename in git diff output to put it in the X selection.
(Without that it requires either more careful drag-selecting just the
filename without the prefix, or editing the prefix out after pasting it
somewhere.)

Ah, so I shouldn’t be blaming git developers.

It's unhelpful (and arguably undocumented) that git format-patch takes
notice of that setting, too.

Or maybe I should. git is filled with surprises like this action-at-a-distance and problems with things not round-tripping properly.

Don’t git developers care about backward-compatibility? All these
cosmetic changes do is make it harder for people to apply patches.

diff.noprefix was added in 1.7.2​:
https://www.kernel.org/pub/software/scm/git/docs/RelNotes-1.7.2.txt

Are you on that, or an earlier 1.7.x?

$ git --version
git version 1.7.12.4 (Apple Git-37)

Obviously it's disappointing that
git format-patch changed in non-backwards-compatible way, but it's even
worse if git am from the same version won't accept its output.

It won’t.

Is there an option in 1.9 to put the prefixes back?

I shall attempt to remember to temporarily comment-out that line in my
config the next time I'm submitting a patch. Thanks for alerting me to
this issue.

I can't immediately see I way of making the config setting only apply to
diff when directly invoked by me as git diff and not when used
internally by another git tool.

I also note that git format-patch has a --no-prefix switch (for people
who don't normally want to break their patches but do just this one
time?), but not a --prefix switch for overriding diff.noprefix if set.

I do see these options​:

$ git diff --src-prefix=a/ --dst-prefix=b/

In fact what the prefix consists of is irrelevant as long as it ends with a slash and contains no other slashes.

Hmm, as I was writing that it occurred to me that ./ might work. Indeed, it does. git am accepts it without complaint.

Can you put --src-prefix=./ --dst-prefix=./ in your config file? (I know very little about git config files. I have being trying hard to learn as little git as I can get away with. :-)

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 17, 2016

From @Smylers

Father Chrysostomos via RT writes​:

On Sat Aug 13 11​:46​:44 2016, smylers@​stripey.com wrote​:

Father Chrysostomos via RT writes​:

BTW, ‘git am’ version 1.7 wouldn’t accept the patch, which is
missing the a/ and b/ file path prefixes

Ah, sorry. I have diff.noprefix set in my .gitconfig, so that I can
double-click a filename in git diff output to put it in the X selection.

It's unhelpful (and arguably undocumented) that git format-patch
takes notice of that setting, too.

And unsurprisingly, we aren't the first pair of people to have
encountered this.

Don’t git developers care about backward-compatibility? All these
cosmetic changes do is make it harder for people to apply patches.

Reading bug reports and the Git mailing list, it seems that at least
several Git developers aren't keen on the existence of diff.noprefix,
and especially not in ~/.gitconfig. Unfortunately this sentiment is not
yet reflected in the manpage for git-config, despite suggestions of
doing so.

Obviously it's disappointing that git format-patch changed in
non-backwards-compatible way, but it's even worse if git am from the
same version won't accept its output.

It won’t.

There are suggestions that git am will apply a diff.noprefix patch if
you supply -p0 (not that you should have to do that, nor know when it's
required). There've also been suggestions of improving the git am (or
git apply) error message when -p1 fails to suggest -p0 if it looks like
that would work.

I shall attempt to remember to temporarily comment-out that line in
my config the next time I'm submitting a patch. Thanks for alerting
me to this issue.

I can't immediately see I way of making the config setting only
apply to diff when directly invoked by me as git diff and not when
used internally by another git tool.

I also note that git format-patch has a --no-prefix switch, but not
a --prefix switch for overriding diff.noprefix if set.

I do see these options​:

$ git diff --src-prefix=a/ --dst-prefix=b/

Yep, so if where perlhack.pod tells me to run​:

  % git format-patch -1

I instead do this​:

  % git format-patch --src-prefix=a/ --dst-prefix=b/ -1

then that should generate a patch that works for you.

Attached is a patch perlhack.pod to include those options (and of course
generated using its own advice). I'm not sure whether it should be
applied though​: it seems a little unwieldy to inflict the longer command
on all readers of that document when the additional parts will be
irrelevant to many of them.

The less-typing variant is to make git ignore ~/.gitconfig entirely. Not
that Git provides a handy option for that, but if you unset $HOME then
it can't find your config​:

  $ HOME= git format-patch -1

Credit​: http​://stackoverflow.com/a/23400888/1366011

However, the tricksiness of that, and its being a syntax error for C
Shell users, makes it a poor choice to recommend in perlhack.pod.

Hmm, as I was writing that it occurred to me that ./ might work.
Indeed, it does. git am accepts it without complaint.

Can you put --src-prefix=./ --dst-prefix=./ in your config file?

Unfortunately not. git's command-line options and config-file options
are separate*. Some command-line options have config-file equivalents,
but --(src|dst)-prefix don't appear to be among those.

Nor can I create an alias for git format-patch which includes them​: git
aliases can't override the names of built-in git commands†.

(I could of course create an alias with a different name to invoke
format-patch with the above options. But the existence of such an alias
wouldn't help us when, many months from now, I'm next looking up how to
submit a patch and copying commands verbatim from githack.pod.)

What I could do is unset diff.noprefix but create an alias called
something other than diff that does diff --no-prefix, and train my
fingers to use that instead.

Or maybe in each of my main project directories I could do​:

  $ ln -s . a
  $ ln -s . b

along with of course​:

  $ mkdir -p ~/.config/git
  $ echo -e "a\nb" >> ~/.config/git/ignore

Or maybe a Vim plugin that on a ‘file not found’ error strips any
leading a/ or b/ from the path and tries again?

Smylers

* Unlike in CVS, where config-file options simply are default
command-line options, ensuring that everything can be set in either
place.

† Unlike in CVS, where that's effectively the only thing they do.
(Aaron C tells me this was a specific design decision by Git developers
to aid with scripting, ensuring that standard commands always behave in
consistent ways — which is particularly ironic here, given that I'm
trying to de-customize git format-patch specifically to revert it to
behaving in a standard way.) And I can't believe I'm extolling CVS over
Git.

--
http​://twitter.com/Smylers2

@p5pRT
Copy link
Author

p5pRT commented Aug 17, 2016

From @Smylers

0001-Ensure-standard-diff-with-git-format-patch.patch
From 7d80ebc692ec88392e16c4544a3c8a896bd16e96 Mon Sep 17 00:00:00 2001
From: Smylers <Smylers@stripey.com>
Date: Wed, 17 Aug 2016 12:43:31 +0100
Subject: [PATCH] Ensure standard diff with git format-patch

git format-patch command in the Super Quick Patch Guide includes
overriding the contributor's local diff prefix config, so that the
supplied patch can be applied cleanly with git am.

See comments in RT #128910.
---
 pod/perlhack.pod | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pod/perlhack.pod b/pod/perlhack.pod
index 4e790633..029e46e1 100644
--- a/pod/perlhack.pod
+++ b/pod/perlhack.pod
@@ -69,7 +69,7 @@ via email.
 If your changes are in a single git commit, run the following commands
 to generate the patch file and attach it to your bug report:
 
-  % git format-patch -1
+  % git format-patch --src-prefix=a/ --dst-prefix=b/ -1
   % ./perl -Ilib utils/perlbug -p 0001-*.patch
 
 The perlbug program will ask you a few questions about your email
-- 
1.9.1

@p5pRT
Copy link
Author

p5pRT commented Aug 17, 2016

From @Tux

On Wed, 17 Aug 2016 13​:24​:31 +0100, Smylers <Smylers@​stripey.com> wrote​:

The less-typing variant is to make git ignore ~/.gitconfig entirely. Not
that Git provides a handy option for that, but if you unset $HOME then
it can't find your config​:

$ HOME= git format-patch -1

Credit​: http​://stackoverflow.com/a/23400888/1366011

However, the tricksiness of that, and its being a syntax error for C
Shell users, makes it a poor choice to recommend in perlhack.pod.

Being a tcsh user, I could suggest using env, which works alike for both

$ env HOME= git format-patch -1
% env HOME= git format-patch -1

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.25 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2016

From @arc

Smylers <Smylers@​stripey.com> wrote​:

I also note that git format-patch has a --no-prefix switch (for people
who don't normally want to break their patches but do just this one
time?), but not a --prefix switch for overriding diff.noprefix if set.

That's true. But you may be able to use this to work around that lack​:

git -c diff.noprefix=no format-patch ...

The general principle here is that you can insert any number of "-c
CONFIG-VAR=VAL" options between "git" and the name of a subcommand.

This doesn't help with the need to remember to add that to invocations
of a rarely-used command, of course, but it might be worth suggesting
in perlhack.pod.

--
Aaron Crane ** http​://aaroncrane.co.uk/

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2016

From @ap

* Smylers <Smylers@​stripey.com> [2016-08-13 20​:48]​:

I can't immediately see I way of making the config setting only apply
to diff when directly invoked by me as git diff and not when used
internally by another git tool.

Maybe `git config --global alias.d 'diff --no-prefix'` might be an idea?

Git is very ad-hoc about the boundary between plumbing and porcelain, so
it is generally less surprising to impose preferences through the use of
aliases while leaving the defaults in place, and only twiddling settings
when you’re sure you want to impose them globally.

Regards,
--
Aristotle Pagaltzis // <http​://plasmasturm.org/>

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.26.0, this and 210 other issues have been
resolved.

Perl 5.26.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.26.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant