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

Changes in regex recursion in 5.24 #15394

Closed
p5pRT opened this issue Jun 17, 2016 · 39 comments
Closed

Changes in regex recursion in 5.24 #15394

p5pRT opened this issue Jun 17, 2016 · 39 comments

Comments

@p5pRT
Copy link

p5pRT commented Jun 17, 2016

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

Searchable as RT128420$

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From jg@jgsoft.com

Hi.

While preparing RegexBuddy to support Perl 5.24 I discovered some
changes in the regex engine that I find hard to explain, and thus might
be bugs. In particular, regex recursion works differently, and I can't
see the logic behind the new behavior. It looks like backtracking into
the recursion isn't working correctly.

Here are some sample regexes along with the subject strings and the
matches they produce in 5.22 and 5.24.

re​: /aa$|a(?R)a|a/
subject​: "aaa"
5.22​: "aaa"
5.24​: "aa" (the first two, as if the second alternative in the regex
matched without the recursion)

re​: /(?​:\1|a)([bcd])\1(?​:(?R)|e)\1/
subject​: "abbaccaddedcb"
5.22​: "abbaccaddedcb"
5.24​: "added"

re​: /(.)\W*+(?R)\W*+\1|\W*+.\W*+/i
subject​: "A man, a plan, a canal​: Panama"
5.22​: "A man, a plan, a canal​: Panama"
5.24​: "A "

I also found that infinite recursion is no longer an error but is
treated as match failure. Perhaps the above differences between 5.22
and 5.24 are unintended side effects of attempting to fix any issues
with infinite recursion.

The perlbug page in the docs tells me you want the output of perl -V so
I have attached that.

I've attached a sample program that demonstrates the matches. X marks
the spot. Perl 5.22 matches the strings entirely​:

5.022002
X
X
X

Perl 5.24 finds partial matches​:

5.024000
Xa
abbaccXcb
Xman, a plan, a canal​: Panama

Please let me know whether the Perl developers consider this to be a
bug. If not, I'd like to understand the logic behind the behavior so
that we can make RegexBuddy emulate it.

Kind regards,
Jan Goyvaerts

--
http​://www.just-great-software.com

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From jg@jgsoft.com

Summary of my perl5 (revision 5 version 24 subversion 0) configuration​:
 
  Platform​:
  osname=MSWin32, osvers=6.3, archname=MSWin32-x64-multi-thread
  uname='Win32 strawberry-perl 5.24.0.1 #1 Tue May 10 21​:30​:49 2016 x64'
  config_args='undef'
  hint=recommended, useposix=true, d_sigaction=undef
  useithreads=define, usemultiplicity=define
  use64bitint=define, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='gcc', ccflags =' -s -O2 -DWIN32 -DWIN64 -DCONSERVATIVE -DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -fwrapv -fno-strict-aliasing -mms-bitfields',
  optimize='-s -O2',
  cppflags='-DWIN32'
  ccversion='', gccversion='4.9.2', gccosandvers=''
  intsize=4, longsize=4, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3
  ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='long long', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='g++', ldflags ='-s -L"C​:\strawberry\perl\lib\CORE" -L"C​:\strawberry\c\lib"'
  libpth=C​:\strawberry\c\lib C​:\strawberry\c\x86_64-w64-mingw32\lib C​:\strawberry\c\lib\gcc\x86_64-w64-mingw32\4.9.2
  libs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
  perllibs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
  libc=, so=dll, useshrplib=true, libperl=libperl524.a
  gnulibc_version=''
  Dynamic Linking​:
  dlsrc=dl_win32.xs, dlext=xs.dll, d_dlsymun=undef, ccdlflags=' '
  cccdlflags=' ', lddlflags='-mdll -s -L"C​:\strawberry\perl\lib\CORE" -L"C​:\strawberry\c\lib"'

Characteristics of this binary (from libperl)​:
  Compile-time options​: HAS_TIMES HAVE_INTERP_INTERN MULTIPLICITY
  PERLIO_LAYERS PERL_COPY_ON_WRITE
  PERL_DONT_CREATE_GVSV
  PERL_HASH_FUNC_ONE_AT_A_TIME_HARD
  PERL_IMPLICIT_CONTEXT PERL_IMPLICIT_SYS
  PERL_MALLOC_WRAP PERL_PRESERVE_IVUV USE_64_BIT_INT
  USE_ITHREADS USE_LARGE_FILES USE_LOCALE
  USE_LOCALE_COLLATE USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC USE_LOCALE_TIME USE_PERLIO
  USE_PERL_ATOF
  Built under MSWin32
  Compiled at May 10 2016 21​:42​:01
  @​INC​:
  G​:/Dev/Perl524/site/lib
  G​:/Dev/Perl524/vendor/lib
  G​:/Dev/Perl524/lib
  .

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From jg@jgsoft.com

Summary of my perl5 (revision 5 version 22 subversion 2) configuration​:
 
  Platform​:
  osname=MSWin32, osvers=6.3, archname=MSWin32-x64-multi-thread
  uname='Win32 strawberry-perl 5.22.2.1 #1 Sat Apr 30 17​:18​:06 2016 x64'
  config_args='undef'
  hint=recommended, useposix=true, d_sigaction=undef
  useithreads=define, usemultiplicity=define
  use64bitint=define, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='gcc', ccflags =' -s -O2 -DWIN32 -DWIN64 -DCONSERVATIVE -DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -fwrapv -fno-strict-aliasing -mms-bitfields',
  optimize='-s -O2',
  cppflags='-DWIN32'
  ccversion='', gccversion='4.9.2', gccosandvers=''
  intsize=4, longsize=4, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3
  ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='long long', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='g++', ldflags ='-s -L"C​:\strawberry\perl\lib\CORE" -L"C​:\strawberry\c\lib"'
  libpth=C​:\strawberry\c\lib C​:\strawberry\c\x86_64-w64-mingw32\lib C​:\strawberry\c\lib\gcc\x86_64-w64-mingw32\4.9.2
  libs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
  perllibs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
  libc=, so=dll, useshrplib=true, libperl=libperl522.a
  gnulibc_version=''
  Dynamic Linking​:
  dlsrc=dl_win32.xs, dlext=xs.dll, d_dlsymun=undef, ccdlflags=' '
  cccdlflags=' ', lddlflags='-mdll -s -L"C​:\strawberry\perl\lib\CORE" -L"C​:\strawberry\c\lib"'

Characteristics of this binary (from libperl)​:
  Compile-time options​: HAS_TIMES HAVE_INTERP_INTERN MULTIPLICITY
  PERLIO_LAYERS PERL_DONT_CREATE_GVSV
  PERL_HASH_FUNC_ONE_AT_A_TIME_HARD
  PERL_IMPLICIT_CONTEXT PERL_IMPLICIT_SYS
  PERL_MALLOC_WRAP PERL_NEW_COPY_ON_WRITE
  PERL_PRESERVE_IVUV USE_64_BIT_INT USE_ITHREADS
  USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE
  USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_LOCALE_TIME
  USE_PERLIO USE_PERL_ATOF
  Built under MSWin32
  Compiled at Apr 30 2016 17​:23​:36
  @​INC​:
  G​:/Dev/Perl522/site/lib
  G​:/Dev/Perl522/vendor/lib
  G​:/Dev/Perl522/lib
  .

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From jg@jgsoft.com

replace.pl

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From @dcollinsn

A few bisects later...

This was "broken" in​:

d5a00e4 is the first bad commit
commit d5a00e4
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Sat Mar 5 22​:04​:28 2016 +0100

  Unify GOSTART and GOSUB

  GOSTART is a special case of GOSUB, we can remove a lot of offset twiddling,
  and other special casing by unifying them, at pretty much no cost.

  GOSUB has 2 arguments, ARG() and ARG2L(), which are interpreted as
  a U32 and an I32 respectively. ARG() holds the "parno" we will recurse
  into. ARG2L() holds a signed offset to the relevant start node for the
  recursion.

  Prior to this patch the argument to GOSUB would always be >=, and unlike
  other parts of our logic we would not use 0 to represent "start/end" of
  pattern, as GOSTART would be used for "recurse to beginning of pattern",
  after this patch we use 0 to represent "start/end", and a lot of
  complexity "goes away" along with GOSTART regops.

:040000 040000 98c201349ff073a1e14744c0471ef3d9ab5fb584 a2df6849db8b2292fe1340f916f941c0e3c0f80f M pod
:100644 100644 f8f4b91df368dfbec28f74e030de78fc4b260095 b789ca30c71b1698e2b7c16abdfc6a88fcd960d2 M regcomp.c
:100644 100644 c08888e8f81a6cfcc848bb0c422c12574a22fe0f f16197f891b2aed9b705edfa7b85b63657db6a1c M regcomp.h
:100644 100644 8f9861ab6be65be243f9a02f3feb17d1dc05dc18 ac679552703103ac6f6137d5eb1cf838a0c3787b M regcomp.sym
:100644 100644 66dc245ddc68fb3c81a9e14ca3ea99310d249175 5e188fdc77e43e6a2f2bf578db31b329306dbdc8 M regexec.c
:100644 100644 ff44df27e7dfa6d64d451687e5641588ab310796 02258fe176d72cc9e6aede142bdeadb507d53072 M regexp.h
:100644 100644 f27abe0c7c4d43a4d17bcd05a031a671cd6e1ba0 f820c5684e6967f591ec0b0f276aaf4f65d911bb M regnodes.h
bisect run success
That took 725 seconds.

And it was "fixed" in​:

da7cf1c is the first bad commit
commit da7cf1c
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Tue May 10 09​:44​:31 2016 +0200

  fix #128109 - do not move RExC_open_parens[0] in reginsert

  In d5a00e4 I merged GOSUB and GOSTART,
  part of which involved making RExC_open_parens[0] refer to the start of
  the pattern, and RExC_close_parens[0] referring to the end of the pattern.

  This tripped up in reginsert in a subtle way, the start of the pattern
  cannot and should not move in reginsert(). Unlike a paren that might
  be at the start of the pattern which should move when something is inserted
  in front of it, the start is a fixed point and should never move.

  This patches fixes this up, and adds an assert to check that reginsert()
  is not called once study_chunk() starts, as reginsert() does not adjust
  RExC_recurse.

  This was noticed by hv while debugging [perl #128085], thanks hugo!

:100644 100644 c6823b51e82c55b39104ebb58d52a1a86d540a9e e6b352b2b796001d8501bf373b3fdf46956c9799 M regcomp.c
bisect run success
That took 429 seconds.

This should perhaps be included in 5.24.1?

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From @dcollinsn

Patch adds two of the requestor's testcases that I used for the bisect to re_tests for regression testing. Confirmed passing on blead, failing on 5.24.0.

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From @dcollinsn

0001-perl-128420-Add-tests-for-regex-recursion.patch
From 6965891ac125072bbd890010ea752a27b34b585f Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Fri, 17 Jun 2016 19:40:57 -0400
Subject: [PATCH] [perl #128420] Add tests for regex recursion

d5a00e4af introduced a bug in reginsert that was fixed by da7cf1cc7,
originally documented in [perl #128109]. This patch adds two
regression tests for the testcase reported by Jan Goyvaerts in
[perl #128420].
---
 t/re/re_tests | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/re/re_tests b/t/re/re_tests
index 34ac94a..7e8522d 100644
--- a/t/re/re_tests
+++ b/t/re/re_tests
@@ -1966,6 +1966,8 @@ ab(?#Comment){2}c	abbc	y	$&	abbc
 .{1}??	-	c	-	Nested quantifiers
 .{1}?+	-	c	-	Nested quantifiers
 (?:.||)(?|)000000000@	000000000@	y	$&	000000000@		#  [perl #126405]
+aa$|a(?R)a|a	aaa	y	$&	aaa		# [perl 128420] recursive matches
+(?:\1|a)([bcd])\1(?:(?R)|e)\1	abbaccaddedcb	y	$&	abbaccaddedcb		# [perl 128420] recursive match with backreferences
 
 # Keep these lines at the end of the file
 # vim: softtabstop=0 noexpandtab
-- 
2.8.1

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From [Unknown Contact. See original ticket]

Patch adds two of the requestor's testcases that I used for the bisect to re_tests for regression testing. Confirmed passing on blead, failing on 5.24.0.

@p5pRT
Copy link
Author

p5pRT commented Jun 18, 2016

From @khwilliamson

Thanks, applied as ec5bd22

I'm marking this ticket as a 5.24.1 blocker
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Jun 18, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Jun 19, 2016

From @demerphq

On 17 June 2016 at 03​:25, Jan Goyvaerts <perlbug-followup@​perl.org> wrote​:

# New Ticket Created by Jan Goyvaerts
# Please include the string​: [perl #128420]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=128420 >

Hi.

While preparing RegexBuddy to support Perl 5.24 I discovered some
changes in the regex engine that I find hard to explain, and thus might
be bugs. In particular, regex recursion works differently, and I can't
see the logic behind the new behavior. It looks like backtracking into
the recursion isn't working correctly.

Here are some sample regexes along with the subject strings and the
matches they produce in 5.22 and 5.24.

re​: /aa$|a(?R)a|a/
subject​: "aaa"
5.22​: "aaa"
5.24​: "aa" (the first two, as if the second alternative in the regex
matched without the recursion)

re​: /(?​:\1|a)([bcd])\1(?​:(?R)|e)\1/
subject​: "abbaccaddedcb"
5.22​: "abbaccaddedcb"
5.24​: "added"

re​: /(.)\W*+(?R)\W*+\1|\W*+.\W*+/i
subject​: "A man, a plan, a canal​: Panama"
5.22​: "A man, a plan, a canal​: Panama"
5.24​: "A "

I also found that infinite recursion is no longer an error but is
treated as match failure. Perhaps the above differences between 5.22
and 5.24 are unintended side effects of attempting to fix any issues
with infinite recursion.

Could very likely be. We tried to detect infinite recursion better
than we used to, and its possible that something broke as a side
effect.

The perlbug page in the docs tells me you want the output of perl -V so
I have attached that.

I've attached a sample program that demonstrates the matches. X marks
the spot. Perl 5.22 matches the strings entirely​:

5.022002
X
X
X

Perl 5.24 finds partial matches​:

5.024000
Xa
abbaccXcb
Xman, a plan, a canal​: Panama

Please let me know whether the Perl developers consider this to be a
bug. If not, I'd like to understand the logic behind the behavior so
that we can make RegexBuddy emulate

I will review this. If at all possible please test out blead as well.

There was some strangeness about the 5.24 release, and its possible a
patch that should have been included was not. If so it would be fixed
in 5.24.1 and in blead.

cheers,
Yves

@p5pRT
Copy link
Author

p5pRT commented Jun 19, 2016

From @demerphq

On 19 June 2016 at 08​:48, demerphq <demerphq@​gmail.com> wrote​:

On 17 June 2016 at 03​:25, Jan Goyvaerts <perlbug-followup@​perl.org> wrote​:

# New Ticket Created by Jan Goyvaerts
# Please include the string​: [perl #128420]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=128420 >

Hi.

While preparing RegexBuddy to support Perl 5.24 I discovered some
changes in the regex engine that I find hard to explain, and thus might
be bugs. In particular, regex recursion works differently, and I can't
see the logic behind the new behavior. It looks like backtracking into
the recursion isn't working correctly.

Here are some sample regexes along with the subject strings and the
matches they produce in 5.22 and 5.24.

re​: /aa$|a(?R)a|a/
subject​: "aaa"
5.22​: "aaa"
5.24​: "aa" (the first two, as if the second alternative in the regex
matched without the recursion)

re​: /(?​:\1|a)([bcd])\1(?​:(?R)|e)\1/
subject​: "abbaccaddedcb"
5.22​: "abbaccaddedcb"
5.24​: "added"

re​: /(.)\W*+(?R)\W*+\1|\W*+.\W*+/i
subject​: "A man, a plan, a canal​: Panama"
5.22​: "A man, a plan, a canal​: Panama"
5.24​: "A "

I also found that infinite recursion is no longer an error but is
treated as match failure. Perhaps the above differences between 5.22
and 5.24 are unintended side effects of attempting to fix any issues
with infinite recursion.

Could very likely be. We tried to detect infinite recursion better
than we used to, and its possible that something broke as a side
effect.

The perlbug page in the docs tells me you want the output of perl -V so
I have attached that.

I've attached a sample program that demonstrates the matches. X marks
the spot. Perl 5.22 matches the strings entirely​:

5.022002
X
X
X

Perl 5.24 finds partial matches​:

5.024000
Xa
abbaccXcb
Xman, a plan, a canal​: Panama

Please let me know whether the Perl developers consider this to be a
bug. If not, I'd like to understand the logic behind the behavior so
that we can make RegexBuddy emulate

I will review this. If at all possible please test out blead as well.

There was some strangeness about the 5.24 release, and its possible a
patch that should have been included was not. If so it would be fixed
in 5.24.1 and in blead.

Yeah, this is a regression in 5.24 that should be fixed in 5.24.1
(once we definitively find the patch(es) responsible), but we should
add your test cases to our test suite to ensure that this does not
happen again.

Like one way or another this is my fault. My apologies for any inconvenience.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Apr 10, 2017

From jg@jgsoft.com

yves orton via RT wrote​:

Yeah, this is a regression in 5.24 that should be fixed in 5.24.1

I get the exact same behavior in 5.24.1 as in 5.24.0. Is this still
scheduled to be fixed in 5.24 or will the fix have to wait for 5.26?

Kind regards,
Jan Goyvaerts

--
http​://www.just-great-software.com

@p5pRT
Copy link
Author

p5pRT commented Apr 10, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Apr 11, 2017

From @xsawyerx

At this point, I think 5.26.1 and 5.24.3 might be better for this.

On 04/10/2017 02​:27 AM, Jan Goyvaerts wrote​:

yves orton via RT wrote​:

Yeah, this is a regression in 5.24 that should be fixed in 5.24.1

I get the exact same behavior in 5.24.1 as in 5.24.0. Is this still
scheduled to be fixed in 5.24 or will the fix have to wait for 5.26?

Kind regards,
Jan Goyvaerts

--
http​://www.just-great-software.com

@p5pRT
Copy link
Author

p5pRT commented Apr 11, 2017

From @demerphq

On 11 April 2017 at 12​:09, Sawyer X <xsawyerx@​gmail.com> wrote​:

At this point, I think 5.26.1 and 5.24.3 might be better for this.

On 04/10/2017 02​:27 AM, Jan Goyvaerts wrote​:

yves orton via RT wrote​:

Yeah, this is a regression in 5.24 that should be fixed in 5.24.1

I get the exact same behavior in 5.24.1 as in 5.24.0. Is this still
scheduled to be fixed in 5.24 or will the fix have to wait for 5.26?

You want me to backport the patch?

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Apr 11, 2017

From jg@jgsoft.com

demerphq wrote​:

On 04/10/2017 02​:27 AM, Jan Goyvaerts wrote​:

yves orton via RT wrote​:

Yeah, this is a regression in 5.24 that should be fixed in 5.24.1
I get the exact same behavior in 5.24.1 as in 5.24.0. Is this still
scheduled to be fixed in 5.24 or will the fix have to wait for 5.26?

You want me to backport the patch?

You can do whatever you think is best for the Perl community. I just
want to know what the plan is.

I'm the developer of RegexBuddy which emulates the regex features in
many applications and programming languages, including Perl 5.8 through
5.22. Basically, I need to decide whether to spend the time to make
RegexBuddy emulate the buggy behavior in Perl 5.24 (or determine the
situations in which the bug occurs and flag those as errors). I've
delayed supporting Perl 5.24 because I was under the impression that it
was going to be fixed in Perl 5.24.1 which would render the effort of
emulating it unnecessary.

Another change I've noticed in Perl 5.24.0 and 5.24.1 compared with
previous versions is that [[​:AlPhA​:]] and [[​:w​:]] (and seemingly every
other single letter class) are no longer errors, but also doesn't seem
to match anything (or at least nothing obvious that I could figure out).

Kind regards,
Jan Goyvaerts

--
http​://www.just-great-software.com

@p5pRT
Copy link
Author

p5pRT commented Apr 11, 2017

From @demerphq

On 11 April 2017 at 12​:36, Jan Goyvaerts <jg@​jgsoft.com> wrote​:

demerphq wrote​:

On 04/10/2017 02​:27 AM, Jan Goyvaerts wrote​:

yves orton via RT wrote​:

Yeah, this is a regression in 5.24 that should be fixed in 5.24.1

I get the exact same behavior in 5.24.1 as in 5.24.0. Is this still
scheduled to be fixed in 5.24 or will the fix have to wait for 5.26?

You want me to backport the patch?

You can do whatever you think is best for the Perl community. I just want
to know what the plan is.

That was aimed at Sawyer X. :-)

I will put together a patch for this.

I'm the developer of RegexBuddy which emulates the regex features in many
applications and programming languages, including Perl 5.8 through 5.22.
Basically, I need to decide whether to spend the time to make RegexBuddy
emulate the buggy behavior in Perl 5.24 (or determine the situations in
which the bug occurs and flag those as errors). I've delayed supporting
Perl 5.24 because I was under the impression that it was going to be fixed
in Perl 5.24.1 which would render the effort of emulating it unnecessary.

I was under the impression that the patch was already applied to the
Perl 5.24 branch. This thread bobbing up again is the first I've
thought about this for ages. Very for sorry for any trouble this has
caused you.

Another change I've noticed in Perl 5.24.0 and 5.24.1 compared with previous
versions is that [[​:AlPhA​:]] and [[​:w​:]] (and seemingly every other single
letter class) are no longer errors, but also doesn't seem to match anything
(or at least nothing obvious that I could figure out).

If you notice things like that could file a new ticket please? I am
sure Karl will want to know.

cheers,
Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Apr 11, 2017

From @khwilliamson

On 04/11/2017 05​:02 AM, demerphq wrote​:

On 11 April 2017 at 12​:36, Jan Goyvaerts <jg@​jgsoft.com> wrote​:
Another change I've noticed in Perl 5.24.0 and 5.24.1 compared with previous
versions is that [[​:AlPhA​:]] and [[​:w​:]] (and seemingly every other single
letter class) are no longer errors, but also doesn't seem to match anything
(or at least nothing obvious that I could figure out).

If you notice things like that could file a new ticket please? I am
sure Karl will want to know.

These are now legal. The first one warns if warnings are enabled, as
they always should be​:

  Assuming NOT a POSIX class since the name must be all lowercase letters
in regex; marked by <-- HERE in m/[[​:AlPhA​:] <-- HERE ]/ at -e line 1.

The single letter constructs, which you assume are meant to be Posix
classes, don't warn, as none of them are close enough to a legal class
name that they could have been intended to be one. That could be
changed if there was evidence that people think [[​:w​:]] might mean
something. Maybe this is legal in other languages? Perl uses the names
in the Posix standard, plus a couple of extensions.

If you want to see what a pattern compiles into, and hence what it matches,

use re qw(Debug COMPILE);
qr/pattern/;
no re;

That shows that [[​:w​:]] matches a string that contains a ']' immediately
preceded by one of '[', '​:', or 'w'.

It's impossible to figure out for sure what was the programmer's intent.

@p5pRT
Copy link
Author

p5pRT commented Apr 12, 2017

From jg@jgsoft.com

Karl Williamson wrote​:

The single letter constructs, which you assume are meant to be Posix
classes, don't warn, as none of them are close enough to a legal class
name that they could have been intended to be one.

Dinkumware's implementation of std​::regex (and possibly other
implementations) support [​:w​:], [​:s], and [​:d​:] as shorthands for
[​:word​:], [​:space​:], and [​:digit​:].

Boost​::regex additionally supports [​:v​:] and [​:h​:] to match vertical and
horizontal whitespace.

If you want to see what a pattern compiles into, and hence what it
matches,

Thanks for the tip.

That shows that [[​:w​:]] matches a string that contains a ']'
immediately preceded by one of '[', '​:', or 'w'.

It's impossible to figure out for sure what was the programmer's intent.

If the programmer intended to add a literal : to the character class
then a single : would suffice.

Anyway, Perl 5.24's behavior is fine. It's just that the change took me
by surprise. It certainly doesn't break any existing regexes when
porting code from 5.22 to 5.24.

Kind regards,
Jan Goyvaerts

--
http​://www.just-great-software.com

@p5pRT
Copy link
Author

p5pRT commented Apr 12, 2017

From @demerphq

On 12 April 2017 at 08​:02, Jan Goyvaerts <jg@​jgsoft.com> wrote​:

Karl Williamson wrote​:

The single letter constructs, which you assume are meant to be Posix
classes, don't warn, as none of them are close enough to a legal class
name that they could have been intended to be one.

Dinkumware's implementation of std​::regex (and possibly other
implementations) support [​:w​:], [​:s], and [​:d​:] as shorthands for [​:word​:],
[​:space​:], and [​:digit​:].

Do you know why? Do they not support \w \s and \d?

Boost​::regex additionally supports [​:v​:] and [​:h​:] to match vertical and
horizontal whitespace.

I dont think we handle POSIX charclasses very cleanly. We warn about
misplaced "^" negation, but we dont warn about mistyped POSIX names,
which means we have not properly reserved the POSIX namespace for our
own use, and we let people typo themselves into incorrect matches. IMO
silently treating a typoed POSIX charclass as its constituent
characters is an insidious thing to do.

1. I think in the next release we should produce a deprecation warning
when we see anything that is POSIX-char-class-like but which is
invalid.

2. Then in a release or so afterwards we could make them errors.

3. Then in a release or so after that we could add support for these
short forms if we wanted.

IMO, even if we dont do step 3 we should step 1 and 2.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Apr 12, 2017

From jg@jgsoft.com

demerphq wrote​:

Dinkumware's implementation of std​::regex (and possibly other
implementations) support [​:w​:], [​:s], and [​:d​:] as shorthands for [​:word​:],
[​:space​:], and [​:digit​:].

Do you know why? Do they not support \w \s and \d?

std​::regex supports \w, \s, and \d when using its "ECMAScript" grammar
but not when using the basic, extended, grep, egrep, or awk grammars.

boost​::regex supports \w, \s, \d, and \h in all grammars. It does not
support \v as a shorthand. So [[​:v​:]] is the only concise way of
matching vertical whitespace in Boost.

1. I think in the next release we should produce a deprecation warning
when we see anything that is POSIX-char-class-like but which is
invalid.

2. Then in a release or so afterwards we could make them errors.

3. Then in a release or so after that we could add support for these
short forms if we wanted.

Sounds like a good plan to me. Anyway, RegexBuddy will emulate whatever
you guys decide.

Another inconsistency you have right now is that Perl 5.24 does throw
errors about unknown POSIX classes for [[​:a-z​:]] and [[​:a!z​:]] but not
for [[​:az​:]] or [[​:A-Z​:]].

Kind regards,
Jan Goyvaerts

--
http​://www.just-great-software.com

@p5pRT
Copy link
Author

p5pRT commented May 1, 2017

From jg@jgsoft.com

Hi.

Running this under Strawberry Perl 5.24.1

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {
  print "$&\n";
} else {
  print "fail\n";
}

causes the Perl interpreter to crash.

In Perl 5.22 it correctly matches the whole subject string.

Kind regards,
Jan Goyvaerts

--
http​://www.just-great-software.com

@p5pRT
Copy link
Author

p5pRT commented May 1, 2017

From zefram@fysh.org

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

-zefram

@p5pRT
Copy link
Author

p5pRT commented May 2, 2017

From @iabyn

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

  fix #128109 - do not move RExC_open_parens[0] in reginsert

--
Wesley Crusher gets beaten up by his classmates for being a smarmy git,
and consequently has a go at making some friends of his own age for a
change.
  -- Things That Never Happen in "Star Trek" #18

@p5pRT
Copy link
Author

p5pRT commented May 2, 2017

@iabyn - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed May 2, 2017
@p5pRT
Copy link
Author

p5pRT commented May 2, 2017

From @ilmari

Dave Mitchell <davem@​iabyn.com> writes​:

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

fix \#128109 \- do not move RExC\_open\_parens\[0\] in reginsert

That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it,
cherry-pick cleanly to maint-5.24. Seeing as it's a crasher and
regression from 5.22, it's a candidate for backporting according to
perlpolicy.

--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen

@p5pRT
Copy link
Author

p5pRT commented May 2, 2017

From @demerphq

On 2 May 2017 at 17​:39, Dagfinn Ilmari Mannsåker <ilmari@​ilmari.org> wrote​:

Dave Mitchell <davem@​iabyn.com> writes​:

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

fix \#128109 \- do not move RExC\_open\_parens\[0\] in reginsert

That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it,
cherry-pick cleanly to maint-5.24. Seeing as it's a crasher and
regression from 5.22, it's a candidate for backporting according to
perlpolicy.

I just pushed

78b60e1 Add tests for regex recursion
4539ae3 fix #128109 - do not move RExC_open_parens[0] in reginsert
5edefd5 fix #128085 - SIGSEGV in S_regmatch with S_study_chunk​:
Assertion "!frame" failed.

to maint-5.24 -- I took the liberty of including 5edefd5 which was
part of how I found out about #128109

These patches are equivalent to (respectively)​:

ec5bd22
da7cf1c
1bda7a7

Cheers,
Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented May 2, 2017

From @demerphq

On 11 April 2017 at 12​:36, Jan Goyvaerts <jg@​jgsoft.com> wrote​:

demerphq wrote​:

On 04/10/2017 02​:27 AM, Jan Goyvaerts wrote​:

yves orton via RT wrote​:

Yeah, this is a regression in 5.24 that should be fixed in 5.24.1

I get the exact same behavior in 5.24.1 as in 5.24.0. Is this still
scheduled to be fixed in 5.24 or will the fix have to wait for 5.26?

You want me to backport the patch?

You can do whatever you think is best for the Perl community. I just want
to know what the plan is.

I'm the developer of RegexBuddy which emulates the regex features in many
applications and programming languages, including Perl 5.8 through 5.22.
Basically, I need to decide whether to spend the time to make RegexBuddy
emulate the buggy behavior in Perl 5.24 (or determine the situations in
which the bug occurs and flag those as errors).

Rereading this thread I realized I did not comment on this, so i will now.

I would not emulate the buggy behavior.

I've delayed supporting
Perl 5.24 because I was under the impression that it was going to be fixed
in Perl 5.24.1 which would render the effort of emulating it unnecessary.

The patches are now pushed to the maint branch, so the next release of
5.24 will have the fix in it.

Yves

@p5pRT
Copy link
Author

p5pRT commented May 2, 2017

From @steve-m-hay

On 2 May 2017 at 18​:38, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 17​:39, Dagfinn Ilmari Mannsåker <ilmari@​ilmari.org> wrote​:

Dave Mitchell <davem@​iabyn.com> writes​:

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

fix \#128109 \- do not move RExC\_open\_parens\[0\] in reginsert

That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it,
cherry-pick cleanly to maint-5.24. Seeing as it's a crasher and
regression from 5.22, it's a candidate for backporting according to
perlpolicy.

I just pushed

78b60e1 Add tests for regex recursion
4539ae3 fix #128109 - do not move RExC_open_parens[0] in reginsert
5edefd5 fix #128085 - SIGSEGV in S_regmatch with S_study_chunk​:
Assertion "!frame" failed.

to maint-5.24 -- I took the liberty of including 5edefd5 which was
part of how I found out about #128109

I don't think this should have been done right now. maint-5.22 and
maint-5.24 are both still in code freeze pending the release of 5.22.4
and 5.24.2, which are intended only to contain the last @​INC fixes
(and possibly some other security fixes, as per the previous releases
on those branches).

There are many, many other bug fixes (crashes and otherwise) that
could be backported, but haven't been yet. They were deliberately
being left for 5.24.3, once the last of the @​INC fixes was out of the
way.

@p5pRT
Copy link
Author

p5pRT commented May 3, 2017

From @demerphq

On 2 May 2017 at 22​:49, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 2 May 2017 at 18​:38, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 17​:39, Dagfinn Ilmari Mannsåker <ilmari@​ilmari.org> wrote​:

Dave Mitchell <davem@​iabyn.com> writes​:

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

fix \#128109 \- do not move RExC\_open\_parens\[0\] in reginsert

That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it,
cherry-pick cleanly to maint-5.24. Seeing as it's a crasher and
regression from 5.22, it's a candidate for backporting according to
perlpolicy.

I just pushed

78b60e1 Add tests for regex recursion
4539ae3 fix #128109 - do not move RExC_open_parens[0] in reginsert
5edefd5 fix #128085 - SIGSEGV in S_regmatch with S_study_chunk​:
Assertion "!frame" failed.

to maint-5.24 -- I took the liberty of including 5edefd5 which was
part of how I found out about #128109

I don't think this should have been done right now. maint-5.22 and
maint-5.24 are both still in code freeze pending the release of 5.22.4
and 5.24.2, which are intended only to contain the last @​INC fixes
(and possibly some other security fixes, as per the previous releases
on those branches).

There are many, many other bug fixes (crashes and otherwise) that
could be backported, but haven't been yet. They were deliberately
being left for 5.24.3, once the last of the @​INC fixes was out of the
way.

Sigh. Honestly I give up. Our release process is broken. If it weren't
for the fact that we are going to change these policies that I would
insert a long rant here about how expecting our developers to keep
track of all their patches, which branches they have and have not been
merged and which they should be merged to in some distant point in
time is not a good way manage things[1]. Just like distributing swords
in lakes in the middle of the night not a sound way to form a
government. :-)

Anyway, bulk reverted in 9ecaf4d
sorry for yet more revert turbulence.

cheers,
Yves
[1] So here is the *short* rant​: I know there are some on this list
that one way or another do not find these types of things a burden.
But processes should work for everybody, not just the meticulous and
careful, and they should be robust to things like unexpected
life-changes. This patch was written in a period when I was dealing
with the death of my father and a very sick mother. Making sure the
patch was backported at the right time so that it didn't violate a
code freeze was simply not a priority for me, and as our release
procedures and policies do not provide for any solution other than
"remember yourself", no-one else was going to do it either. This to me
says that the policies and procedures are broken by design.

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented May 3, 2017

From @steve-m-hay

On 3 May 2017 at 07​:56, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 22​:49, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 2 May 2017 at 18​:38, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 17​:39, Dagfinn Ilmari Mannsåker <ilmari@​ilmari.org> wrote​:

Dave Mitchell <davem@​iabyn.com> writes​:

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

fix \#128109 \- do not move RExC\_open\_parens\[0\] in reginsert

That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it,
cherry-pick cleanly to maint-5.24. Seeing as it's a crasher and
regression from 5.22, it's a candidate for backporting according to
perlpolicy.

I just pushed

78b60e1 Add tests for regex recursion
4539ae3 fix #128109 - do not move RExC_open_parens[0] in reginsert
5edefd5 fix #128085 - SIGSEGV in S_regmatch with S_study_chunk​:
Assertion "!frame" failed.

to maint-5.24 -- I took the liberty of including 5edefd5 which was
part of how I found out about #128109

I don't think this should have been done right now. maint-5.22 and
maint-5.24 are both still in code freeze pending the release of 5.22.4
and 5.24.2, which are intended only to contain the last @​INC fixes
(and possibly some other security fixes, as per the previous releases
on those branches).

There are many, many other bug fixes (crashes and otherwise) that
could be backported, but haven't been yet. They were deliberately
being left for 5.24.3, once the last of the @​INC fixes was out of the
way.

Sigh. Honestly I give up. Our release process is broken. If it weren't
for the fact that we are going to change these policies that I would
insert a long rant here about how expecting our developers to keep
track of all their patches, which branches they have and have not been
merged and which they should be merged to in some distant point in
time is not a good way manage things[1]. Just like distributing swords
in lakes in the middle of the night not a sound way to form a
government. :-)

Anyway, bulk reverted in 9ecaf4d
sorry for yet more revert turbulence.

Thanks.

cheers,
Yves
[1] So here is the *short* rant​: I know there are some on this list
that one way or another do not find these types of things a burden.
But processes should work for everybody, not just the meticulous and
careful, and they should be robust to things like unexpected
life-changes. This patch was written in a period when I was dealing
with the death of my father and a very sick mother. Making sure the
patch was backported at the right time so that it didn't violate a
code freeze was simply not a priority for me, and as our release
procedures and policies do not provide for any solution other than
"remember yourself", no-one else was going to do it either. This to me
says that the policies and procedures are broken by design.

I wasn't aware that the discussions over changes in branch management
were intended to cover maint releases too.

Regarding remembering that patches get backported, I think the
existing process is very simple, actually​: Just add the patch to the
relevant voting file in the maint-votes branch and the maint release
manager will pick it up at the approrpiate point in time. That's the
whole point of having a maint release manager isn't it? -- so that
individual developers don't have to remember all these things
themselves. If ever a patch is non-trivial to backport then I will
seek assistance when I come to cherry-pick it, which is something that
I've done on several occasions in the past.

Also, patches are supposed to get three votes before being backported
-- something else that I didn't see happen on this occasion. Again,
the voting file takes care of that perfectly simply.

It's not obvious to me that this process needs to change since it
doesn't suffer from the "blockage" issue that blead has during a long
code freeze​: There is no active development being done on maint
branches anyway (virtually by definition).

@p5pRT
Copy link
Author

p5pRT commented May 3, 2017

From @demerphq

On 3 May 2017 at 09​:25, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 3 May 2017 at 07​:56, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 22​:49, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 2 May 2017 at 18​:38, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 17​:39, Dagfinn Ilmari Mannsåker <ilmari@​ilmari.org> wrote​:

Dave Mitchell <davem@​iabyn.com> writes​:

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

fix \#128109 \- do not move RExC\_open\_parens\[0\] in reginsert

That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it,
cherry-pick cleanly to maint-5.24. Seeing as it's a crasher and
regression from 5.22, it's a candidate for backporting according to
perlpolicy.

I just pushed

78b60e1 Add tests for regex recursion
4539ae3 fix #128109 - do not move RExC_open_parens[0] in reginsert
5edefd5 fix #128085 - SIGSEGV in S_regmatch with S_study_chunk​:
Assertion "!frame" failed.

to maint-5.24 -- I took the liberty of including 5edefd5 which was
part of how I found out about #128109

I don't think this should have been done right now. maint-5.22 and
maint-5.24 are both still in code freeze pending the release of 5.22.4
and 5.24.2, which are intended only to contain the last @​INC fixes
(and possibly some other security fixes, as per the previous releases
on those branches).

There are many, many other bug fixes (crashes and otherwise) that
could be backported, but haven't been yet. They were deliberately
being left for 5.24.3, once the last of the @​INC fixes was out of the
way.

Sigh. Honestly I give up. Our release process is broken. If it weren't
for the fact that we are going to change these policies that I would
insert a long rant here about how expecting our developers to keep
track of all their patches, which branches they have and have not been
merged and which they should be merged to in some distant point in
time is not a good way manage things[1]. Just like distributing swords
in lakes in the middle of the night not a sound way to form a
government. :-)

Anyway, bulk reverted in 9ecaf4d
sorry for yet more revert turbulence.

Thanks.

cheers,
Yves
[1] So here is the *short* rant​: I know there are some on this list
that one way or another do not find these types of things a burden.
But processes should work for everybody, not just the meticulous and
careful, and they should be robust to things like unexpected
life-changes. This patch was written in a period when I was dealing
with the death of my father and a very sick mother. Making sure the
patch was backported at the right time so that it didn't violate a
code freeze was simply not a priority for me, and as our release
procedures and policies do not provide for any solution other than
"remember yourself", no-one else was going to do it either. This to me
says that the policies and procedures are broken by design.

I wasn't aware that the discussions over changes in branch management
were intended to cover maint releases too.

I had no direct thoughts on this, but when i wrote that mail I was
thinking that they kinda flow together. However some of what you say
below makes me reconsider.

Regarding remembering that patches get backported, I think the
existing process is very simple, actually​: Just add the patch to the
relevant voting file in the maint-votes branch and the maint release
manager will pick it up at the approrpiate point in time. That's the
whole point of having a maint release manager isn't it? -- so that
individual developers don't have to remember all these things
themselves. If ever a patch is non-trivial to backport then I will
seek assistance when I come to cherry-pick it, which is something that
I've done on several occasions in the past.

Ok, I think I missed some of these details being announced. However I
see no mention of a maint-votes branch in the docs, at least not the
docs for 5.24.

Also this maint-votes branch is pretty clunky. Why don't we just put a
file in blead? Then when you commit your patch you can do a follow up
patch to the file. We can exclude the file from the build process when
we do a release.

Anyway, if there is a file I can edit for these things then obviously
some of what I complained about has been addressed already.

Also, patches are supposed to get three votes before being backported
-- something else that I didn't see happen on this occasion. Again,
the voting file takes care of that perfectly simply.

I was under the impression that there were, at least for two of the
three patches. For the latter I interpreted the approval for the other
a bit loosely and included it too, as they were all part of the same
meta-bug, if not specific bug-report.

It's not obvious to me that this process needs to change since it
doesn't suffer from the "blockage" issue that blead has during a long
code freeze​: There is no active development being done on maint
branches anyway (virtually by definition).

No. I withdraw that. Albeit clunkier than I would like there is a
procedure to fire-and-forget, so the problem I complained about is
solved and I just didn't know it fully. I apologise for the noise, and
for not getting with the program quicker.

On the other hand, I then think we should put push blocks on these
branches and lock them down to the person who is doing the branch
management. I was just trying to "take care of business" without
realizing I was breaking the rules. IMO technical provisions to
prevent that kind of thing are a good idea.

If I had pushed and gotten back a message back telling me what to do I
think things would work smoother. I mean even if everybody knows the
policy, people make mistakes, etc. I could imagine something like​:

###########################################################################
Push refused - You are not the branch maintainer. What you probably
want to do is push your commits to a temporary branch and then update
maint-votes branch, which you can do with something like​:

  git push origin $branch​:backports/$branch-TOPIC
  perl Porting/add-backport-patch TOPIC $sha1 $sha2 $sha3 ...
##########################################################################

And then we write add-backport-patch to do whatever vote file git
ker-fsckery necessary to add the patches to the vote file. (And
insulate anybody doing this from however we store the file. This whole
dangling branch thing is cute, but annoying.)

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented May 3, 2017

From @steve-m-hay

On 3 May 2017 at 09​:00, demerphq <demerphq@​gmail.com> wrote​:

On 3 May 2017 at 09​:25, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 3 May 2017 at 07​:56, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 22​:49, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 2 May 2017 at 18​:38, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 17​:39, Dagfinn Ilmari Mannsåker <ilmari@​ilmari.org> wrote​:

Dave Mitchell <davem@​iabyn.com> writes​:

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

fix \#128109 \- do not move RExC\_open\_parens\[0\] in reginsert

That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it,
cherry-pick cleanly to maint-5.24. Seeing as it's a crasher and
regression from 5.22, it's a candidate for backporting according to
perlpolicy.

I just pushed

78b60e1 Add tests for regex recursion
4539ae3 fix #128109 - do not move RExC_open_parens[0] in reginsert
5edefd5 fix #128085 - SIGSEGV in S_regmatch with S_study_chunk​:
Assertion "!frame" failed.

to maint-5.24 -- I took the liberty of including 5edefd5 which was
part of how I found out about #128109

I don't think this should have been done right now. maint-5.22 and
maint-5.24 are both still in code freeze pending the release of 5.22.4
and 5.24.2, which are intended only to contain the last @​INC fixes
(and possibly some other security fixes, as per the previous releases
on those branches).

There are many, many other bug fixes (crashes and otherwise) that
could be backported, but haven't been yet. They were deliberately
being left for 5.24.3, once the last of the @​INC fixes was out of the
way.

Sigh. Honestly I give up. Our release process is broken. If it weren't
for the fact that we are going to change these policies that I would
insert a long rant here about how expecting our developers to keep
track of all their patches, which branches they have and have not been
merged and which they should be merged to in some distant point in
time is not a good way manage things[1]. Just like distributing swords
in lakes in the middle of the night not a sound way to form a
government. :-)

Anyway, bulk reverted in 9ecaf4d
sorry for yet more revert turbulence.

Thanks.

cheers,
Yves
[1] So here is the *short* rant​: I know there are some on this list
that one way or another do not find these types of things a burden.
But processes should work for everybody, not just the meticulous and
careful, and they should be robust to things like unexpected
life-changes. This patch was written in a period when I was dealing
with the death of my father and a very sick mother. Making sure the
patch was backported at the right time so that it didn't violate a
code freeze was simply not a priority for me, and as our release
procedures and policies do not provide for any solution other than
"remember yourself", no-one else was going to do it either. This to me
says that the policies and procedures are broken by design.

I wasn't aware that the discussions over changes in branch management
were intended to cover maint releases too.

I had no direct thoughts on this, but when i wrote that mail I was
thinking that they kinda flow together. However some of what you say
below makes me reconsider.

Regarding remembering that patches get backported, I think the
existing process is very simple, actually​: Just add the patch to the
relevant voting file in the maint-votes branch and the maint release
manager will pick it up at the approrpiate point in time. That's the
whole point of having a maint release manager isn't it? -- so that
individual developers don't have to remember all these things
themselves. If ever a patch is non-trivial to backport then I will
seek assistance when I come to cherry-pick it, which is something that
I've done on several occasions in the past.

Ok, I think I missed some of these details being announced. However I
see no mention of a maint-votes branch in the docs, at least not the
docs for 5.24.

You're right - there is no mention of it in the docs (even in blead),
which amazes me. We've been using this process for some time now, and
documenting it after some trial period has evidently slipped through
the cracks. Sorry about that. I will try to fix that, though perhaps
it should wait until we've made any changes in light of your proposals
below.

Also this maint-votes branch is pretty clunky. Why don't we just put a
file in blead? Then when you commit your patch you can do a follow up
patch to the file. We can exclude the file from the build process when
we do a release.

I think this may have been discussed at the time the voting files were
introduced, and there were some objections to having a file in blead
because the votes being cast would then cause noise on the blead
commit history.

Anyway, if there is a file I can edit for these things then obviously
some of what I complained about has been addressed already.

Also, patches are supposed to get three votes before being backported
-- something else that I didn't see happen on this occasion. Again,
the voting file takes care of that perfectly simply.

I was under the impression that there were, at least for two of the
three patches. For the latter I interpreted the approval for the other
a bit loosely and included it too, as they were all part of the same
meta-bug, if not specific bug-report.

Sorry if I missed that. This is why having a file *somewhere* to keep
track of the votes is definitely a good idea (IMO).

It's not obvious to me that this process needs to change since it
doesn't suffer from the "blockage" issue that blead has during a long
code freeze​: There is no active development being done on maint
branches anyway (virtually by definition).

No. I withdraw that. Albeit clunkier than I would like there is a
procedure to fire-and-forget, so the problem I complained about is
solved and I just didn't know it fully. I apologise for the noise, and
for not getting with the program quicker.

On the other hand, I then think we should put push blocks on these
branches and lock them down to the person who is doing the branch
management. I was just trying to "take care of business" without
realizing I was breaking the rules. IMO technical provisions to
prevent that kind of thing are a good idea.

I think I would welcome the idea of push blocks (on the proposed blead
release branch too) during code freeze because that's when it becomes
most important to stop accidental commits, but I'm not sure they'd be
necessary the rest of the time. Other committers have pushed to maint
branches in the past. As long as it's well ahead of a release and
follows the voting rules then I have no problem with that (it saves me
a bit of work!). Would it be easy for the branch manager to add/remove
these push blocks, or is it a more complicated thing to do that (e.g.)
Dennis would have to do, and once it's done it's done?

If I had pushed and gotten back a message back telling me what to do I
think things would work smoother. I mean even if everybody knows the
policy, people make mistakes, etc. I could imagine something like​:

###########################################################################
Push refused - You are not the branch maintainer. What you probably
want to do is push your commits to a temporary branch and then update
maint-votes branch, which you can do with something like​:

git push origin $branch​:backports/$branch-TOPIC
perl Porting/add-backport-patch TOPIC $sha1 $sha2 $sha3 ...
##########################################################################

And then we write add-backport-patch to do whatever vote file git
ker-fsckery necessary to add the patches to the vote file. (And
insulate anybody doing this from however we store the file. This whole
dangling branch thing is cute, but annoying.)

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented May 3, 2017

From @demerphq

[using * to mark my comments since apparently gmail html to text is
broken for quoting]
On 3 May 2017 at 10​:19, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 3 May 2017 at 09​:00, demerphq <demerphq@​gmail.com> wrote​:

On 3 May 2017 at 09​:25, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 3 May 2017 at 07​:56, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 22​:49, Steve Hay <steve.m.hay@​googlemail.com> wrote​:
....
You're right - there is no mention of it in the docs (even in blead),
which amazes me. We've been using this process for some time now, and
documenting it after some trial period has evidently slipped through
the cracks. Sorry about that. I will try to fix that, though perhaps
it should wait until we've made any changes in light of your proposals
below.

* Ok, cool.

Also this maint-votes branch is pretty clunky. Why don't we just put a
file in blead? Then when you commit your patch you can do a follow up
patch to the file. We can exclude the file from the build process when
we do a release.

I think this may have been discussed at the time the voting files were
introduced, and there were some objections to having a file in blead
because the votes being cast would then cause noise on the blead
commit history.

* I see. Hrm. Thinking about it more if there was a tool that managed
it then it doesnt matter where it lives, and having the separate
branch is at least "fair" in that it is as easy, or hard to use,
regardless what your workflow is.

* I guess I have to write a tool. :-)

Anyway, if there is a file I can edit for these things then obviously
some of what I complained about has been addressed already.

Also, patches are supposed to get three votes before being backported
-- something else that I didn't see happen on this occasion. Again,
the voting file takes care of that perfectly simply.

I was under the impression that there were, at least for two of the
three patches. For the latter I interpreted the approval for the other
a bit loosely and included it too, as they were all part of the same
meta-bug, if not specific bug-report.

Sorry if I missed that. This is why having a file *somewhere* to keep
track of the votes is definitely a good idea (IMO).

* Yes. The issues of votes requires somewhere to store and track them.

It's not obvious to me that this process needs to change since it
doesn't suffer from the "blockage" issue that blead has during a long
code freeze​: There is no active development being done on maint
branches anyway (virtually by definition).

No. I withdraw that. Albeit clunkier than I would like there is a
procedure to fire-and-forget, so the problem I complained about is
solved and I just didn't know it fully. I apologise for the noise, and
for not getting with the program quicker.

On the other hand, I then think we should put push blocks on these
branches and lock them down to the person who is doing the branch
management. I was just trying to "take care of business" without
realizing I was breaking the rules. IMO technical provisions to
prevent that kind of thing are a good idea.

I think I would welcome the idea of push blocks (on the proposed blead
release branch too) during code freeze because that's when it becomes
most important to stop accidental commits, but I'm not sure they'd be
necessary the rest of the time. Other committers have pushed to maint
branches in the past. As long as it's well ahead of a release and
follows the voting rules then I have no problem with that (it saves me
a bit of work!). Would it be easy for the branch manager to add/remove
these push blocks, or is it a more complicated thing to do that (e.g.)
Dennis would have to do, and once it's done it's done?

* this is all me here

We could do that, but it would require some finagling. Perhaps better
would be to require a signed-off-by signature in the commit message.
Maybe something as simple as each commit requires at least three lines
of the form

vote​: demerphq
vote​: Steve Hay
vote​: Dave Mitchell

then the push hook can validate that, and if it is missing give a
suitable message about how to mange all of this. If we have tools to
manage the vote branch we could even automate this process.

(IIRC There is a more formal git concept of signed-off-by but i dont
know all the details or how suitable it would be for us, I will dig
and see, although if someone knows feel free to educate us.)

cheers,
Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented May 4, 2017

From @craigberry

On Wed, May 3, 2017 at 4​:21 AM, demerphq <demerphq@​gmail.com> wrote​:

[using * to mark my comments since apparently gmail html to text is
broken for quoting]
On 3 May 2017 at 10​:19, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

I think I would welcome the idea of push blocks (on the proposed blead
release branch too) during code freeze because that's when it becomes
most important to stop accidental commits, but I'm not sure they'd be
necessary the rest of the time. Other committers have pushed to maint
branches in the past. As long as it's well ahead of a release and
follows the voting rules then I have no problem with that (it saves me
a bit of work!). Would it be easy for the branch manager to add/remove
these push blocks, or is it a more complicated thing to do that (e.g.)
Dennis would have to do, and once it's done it's done?

* this is all me here

We could do that, but it would require some finagling. Perhaps better
would be to require a signed-off-by signature in the commit message.
Maybe something as simple as each commit requires at least three lines
of the form

vote​: demerphq
vote​: Steve Hay
vote​: Dave Mitchell

then the push hook can validate that, and if it is missing give a
suitable message about how to mange all of this. If we have tools to
manage the vote branch we could even automate this process.

I think what's missing in this discussion is the degree to which the
base.pm business has completely gummed up the works of the maint
releases and that's really what got in your way. Not saying that it
could be otherwise, but it has meant a very long code freeze in maint
which as far as I can remember has never happened before. In any
normal year I don't think there would have been a problem applying
what you did to maint. If it were missing a vote we could have
scrounged another vote more easily than reverting. Or agreed that
neither of the other two people on the planet qualified to review your
work on the regex engine was available to vote Subject, of course, to
Steve's decision on a case-by-case basis, which would probably come
with a reminder about any parts of the maint policy that had been
overlooked..

All of which is to say, heavy-duty automation that enforces the maint
policy in a rigid way and is hard to turn on and off seems like
overkill to me. Even with the many-month-long code freeze what
happened to you is unique as far as I know, and if we let such lengthy
freezes become the norm we've got bigger problems.

@p5pRT
Copy link
Author

p5pRT commented May 5, 2017

From @demerphq

On 4 May 2017 23​:41, "Craig A. Berry" <craig.a.berry@​gmail.com> wrote​:

On Wed, May 3, 2017 at 4​:21 AM, demerphq <demerphq@​gmail.com> wrote​:

[using * to mark my comments since apparently gmail html to text is
broken for quoting]
On 3 May 2017 at 10​:19, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

I think I would welcome the idea of push blocks (on the proposed blead
release branch too) during code freeze because that's when it becomes
most important to stop accidental commits, but I'm not sure they'd be
necessary the rest of the time. Other committers have pushed to maint
branches in the past. As long as it's well ahead of a release and
follows the voting rules then I have no problem with that (it saves me
a bit of work!). Would it be easy for the branch manager to add/remove
these push blocks, or is it a more complicated thing to do that (e.g.)
Dennis would have to do, and once it's done it's done?

* this is all me here

We could do that, but it would require some finagling. Perhaps better
would be to require a signed-off-by signature in the commit message.
Maybe something as simple as each commit requires at least three lines
of the form

vote​: demerphq
vote​: Steve Hay
vote​: Dave Mitchell

then the push hook can validate that, and if it is missing give a
suitable message about how to mange all of this. If we have tools to
manage the vote branch we could even automate this process.

I think what's missing in this discussion is the degree to which the
base.pm business has completely gummed up the works of the maint
releases and that's really what got in your way. Not saying that it
could be otherwise, but it has meant a very long code freeze in maint
which as far as I can remember has never happened before. In any
normal year I don't think there would have been a problem applying
what you did to maint. If it were missing a vote we could have
scrounged another vote more easily than reverting. Or agreed that
neither of the other two people on the planet qualified to review your
work on the regex engine was available to vote Subject, of course, to
Steve's decision on a case-by-case basis, which would probably come
with a reminder about any parts of the maint policy that had been
overlooked.

Thanks for saying this.

All of which is to say, heavy-duty automation that enforces the maint
policy in a rigid way and is hard to turn on and off seems like
overkill to me. Even with the many-month-long code freeze what
happened to you is unique as far as I know, and if we let such lengthy
freezes become the norm we've got bigger problems.

When I was in high school I did a short course in a sheet metal and machine
shop. During that course I got to use all sorts of crazy tools like
machines that could fold sheet metal like it was paper, or cutting devices
that could take your finger off before you even felt the pain. It taught me
a powerful respect for technical safety measures which has stuck with me
ever since. So I personally don't see requiring vote lines in maint commit
message as a huge burden.

Having said that you have a point about not making policy from a black Swan
event.

We can ruminate on this for a bit and revisit it the next time someone
makes the same mistakes I did.

Cheers
Yves

@p5pRT
Copy link
Author

p5pRT commented Jul 20, 2017

From @steve-m-hay

On 3 May 2017 at 09​:19, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 3 May 2017 at 09​:00, demerphq <demerphq@​gmail.com> wrote​:

On 3 May 2017 at 09​:25, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 3 May 2017 at 07​:56, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 22​:49, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 2 May 2017 at 18​:38, demerphq <demerphq@​gmail.com> wrote​:

On 2 May 2017 at 17​:39, Dagfinn Ilmari Mannsåker <ilmari@​ilmari.org> wrote​:

Dave Mitchell <davem@​iabyn.com> writes​:

On Mon, May 01, 2017 at 02​:51​:16AM +0100, Zefram wrote​:

Jan Goyvaerts wrote​:

if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) {

This has been fixed in blead.

in particular, v5.25.0-36-gda7cf1c​:

fix \#128109 \- do not move RExC\_open\_parens\[0\] in reginsert

That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it,
cherry-pick cleanly to maint-5.24. Seeing as it's a crasher and
regression from 5.22, it's a candidate for backporting according to
perlpolicy.

I just pushed

78b60e1 Add tests for regex recursion
4539ae3 fix #128109 - do not move RExC_open_parens[0] in reginsert
5edefd5 fix #128085 - SIGSEGV in S_regmatch with S_study_chunk​:
Assertion "!frame" failed.

to maint-5.24 -- I took the liberty of including 5edefd5 which was
part of how I found out about #128109

I don't think this should have been done right now. maint-5.22 and
maint-5.24 are both still in code freeze pending the release of 5.22.4
and 5.24.2, which are intended only to contain the last @​INC fixes
(and possibly some other security fixes, as per the previous releases
on those branches).

There are many, many other bug fixes (crashes and otherwise) that
could be backported, but haven't been yet. They were deliberately
being left for 5.24.3, once the last of the @​INC fixes was out of the
way.

Sigh. Honestly I give up. Our release process is broken. If it weren't
for the fact that we are going to change these policies that I would
insert a long rant here about how expecting our developers to keep
track of all their patches, which branches they have and have not been
merged and which they should be merged to in some distant point in
time is not a good way manage things[1]. Just like distributing swords
in lakes in the middle of the night not a sound way to form a
government. :-)

Anyway, bulk reverted in 9ecaf4d
sorry for yet more revert turbulence.

Thanks.

cheers,
Yves
[1] So here is the *short* rant​: I know there are some on this list
that one way or another do not find these types of things a burden.
But processes should work for everybody, not just the meticulous and
careful, and they should be robust to things like unexpected
life-changes. This patch was written in a period when I was dealing
with the death of my father and a very sick mother. Making sure the
patch was backported at the right time so that it didn't violate a
code freeze was simply not a priority for me, and as our release
procedures and policies do not provide for any solution other than
"remember yourself", no-one else was going to do it either. This to me
says that the policies and procedures are broken by design.

I wasn't aware that the discussions over changes in branch management
were intended to cover maint releases too.

I had no direct thoughts on this, but when i wrote that mail I was
thinking that they kinda flow together. However some of what you say
below makes me reconsider.

Regarding remembering that patches get backported, I think the
existing process is very simple, actually​: Just add the patch to the
relevant voting file in the maint-votes branch and the maint release
manager will pick it up at the approrpiate point in time. That's the
whole point of having a maint release manager isn't it? -- so that
individual developers don't have to remember all these things
themselves. If ever a patch is non-trivial to backport then I will
seek assistance when I come to cherry-pick it, which is something that
I've done on several occasions in the past.

Ok, I think I missed some of these details being announced. However I
see no mention of a maint-votes branch in the docs, at least not the
docs for 5.24.

You're right - there is no mention of it in the docs (even in blead),
which amazes me. We've been using this process for some time now, and
documenting it after some trial period has evidently slipped through
the cracks. Sorry about that. I will try to fix that, though perhaps
it should wait until we've made any changes in light of your proposals
below.

I've now documented the current process in commit
29f8566.

Hopefully this will avert similar confusion in the future. If not then
we may need to reconsider things.

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