Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.98k stars 559 forks source link

Debugger command regression: Now requires more spaces #13342

Closed p5pRT closed 7 years ago

p5pRT commented 11 years ago

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

Searchable as RT120174$

p5pRT commented 11 years ago

From @Smylers

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


In the Perl debugger commands written like the following used to work\, but now throw errors​:

  p@​ARGV   x@​ARGV   x\@​ARGV   x\%INC

They still work if you type a space between the command letter and the punctuation character which starts its argument\, but they used to work without the space too.

It only seems to be the single-letter debugger commands that are affected. print@​ARGV and so on still works without spaces.

The error message in blead is​:

  DB\<72> x\@​ARGV   Backslash found where operator expected at (eval 8)[lib/perl5db.pl​:732] line 2\, near "x\"   at (eval 8)[lib/perl5db.pl​:732] line 2.   eval 'no strict; ($@​\, $!\, $^E\, $\,\, $/\, $\\\, $^W) = @​DB​::saved;package main; $^D = $^D | $DB​::db_stop;   x\\@​ARGV;   ' called at lib/perl5db.pl line 732   DB​::eval called at lib/perl5db.pl line 3091   DB​::DB called at -e line 1   syntax error at (eval 8)[lib/perl5db.pl​:732] line 2\, near "x\"

This is it working in v5.14.3​:

  DB\<72> x\@​ARGV   0 ARRAY(0x2267d18)   empty array

I haven't tried to narrow it down any further\, mainly cos I don't know how to come up with a non-interactive way of demonstrating the change.



Flags​:   category=core   severity=low


Site configuration information for perl 5.19.5​:

Configured by smylers at Wed Oct 9 17​:34​:57 BST 2013.

Summary of my perl5 (revision 5 version 19 subversion 5) configuration​:   Commit id​: 01582e5ce2e8b00ce08a55eb4a588e811e479912   Platform​:   osname=linux\, osvers=3.8.0-32-generic\, archname=x86_64-linux   uname='linux fozzie 3.8.0-32-generic #47-ubuntu smp tue oct 1 22​:35​:23 utc 2013 x86_64 x86_64 x86_64 gnulinux '   config_args='-des -Dusedevel'   hint=recommended\, useposix=true\, d_sigaction=define   useithreads=undef\, usemultiplicity=undef   useperlio=define\, d_sfio=undef\, uselargefiles=define\, usesocks=undef   use64bitint=define\, use64bitall=define\, uselongdouble=undef   usemymalloc=n\, bincompat5005=undef   Compiler​:   cc='cc'\, ccflags ='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\,   optimize='-O2'\,   cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'   ccversion=''\, gccversion='4.7.3'\, gccosandvers=''   intsize=4\, longsize=8\, ptrsize=8\, doublesize=8\, byteorder=12345678   d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=16   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 /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib   libs=-lnsl -ldl -lm -lcrypt -lutil -lc   perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc   libc=\, so=so\, useshrplib=false\, libperl=libperl.a   gnulibc_version='2.17'   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.19.5​:   lib   /home/smylers/lib/perl5/site_perl   /home/smylers/lib/perl5   /usr/local/lib/perl5/site_perl/5.19.5/x86_64-linux   /usr/local/lib/perl5/site_perl/5.19.5   /usr/local/lib/perl5/5.19.5/x86_64-linux   /usr/local/lib/perl5/5.19.5   .


Environment for perl 5.19.5​:   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   PERL5LIB=/home/smylers/lib/perl5/site_perl​:/home/smylers/lib/perl5   PERL_BADLANG (unset)   PERL_CPANM_OPT=--sudo --prompt   SHELL=/bin/bash

p5pRT commented 7 years ago

From @jkeenan

On Thu\, 10 Oct 2013 11​:14​:24 GMT\, smylers@​stripey.com wrote​:

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

-----------------------------------------------------------------

In the Perl debugger commands written like the following used to work\, but now throw errors​:

p@​ARGV x@​ARGV x\@​ARGV x\%INC

They still work if you type a space between the command letter and the punctuation character which starts its argument\, but they used to work without the space too.

It only seems to be the single-letter debugger commands that are affected. print@​ARGV and so on still works without spaces.

The error message in blead is​:

DB\<72> x\@​ARGV Backslash found where operator expected at (eval 8)[lib/perl5db.pl​:732] line 2\, near "x\" at (eval 8)[lib/perl5db.pl​:732] line 2. eval 'no strict; ($@​\, $!\, $^E\, $\,\, $/\, $\\\, $^W) = @​DB​::saved;package main; $^D = $^D | $DB​::db_stop; x\\@​ARGV; ' called at lib/perl5db.pl line 732 DB​::eval called at lib/perl5db.pl line 3091 DB​::DB called at -e line 1 syntax error at (eval 8)[lib/perl5db.pl​:732] line 2\, near "x\"

This is it working in v5.14.3​:

DB\<72> x\@​ARGV 0 ARRAY(0x2267d18) empty array

I haven't tried to narrow it down any further\, mainly cos I don't know how to come up with a non-interactive way of demonstrating the change.

I have confirmed the regression. It is still present in blead (5.27.3).

However\, we should first ask​: Is this a bug? That is\, was the fact that we once could use the 'p' and 'x' commands in the debugger without a subsequent wordspace intentional or accidental?

Note​: I'm not saying this should not be fixed. I simply think the question should be faced.

Use of perlbrew to get various versions of perl shows roughly when the regression occurred​:

#####   good perlbrew use perl-5.16.3 Loading DB routines from perl5db.pl version 1.37   bad perlbrew use perl-5.18.4 Loading DB routines from perl5db.pl version 1.39_11 #####

Which means that the problem occurred roughly between these two commits​:

##### commit 68cd360812f9eaa2d34c45c501e2fef87c44ccde Author​: Jesse Vincent \jesse@&#8203;bestpractical\.com Date​: Tue Apr 24 19​:02​:34 2012 -0400 -$VERSION = '1.36'; +$VERSION = '1.37';

commit 1799399c45baa7e5af45db33567d2a2de629d5e0 Author​: Ricardo Signes \rjbs@&#8203;cpan\.org AuthorDate​: Fri Jun 7 11​:51​:45 2013 -0400 Commit​: Ricardo Signes \rjbs@&#8203;cpan\.org CommitDate​: Fri Jun 7 11​:51​:45 2013 -0400

  version bumps and perldelta for debugger depth -$VERSION = '1.39_10'; +$VERSION = '1.40'; #####

There was a lot of work being done on the debugger during this period. The diff is large enough that I'm attaching it gzipped first.

I don't know how to bisect a debugger problem. But I was able to create a branch based on perl-5.16.3 (jkeenan/120174-starting-from-5.16.3) in which I wrote some tests for the problem with passed. Those same tests\, when added to a branch based on blead (5.27.3) (jkeenan/120174-starting-from-5.27.3)\, fail. Hence\, the tests -- which\, I concede\, are rough\, as I'm not familiar with the tests in lib/perl5db.t -- can be used as regression tests once the problem is corrected (assuming we want to correct it).

Thank you very much. -- James E Keenan (jkeenan@​cpan.org)

p5pRT commented 7 years ago

From @jkeenan

120174-0001-Add-tests-for-p-and-x-commands-without-subsequent-wh.patch ```diff From 9cbf7940aed97cb73313c28dc2ba2fd85cc19458 Mon Sep 17 00:00:00 2001 From: James E Keenan Date: Sat, 2 Sep 2017 22:28:20 -0400 Subject: [PATCH] Add tests for 'p' and 'x' commands without subsequent whitespace. Tests pass on perl-5.16.3 but should fail (until source code is corrected) on subsequent versions. For: RT #120174 --- lib/perl5db.t | 86 ++++++++++++++++++++++++++++++++++++++++++++++++- lib/perl5db/t/rt-120174 | 4 +++ 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 lib/perl5db/t/rt-120174 diff --git a/lib/perl5db.t b/lib/perl5db.t index a2dccc6..3d432ad 100644 --- a/lib/perl5db.t +++ b/lib/perl5db.t @@ -31,7 +31,7 @@ BEGIN { $ENV{PERL_RL} = 'Perl'; # Suppress system Term::ReadLine::Gnu } -plan(123); +plan(127); my $rc_filename = '.perldb'; @@ -2817,6 +2817,90 @@ SKIP: ); } +{ + # perl 5 RT #120174 - 'p' command + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'b 2', + 'c', + 'p@abc', + 'q', + ], + prog => '../lib/perl5db/t/rt-120174', + } + ); + + $wrapper->contents_like( + qr/1234/, + q/RT 120174: p command can be invoked without space after 'p'/, + ); +} + +{ + # perl 5 RT #120174 - 'x' command on array + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'b 2', + 'c', + 'x@abc', + 'q', + ], + prog => '../lib/perl5db/t/rt-120174', + } + ); + + $wrapper->contents_like( + qr/0\s+1\n1\s+2\n2\s+3\n3\s+4/ms, + q/RT 120174: x command can be invoked without space after 'x' before array/, + ); +} + +{ + # perl 5 RT #120174 - 'x' command on array ref + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'b 2', + 'c', + 'x\@abc', + 'q', + ], + prog => '../lib/perl5db/t/rt-120174', + } + ); + + $wrapper->contents_like( + qr/\s+0\s+1\n\s+1\s+2\n\s+2\s+3\n\s+3\s+4/ms, + q/RT 120174: x command can be invoked without space after 'x' before array ref/, + ); +} + +{ + # perl 5 RT #120174 - 'x' command on hash ref + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'b 4', + 'c', + 'x\%xyz', + 'q', + ], + prog => '../lib/perl5db/t/rt-120174', + } + ); + + $wrapper->contents_like( + qr/\s+'alpha'\s+=>\s+'beta'\n\s+'gamma'\s+=>\s+'delta'/ms, + q/RT 120174: x command can be invoked without space after 'x' before hash ref/, + ); +} + END { 1 while unlink ($rc_filename, $out_fn); } diff --git a/lib/perl5db/t/rt-120174 b/lib/perl5db/t/rt-120174 new file mode 100644 index 0000000..c79c851 --- /dev/null +++ b/lib/perl5db/t/rt-120174 @@ -0,0 +1,4 @@ +@abc = (1..4); +print "hello world\n"; +%xyz = ( 'alpha' => 'beta', 'gamma' => 'delta' ); +print "goodbye world\n"; -- 2.7.4 ```
p5pRT commented 7 years ago

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

p5pRT commented 7 years ago

From @jkeenan

On Sun\, 03 Sep 2017 03​:02​:20 GMT\, jkeenan wrote​:

Add diff.

-- James E Keenan (jkeenan@​cpan.org)

p5pRT commented 7 years ago

From @jkeenan

120174.68cd360812.1799399c45.perl5db.pl.diff.gz

p5pRT commented 7 years ago

From [Unknown Contact. See original ticket]

On Sun\, 03 Sep 2017 03​:02​:20 GMT\, jkeenan wrote​:

Add diff.

-- James E Keenan (jkeenan@​cpan.org)

p5pRT commented 7 years ago

From @jkeenan

On Sun\, 03 Sep 2017 03​:02​:20 GMT\, jkeenan wrote​:

On Thu\, 10 Oct 2013 11​:14​:24 GMT\, smylers@​stripey.com wrote​:

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

-----------------------------------------------------------------

In the Perl debugger commands written like the following used to work\, but now throw errors​:

p@​ARGV x@​ARGV x\@​ARGV x\%INC

They still work if you type a space between the command letter and the punctuation character which starts its argument\, but they used to work without the space too.

It only seems to be the single-letter debugger commands that are affected. print@​ARGV and so on still works without spaces.

The error message in blead is​:

DB\<72> x\@​ARGV Backslash found where operator expected at (eval 8)[lib/perl5db.pl​:732] line 2\, near "x\" at (eval 8)[lib/perl5db.pl​:732] line 2. eval 'no strict; ($@​\, $!\, $^E\, $\,\, $/\, $\\\, $^W) = @​DB​::saved;package main; $^D = $^D | $DB​::db_stop; x\\@​ARGV; ' called at lib/perl5db.pl line 732 DB​::eval called at lib/perl5db.pl line 3091 DB​::DB called at -e line 1 syntax error at (eval 8)[lib/perl5db.pl​:732] line 2\, near "x\"

This is it working in v5.14.3​:

DB\<72> x\@​ARGV 0 ARRAY(0x2267d18) empty array

I haven't tried to narrow it down any further\, mainly cos I don't know how to come up with a non-interactive way of demonstrating the change.

I have confirmed the regression. It is still present in blead (5.27.3).

However\, we should first ask​: Is this a bug? That is\, was the fact that we once could use the 'p' and 'x' commands in the debugger without a subsequent wordspace intentional or accidental?

Note​: I'm not saying this should not be fixed. I simply think the question should be faced.

Use of perlbrew to get various versions of perl shows roughly when the regression occurred​:

##### good perlbrew use perl-5.16.3 Loading DB routines from perl5db.pl version 1.37 bad perlbrew use perl-5.18.4 Loading DB routines from perl5db.pl version 1.39_11 #####

Which means that the problem occurred roughly between these two commits​:

##### commit 68cd360812f9eaa2d34c45c501e2fef87c44ccde Author​: Jesse Vincent \jesse@&#8203;bestpractical\.com Date​: Tue Apr 24 19​:02​:34 2012 -0400 -$VERSION = '1.36'; +$VERSION = '1.37';

commit 1799399c45baa7e5af45db33567d2a2de629d5e0 Author​: Ricardo Signes \rjbs@&#8203;cpan\.org AuthorDate​: Fri Jun 7 11​:51​:45 2013 -0400 Commit​: Ricardo Signes \rjbs@&#8203;cpan\.org CommitDate​: Fri Jun 7 11​:51​:45 2013 -0400

version bumps and perldelta for debugger depth -$VERSION = '1.39_10'; +$VERSION = '1.40'; #####

There was a lot of work being done on the debugger during this period. The diff is large enough that I'm attaching it gzipped first.

I don't know how to bisect a debugger problem. But I was able to create a branch based on perl-5.16.3 (jkeenan/120174-starting-from- 5.16.3) in which I wrote some tests for the problem with passed. Those same tests\, when added to a branch based on blead (5.27.3) (jkeenan/120174-starting-from-5.27.3)\, fail. Hence\, the tests -- which\, I concede\, are rough\, as I'm not familiar with the tests in lib/perl5db.t -- can be used as regression tests once the problem is corrected (assuming we want to correct it).

I have figured out how to bisect a debugger problem.

The problems with the 'x' command first appeared in this commit​:

##### $ git show --format=fuller d478d7a00517925a72f7077951fbed1283fb6a07 |cat commit d478d7a00517925a72f7077951fbed1283fb6a07 Author​: Shlomi Fish \shlomif@&#8203;shlomifish\.org AuthorDate​: Sun Oct 14 12​:56​:53 2012 +0200 Commit​: Ricardo Signes \rjbs@&#8203;cpan\.org CommitDate​: Mon Nov 12 09​:18​:40 2012 -0500

  Add more commands to the lookup table.

Inline Patch ```diff diff --git a/lib/perl5db.pl b/lib/perl5db.pl index c9844fd..ed34703 100644 --- a/lib/perl5db.pl +++ b/lib/perl5db.pl @@ -2423,7 +2423,15 @@ sub _DB__at_end_of_every_command { # 's' is subroutine. my %cmd_lookup = ( - 'q' => { t => 'm', v => '_handle_q_command' }, + '.' => { t => 's', v => \&_DB__handle_dot_command, }, + 'f' => { t => 's', v => \&_DB__handle_f_command, }, + 'm' => { t => 's', v => \&_DB__handle_m_command, }, + 'q' => { t => 'm', v => '_handle_q_command', }, + 'S' => { t => 'm', v => '_handle_S_command', }, + 't' => { t => 'm', v => '_handle_t_command', }, + 'x' => { t => 'm', v => '_handle_x_command', }, + (map { $_ => { t => 'm', v => '_handle_V_command_and_X_command', }, } + ('X', 'V')), ); sub DB { @@ -2763,16 +2771,12 @@ If level is specified, set C<$trace_to_depth>. =cut - $obj->_handle_t_command; - =head4 C - list subroutines matching/not matching a pattern Walks through C<%sub>, checking to see whether or not to print the name. =cut - $obj->_handle_S_command; - =head4 C - list variables in current package Since the C command actually processes this, just change this to the @@ -2784,8 +2788,6 @@ Uses C to dump out the current values for selected variables. =cut - $obj->_handle_V_command_and_X_command; - =head4 C - evaluate and print an expression Hands the expression off to C, setting it up to print the value @@ -2793,22 +2795,16 @@ via C instead of just printing it directly. =cut - $obj->_handle_x_command; - =head4 C - print methods Just uses C to determine what methods are available. =cut - _DB__handle_m_command($obj); - =head4 C - switch files =cut - _DB__handle_f_command($obj); - =head4 C<.> - return to last-executed line. We set C<$incr> to -1 to indicate that the debugger shouldn't move ahead, @@ -2816,8 +2812,6 @@ and then we look up the line in the magical C<%dbline> hash. =cut - _DB__handle_dot_command($obj); - =head4 C<-> - back one window We change C<$start> to be one window back; if we go back past the first line, ```

The changes to the 'p' command first appeared at this commit​:

##### $ git show --format=fuller 8f144dfcb6b7d7a28faa9e82a687338b141c272f |cat commit 8f144dfcb6b7d7a28faa9e82a687338b141c272f Author​: Shlomi Fish \shlomif@&#8203;shlomifish\.org AuthorDate​: Sun Oct 14 13​:22​:45 2012 +0200 Commit​: Ricardo Signes \rjbs@&#8203;cpan\.org CommitDate​: Mon Nov 12 09​:18​:41 2012 -0500

  Low hanging fruit is now in %cmd_lookup.

Inline Patch ```diff diff --git a/lib/perl5db.pl b/lib/perl5db.pl index d79be51..56b8d65 100644 --- a/lib/perl5db.pl +++ b/lib/perl5db.pl @@ -2425,20 +2425,32 @@ my %cmd_lookup = ( '-' => { t => 'm', v => '_handle_dash_command', }, '.' => { t => 's', v => \&_DB__handle_dot_command, }, + '=' => { t => 'm', v => '_handle_equal_sign_command', }, + 'H' => { t => 'm', v => '_handle_H_command', }, 'S' => { t => 'm', v => '_handle_S_command', }, 'T' => { t => 'm', v => '_handle_T_command', }, + 'W' => { t => 'm', v => '_handle_W_command', }, 'c' => { t => 's', v => \&_DB__handle_c_command, }, 'f' => { t => 's', v => \&_DB__handle_f_command, }, 'm' => { t => 's', v => \&_DB__handle_m_command, }, 'n' => { t => 'm', v => '_handle_n_command', }, + 'p' => { t => 'm', v => '_handle_p_command', }, 'q' => { t => 'm', v => '_handle_q_command', }, 'r' => { t => 'm', v => '_handle_r_command', }, 's' => { t => 'm', v => '_handle_s_command', }, + 'save' => { t => 'm', v => '_handle_save_command', }, + 'source' => { t => 'm', v => '_handle_source_command', }, 't' => { t => 'm', v => '_handle_t_command', }, + 'w' => { t => 'm', v => '_handle_w_command', }, 'x' => { t => 'm', v => '_handle_x_command', }, 'y' => { t => 's', v => \&_DB__handle_y_command, }, (map { $_ => { t => 'm', v => '_handle_V_command_and_X_command', }, } ('X', 'V')), + (map { $_ => { t => 'm', v => '_handle_enable_disable_commands', }, } + qw(enable disable)), + (map { $_ => + { t => 's', v => \&_DB__handle_restart_and_rerun_commands, }, + } qw(R rerun)), ); sub DB { @@ -2888,18 +2900,10 @@ Just calls C. Just calls C. -=cut - - $obj->_handle_w_command; - =head4 C - watch-expression processing. Just calls C. -=cut - - $obj->_handle_W_command; - =head4 C - search forward for a string in the source We take the argument and treat it as a pattern. If it turns out to be a @@ -2963,10 +2967,6 @@ C to avoid problems with C and C. Prints the contents of C<@hist> (if any). -=cut - - $obj->_handle_H_command; - =head4 C - look up documentation Just calls C to print the appropriate document. @@ -2980,36 +2980,19 @@ Just calls C to print the appropriate document. Builds a C expression in the C<$cmd>; this will get executed at the bottom of the loop. -=cut - - $obj->_handle_p_command; - =head4 C<=> - define command alias Manipulates C<%alias> to add or list command aliases. -=cut - - # = - set up a command alias. - $obj->_handle_equal_sign_command; - =head4 C - read commands from a file. Opens a lexical filehandle and stacks it on C<@cmdfhs>; C will pick it up. -=cut - - $obj->_handle_source_command; - =head4 C C - enable or disable breakpoints This enables or disables breakpoints. -=cut - - $obj->_handle_enable_disable_commands; - =head4 C - send current history to a file Takes the complete history, (not the shrunken version you see with C), @@ -3017,11 +3000,6 @@ and saves it to the given filename, so it can be replayed using C. Note that all C<^(save|source)>'s are commented out with a view to minimise recursion. -=cut - - # save source - write commands to a file for later use - $obj->_handle_save_command; - =head4 C - restart Restart the debugger session. @@ -3030,12 +3008,6 @@ Restart the debugger session. Return to any given position in the B-history list -=cut - - # R - restart execution. - # rerun - controlled restart execution. - _DB__handle_restart_and_rerun_commands($obj); - =head4 C<|, ||> - pipe output through the pager. For C<|>, we save C (the debugger's output filehandle) and C ```

Thank you very much.

-- James E Keenan (jkeenan@​cpan.org)

p5pRT commented 7 years ago

From @Smylers

On Sat\, 02 Sep 2017 20​:02​:20 -0700\, jkeenan wrote​:

On Thu\, 10 Oct 2013 11​:14​:24 GMT\, smylers@​stripey.com wrote​:

In the Perl debugger commands written like the following used to work\, but now throw errors​:

p@​ARGV

I have confirmed the regression. It is still present in blead (5.27.3).

Thanks\, James. And for the automated tests and finding the specific commits that caused the regression.

However\, we should first ask​: Is this a bug?

That's a fair question. I think there are three arguments in favour​:

  1 perl itself doesn't require a space between keywords and variable   names\, such as in​:

  $ perl -lE "print@​ARGV" one two three   onetwothree

  So it's an extra hurdle for users to have to remember that the   debugger is different.

  2 Commands typed at the debugger prompt are by definition   single-use-only throwaway instructions. As such\, saving typing   time generally matters more than readability.

That is\, was the fact that we once could use the 'p' and 'x' commands in the debugger without a subsequent wordspace intentional or accidental?

  3 Perl has a history of retaining backwards compatibility even of   ‘accidental’ features\, except where these get in the way of   something else (and that doesn't seem to be the case here).

  So even if this weren't an intentional feature originally\, it was   a feature and used by people.

  (However\, after half a decade of this being broken\, people have   presumably got used to living without this by now.)

I see from the commits you tracked this to that a bunch of single-letter debugger commands were added to a lookup table\, not just p and x. It may be that the regression also affects other commands.

Smylers

p5pRT commented 7 years ago

From @Leont

On Sun\, Sep 3\, 2017 at 5​:02 AM\, James E Keenan via RT \< perlbug-followup@​perl.org> wrote​:

I have confirmed the regression. It is still present in blead (5.27.3).

However\, we should first ask​: Is this a bug? That is\, was the fact that we once could use the 'p' and 'x' commands in the debugger without a subsequent wordspace intentional or accidental?

Note​: I'm not saying this should not be fixed. I simply think the question should be faced.

I can think of no reason not call this regression a bug.

Use of perlbrew to get various versions of perl shows roughly when the regression occurred​:

##### good perlbrew use perl-5.16.3 Loading DB routines from perl5db.pl version 1.37 bad perlbrew use perl-5.18.4 Loading DB routines from perl5db.pl version 1.39_11 #####

Which means that the problem occurred roughly between these two commits​:

##### commit 68cd360812f9eaa2d34c45c501e2fef87c44ccde Author​: Jesse Vincent \jesse@&#8203;bestpractical\.com Date​: Tue Apr 24 19​:02​:34 2012 -0400 -$VERSION = '1.36'; +$VERSION = '1.37';

commit 1799399c45baa7e5af45db33567d2a2de629d5e0 Author​: Ricardo Signes \rjbs@&#8203;cpan\.org AuthorDate​: Fri Jun 7 11​:51​:45 2013 -0400 Commit​: Ricardo Signes \rjbs@&#8203;cpan\.org CommitDate​: Fri Jun 7 11​:51​:45 2013 -0400

version bumps and perldelta for debugger depth

-$VERSION = '1.39_10'; +$VERSION = '1.40'; #####

There was a lot of work being done on the debugger during this period. The diff is large enough that I'm attaching it gzipped first.

This period coincides with a grant on the debugger. It's far from the only regression from that period -_-.

Leon

p5pRT commented 7 years ago

From @shlomif

Hi all!

On Mon\, 04 Sep 2017 01​:51​:18 -0700 "Smylers via RT" \perlbug\-followup@&#8203;perl\.org wrote​:

On Sat\, 02 Sep 2017 20​:02​:20 -0700\, jkeenan wrote​:

On Thu\, 10 Oct 2013 11​:14​:24 GMT\, smylers@​stripey.com wrote​:

In the Perl debugger commands written like the following used to work\, but now throw errors​:

p@​ARGV

I have confirmed the regression. It is still present in blead (5.27.3).

Thanks\, James. And for the automated tests and finding the specific commits that caused the regression.

Since it was my work on refactoring and adding tests to the debugger that broke this (and which I got paid for)\, I am willing to take it on myself to work on a fix. But I'd like to wait for the final verdict of whether this should be fixed? Is it ultimately the pumpking's decision?

My personal inclination is to keep the regression because​: 1. I feel it was an accidental misfeature. 2. It will mean more work for me. 3. It may complicate the code of perl5db.pl and introduce ugliness\, workarounds and edge cases. But that is just my opinion and I feel I don't have the final say on the matter. If the powers at be decide otherwise\, then I will accept it and work on a fix.

Regards\,

  Shlomi Fish

[snipped]

--


Shlomi Fish http​://www.shlomifish.org/ My Favourite FOSS - http​://www.shlomifish.org/open-source/favourite/

One thing I could never understand is why in Microsoft Word\, it often happens that I press enter… and the font changes.

p5pRT commented 7 years ago

From @jkeenan

On Tue\, 05 Sep 2017 19​:51​:00 GMT\, shlomif@​shlomifish.org wrote​:

Hi all!

On Mon\, 04 Sep 2017 01​:51​:18 -0700 "Smylers via RT" \perlbug\-followup@&#8203;perl\.org wrote​:

On Sat\, 02 Sep 2017 20​:02​:20 -0700\, jkeenan wrote​:

On Thu\, 10 Oct 2013 11​:14​:24 GMT\, smylers@​stripey.com wrote​:

In the Perl debugger commands written like the following used to work\, but now throw errors​:

p@​ARGV

I have confirmed the regression. It is still present in blead (5.27.3).

Thanks\, James. And for the automated tests and finding the specific commits that caused the regression.

Since it was my work on refactoring and adding tests to the debugger that broke this (and which I got paid for)\, I am willing to take it on myself to work on a fix. But I'd like to wait for the final verdict of whether this should be fixed? Is it ultimately the pumpking's decision?

Yes\, ultimately it is the pumpking's decision. In this case\, I hope he will perform the most difficult but most necessary function in the pumpking's role. I hope he will say\, "No."

My personal inclination is to keep the regression because​: 1. I feel it was an accidental misfeature.

I agree. Type 'perldoc perldebug' in perl-5.16.3 -- the last production version where you could type a 'p' or an 'x' not followed by a wordspace -- and you will find that the wordspace-less version of the command is neither documented nor tested. You are correct is stating that it was an accidental misfeature.

2. It will mean more work for me.

I somewhat disagree. The former pumpking was the person who actually committed your work to blead\, and all the rest of us committers had the opportunity to review your work in detail. So it's certainly not your fault alone.

Since it was an accidental misfeature\, restoring it to working condition should be the responsibility of those who want to restore it. And it will be *a lot more* work for them!

3. It may complicate the code of perl5db.pl and introduce ugliness\, workarounds and edge cases.

Will it ever! The debugger is an ugly beast\, but it was uglier still before your refactoring project.

We have more than a few open RT tickets concerning the debugger. I would rather have someone focus on them than on restoring this accidental misfeature.

But that is just my opinion and I feel I don't have the final say on the matter. If the powers at be decide otherwise\, then I will accept it and work on a fix.

Regards\,

Shlomi Fish

So I hope the pumpking says\, "No."

Thank you very much.

-- James E Keenan (jkeenan@​cpan.org)

p5pRT commented 7 years ago

From @shlomif

Hi James and all\,

On Tue\, 05 Sep 2017 13​:59​:21 -0700 "James E Keenan via RT" \perlbug\-followup@&#8203;perl\.org wrote​:

On Tue\, 05 Sep 2017 19​:51​:00 GMT\, shlomif@​shlomifish.org wrote​:

Hi all!

On Mon\, 04 Sep 2017 01​:51​:18 -0700 "Smylers via RT" \perlbug\-followup@&#8203;perl\.org wrote​:

On Sat\, 02 Sep 2017 20​:02​:20 -0700\, jkeenan wrote​:

On Thu\, 10 Oct 2013 11​:14​:24 GMT\, smylers@​stripey.com wrote​:

In the Perl debugger commands written like the following used to work\, but now throw errors​:

p@​ARGV

I have confirmed the regression. It is still present in blead (5.27.3).

Thanks\, James. And for the automated tests and finding the specific commits that caused the regression.

Since it was my work on refactoring and adding tests to the debugger that broke this (and which I got paid for)\, I am willing to take it on myself to work on a fix. But I'd like to wait for the final verdict of whether this should be fixed? Is it ultimately the pumpking's decision?

Yes\, ultimately it is the pumpking's decision.

I see.

In this case\, I hope he will perform the most difficult but most necessary function in the pumpking's role. I hope he will say\, "No."

Yes. I recall hearing that Linus Torvalds also once said that that was his most important function as the chief maintainer of the Linux kernel.

My personal inclination is to keep the regression because​: 1. I feel it was an accidental misfeature.

I agree. Type 'perldoc perldebug' in perl-5.16.3 -- the last production version where you could type a 'p' or an 'x' not followed by a wordspace -- and you will find that the wordspace-less version of the command is neither documented nor tested. You are correct is stating that it was an accidental misfeature.

Yes.

2. It will mean more work for me.

I somewhat disagree. The former pumpking was the person who actually committed your work to blead\, and all the rest of us committers had the opportunity to review your work in detail. So it's certainly not your fault alone.

Right. Nevertheless\, I did it as part of a paid grant\, and so I'm also responsible.

Since it was an accidental misfeature\, restoring it to working condition should be the responsibility of those who want to restore it. And it will be *a lot more* work for them!

Yes. I still volunteer to provide a fix patch if the pumpking thinks it should be fixed\, but naturally those who want it restored may need to test it.

3. It may complicate the code of perl5db.pl and introduce ugliness\, workarounds and edge cases.

Will it ever! The debugger is an ugly beast\, but it was uglier still before your refactoring project.

Heh. :-)

We have more than a few open RT tickets concerning the debugger. I would rather have someone focus on them than on restoring this accidental misfeature.

I agree.

But that is just my opinion and I feel I don't have the final say on the matter. If the powers at be decide otherwise\, then I will accept it and work on a fix.

Regards\,

Shlomi Fish

So I hope the pumpking says\, "No."

I hope so to\, but it is up to him.

Thank you very much.

--


Shlomi Fish http​://www.shlomifish.org/ Beginners Site for the Vim text editor - http​://vim.begin-site.org/

\ sub id { my $self = shift; $json_parser_for{ $self }   ->decode($json_for{ $self })->{id} } # Inside‐out JSON‐notated objects

p5pRT commented 7 years ago

From @arc

James E Keenan via RT \perlbug\-followup@&#8203;perl\.org wrote​:

Yes\, ultimately it is the pumpking's decision. In this case\, I hope he will perform the most difficult but most necessary function in the pumpking's role. I hope he will say\, "No."

My personal inclination is to keep the regression because​: 1. I feel it was an accidental misfeature.

I agree. Type 'perldoc perldebug' in perl-5.16.3 -- the last production version where you could type a 'p' or an 'x' not followed by a wordspace -- and you will find that the wordspace-less version of the command is neither documented nor tested. You are correct is stating that it was an accidental misfeature.

I disagree. This change in behaviour has caused sufficient annoyance that at least one user has felt motivated to put the effort into reporting it\, and to engaging with subsequent discussion about the issue. Furthermore\, the previous behaviour dates back to at least Perl 4.000\, as far as I can tell; we've certainly been implicitly teaching our users for a long time that they can rely on it. "We probably wouldn't intentionally do it like that today" doesn't seem like a strong argument in favour of retaining this regression.

Since it was an accidental misfeature\, restoring it to working condition should be the responsibility of those who want to restore it.

That seems harsh. Wanting a feature (accidental or not\, mis- or not) to remain shouldn't be dependent on knowing how to implement it.

And it will be *a lot more* work for [those who want to restore it]!

3. It may complicate the code of perl5db.pl and introduce ugliness\, workarounds and edge cases.

Will it ever! The debugger is an ugly beast\, but it was uglier still before your refactoring project.

On the contrary\, fixing this regression involves making a trivial change to exactly one regex — it's much easier to fix it than to write a test for the behaviour (in either direction)! (And I say this as someone who had literally never looked at the internals of the debugger before now.)

Accordingly\, I've attached a patch that fixes this regression\, including a test for the original behaviour; and I will apply it in a few days unless overruled by the pumpking.

-- Aaron Crane

p5pRT commented 7 years ago

From @arc

0001-RT-120174-regression-in-single-letter-debugger-comma.patch ```diff From a763defe67e9e682795335b7f47260401b1069a2 Mon Sep 17 00:00:00 2001 From: Aaron Crane Date: Thu, 7 Sep 2017 12:56:07 +0100 Subject: [PATCH] RT #120174: regression in single-letter debugger commands In the 5.17.x series, the debugger started requiring a space after single-letter commands. Fix this regression, and add a test for the original behaviour. --- lib/perl5db.pl | 5 +++-- lib/perl5db.t | 21 ++++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/lib/perl5db.pl b/lib/perl5db.pl index 265b4441f3..f9275aee00 100644 --- a/lib/perl5db.pl +++ b/lib/perl5db.pl @@ -529,7 +529,7 @@ BEGIN { use vars qw($VERSION $header); # bump to X.XX in blead, only use X.XX_XX in maint -$VERSION = '1.51'; +$VERSION = '1.52'; $header = "perl5db.pl version $VERSION"; @@ -1871,7 +1871,8 @@ sub _DB__trim_command_and_return_first_component { $cmd =~ s/\A\s+//s; # trim annoying leading whitespace $cmd =~ s/\s+\z//s; # trim annoying trailing whitespace - my ($verb, $args) = $cmd =~ m{\A(\S*)\s*(.*)}s; + my ($verb, $args) = $cmd =~ m{\A(?| (\w\b) \s* (.*) + | (\S*) \s* (.*) )}sx; $obj->cmd_verb($verb); $obj->cmd_args($args); diff --git a/lib/perl5db.t b/lib/perl5db.t index a2dccc6fd3..d64799f7e7 100644 --- a/lib/perl5db.t +++ b/lib/perl5db.t @@ -31,7 +31,7 @@ BEGIN { $ENV{PERL_RL} = 'Perl'; # Suppress system Term::ReadLine::Gnu } -plan(123); +plan(124); my $rc_filename = '.perldb'; @@ -788,6 +788,25 @@ sub _calc_trace_wrapper "p command works."); } +{ + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'n', + 'p"exp=<$exp>"', + 'q', + ], + prog => '../lib/perl5db/t/break-on-dot', + } + ); + + $wrapper->contents_like( + qr/^exp=<1>$/m, + 'RT #120174: require no space for single-letter commands', + ); +} + # Tests for x. { my $wrapper = DebugWrap->new( -- 2.14.1 ```
p5pRT commented 7 years ago

From @Smylers

Aaron Crane via RT writes​:

James E Keenan via RT \perlbug\-followup@&#8203;perl\.org wrote​:

Since it was an accidental misfeature\, restoring it to working condition should be the responsibility of those who want to restore it.

That seems harsh. Wanting a feature (accidental or not\, mis- or not) to remain shouldn't be dependent on knowing how to implement it.

However\, I thought I might more usefully spend time looking into what it would take to do this than simply to argue for it.

After Jim's excellent dissection\, and spotting that the diffs with the regressions looked to be handling debugger commands through some sort of dispatch table\, I thought it sounds like the kind of thing that might just require a tweak to a regexp that pulls the commands out of the input\, so I started looking in that file\, and came up with the attached.

And it will be *a lot more* work for [those who want to restore it]!

3. It may complicate the code of perl5db.pl and introduce ugliness\, workarounds and edge cases.

Will it ever! The debugger is an ugly beast\, but it was uglier still before your refactoring project.

On the contrary\, fixing this regression involves making a trivial change to exactly one regex — it's much easier to fix it than to write a test for the behaviour (in either direction)! (And I say this as someone who had literally never looked at the internals of the debugger before now.)

Accordingly\, I've attached a patch that fixes this regression\, including a test for the original behaviour; and I will apply it in a few days unless overruled by the pumpking.

p5pRT commented 7 years ago

From @Smylers

0001-Add-tests-for-p-and-x-commands-without-subsequent-wh.patch ```diff From 96a4037df59f332f3cb9affdc33718e114098227 Mon Sep 17 00:00:00 2001 From: James E Keenan Date: Sat, 2 Sep 2017 22:28:20 -0400 Subject: [PATCH 1/2] Add tests for 'p' and 'x' commands without subsequent whitespace. Tests pass on perl-5.16.3 but should fail (until source code is corrected) on subsequent versions. For: RT #120174 --- MANIFEST | 1 + lib/perl5db.t | 86 ++++++++++++++++++++++++++++++++++++++++++++++++- lib/perl5db/t/rt-120174 | 4 +++ 3 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 lib/perl5db/t/rt-120174 diff --git MANIFEST MANIFEST index effc4665..9a200067 100644 --- MANIFEST +++ MANIFEST @@ -4604,6 +4604,7 @@ lib/perl5db/t/lvalue-bug Tests for the Perl debugger lib/perl5db/t/MyModule.pm Tests for the Perl debugger lib/perl5db/t/proxy-constants Tests for the Perl debugger lib/perl5db/t/rt-104168 Tests for the Perl debugger +lib/perl5db/t/rt-120174 Tests for the Perl debugger lib/perl5db/t/rt-121509-restart-after-chdir Tests for the Perl debugger lib/perl5db/t/rt-61222 Tests for the Perl debugger lib/perl5db/t/rt-66110 Tests for the Perl debugger diff --git lib/perl5db.t lib/perl5db.t index a2dccc6f..3d432ad5 100644 --- lib/perl5db.t +++ lib/perl5db.t @@ -31,7 +31,7 @@ BEGIN { $ENV{PERL_RL} = 'Perl'; # Suppress system Term::ReadLine::Gnu } -plan(123); +plan(127); my $rc_filename = '.perldb'; @@ -2817,6 +2817,90 @@ SKIP: ); } +{ + # perl 5 RT #120174 - 'p' command + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'b 2', + 'c', + 'p@abc', + 'q', + ], + prog => '../lib/perl5db/t/rt-120174', + } + ); + + $wrapper->contents_like( + qr/1234/, + q/RT 120174: p command can be invoked without space after 'p'/, + ); +} + +{ + # perl 5 RT #120174 - 'x' command on array + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'b 2', + 'c', + 'x@abc', + 'q', + ], + prog => '../lib/perl5db/t/rt-120174', + } + ); + + $wrapper->contents_like( + qr/0\s+1\n1\s+2\n2\s+3\n3\s+4/ms, + q/RT 120174: x command can be invoked without space after 'x' before array/, + ); +} + +{ + # perl 5 RT #120174 - 'x' command on array ref + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'b 2', + 'c', + 'x\@abc', + 'q', + ], + prog => '../lib/perl5db/t/rt-120174', + } + ); + + $wrapper->contents_like( + qr/\s+0\s+1\n\s+1\s+2\n\s+2\s+3\n\s+3\s+4/ms, + q/RT 120174: x command can be invoked without space after 'x' before array ref/, + ); +} + +{ + # perl 5 RT #120174 - 'x' command on hash ref + my $wrapper = DebugWrap->new( + { + cmds => + [ + 'b 4', + 'c', + 'x\%xyz', + 'q', + ], + prog => '../lib/perl5db/t/rt-120174', + } + ); + + $wrapper->contents_like( + qr/\s+'alpha'\s+=>\s+'beta'\n\s+'gamma'\s+=>\s+'delta'/ms, + q/RT 120174: x command can be invoked without space after 'x' before hash ref/, + ); +} + END { 1 while unlink ($rc_filename, $out_fn); } diff --git lib/perl5db/t/rt-120174 lib/perl5db/t/rt-120174 new file mode 100644 index 00000000..c79c8510 --- /dev/null +++ lib/perl5db/t/rt-120174 @@ -0,0 +1,4 @@ +@abc = (1..4); +print "hello world\n"; +%xyz = ( 'alpha' => 'beta', 'gamma' => 'delta' ); +print "goodbye world\n"; -- 1.9.1 ```
p5pRT commented 7 years ago

From @Smylers

0002-perl-120174-Debugger-cmds-not-requiring-spaces.patch ```diff From ae6c2f451db98ef95c3f9b7813fd9e2eec21d29e Mon Sep 17 00:00:00 2001 From: Smylers Date: Wed, 6 Sep 2017 12:32:09 +0100 Subject: [PATCH 2/2] [perl #120174] Debugger cmds not requiring spaces Make debugger commands like these work again, not requiring a space between a single-letter command and a following argument which starts with punctuation: p$^V x@ARGV x\@ARGV x\%INC Regressions were in d478d7a0 and 8f144dfc. --- lib/perl5db.pl | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git lib/perl5db.pl lib/perl5db.pl index 265b4441..d0c707e8 100644 --- lib/perl5db.pl +++ lib/perl5db.pl @@ -529,7 +529,7 @@ BEGIN { use vars qw($VERSION $header); # bump to X.XX in blead, only use X.XX_XX in maint -$VERSION = '1.51'; +$VERSION = '1.52'; $header = "perl5db.pl version $VERSION"; @@ -1871,7 +1871,10 @@ sub _DB__trim_command_and_return_first_component { $cmd =~ s/\A\s+//s; # trim annoying leading whitespace $cmd =~ s/\s+\z//s; # trim annoying trailing whitespace - my ($verb, $args) = $cmd =~ m{\A(\S*)\s*(.*)}s; + # A single-character debugger command can be immediately followed by its + # argument if they aren't both alphanumeric; otherwise require space + # between commands and arguments: + my ($verb, $args) = $cmd =~ m{\A(.\b|\S*)\s*(.*)}s; $obj->cmd_verb($verb); $obj->cmd_args($args); -- 1.9.1 ```
p5pRT commented 7 years ago

From @Smylers

I just wrote​:

However\, I thought I might more usefully spend time looking into what it would take to do this than simply to argue for it.

Then somehow only part of my message made it out of my editor window into RT's web form. Apologies for messing that up.

Here's what the rest of my mail should've said​:

After Jim's excellent bisection\, and spotting that the diffs with the regressions looked to be handling debugger commands through some sort of dispatch table\, I thought it sounds like the kind of thing that might just require a tweak to a regexp that pulls the commands out of the input\, so I had a look.

That turned out to be pretty much the case​: the fix\, which passes all the original tests plus Jim's new ones\, is basically just adding 4 characters to a pattern match.

Attached are patches for Jim's tests plus my fix.

... fixing this regression involves making a trivial change to exactly one regex — it's much easier to fix it than to write a test for the behaviour (in either direction)!

Snap!

Sorry; I did the work yesterday\, but ran out of time to submit it then. My apologies for not getting to that sooner\, or otherwise indicating I was working on it\, and the resultant duplication of effort.

(And I say this as someone who had literally never looked at the internals of the debugger before now.)

Ditto. I'm wondering if I could've found and fixed this 4 years ago when I submitted the bug report — and saved everybody much time — though I doubt I would've known where to look without Jim's bisecting last week.

Accordingly\, I've attached a patch that fixes this regression\, including a test for the original behaviour; and I will apply it in a few days unless overruled by the pumpking.

Thank you!

Our patches are basically the same (and that we came up with them independently provides something like code review)​:

• Your match uses /x\, making it easier to read what it's doing. Mine has   a comment attempting to explain the reasoning behind it.

• I used Jim's tests; you wrote a new test. I haven't checked whether   his tests something that yours doesn't. Yours makes use of a file   that's already there\, whereas Jim's adds a new file.

Smylers

p5pRT commented 7 years ago

From @shlomif

Hi all\,

On Thu\, 07 Sep 2017 07​:21​:53 -0700 "Smylers via RT" \perlbug\-followup@&#8203;perl\.org wrote​:

I just wrote​:

However\, I thought I might more usefully spend time looking into what it would take to do this than simply to argue for it.

Then somehow only part of my message made it out of my editor window into RT's web form. Apologies for messing that up.

Here's what the rest of my mail should've said​:

After Jim's excellent bisection\, and spotting that the diffs with the regressions looked to be handling debugger commands through some sort of dispatch table\, I thought it sounds like the kind of thing that might just require a tweak to a regexp that pulls the commands out of the input\, so I had a look.

That turned out to be pretty much the case​: the fix\, which passes all the original tests plus Jim's new ones\, is basically just adding 4 characters to a pattern match.

Attached are patches for Jim's tests plus my fix.

... fixing this regression involves making a trivial change to exactly one regex — it's much easier to fix it than to write a test for the behaviour (in either direction)!

Snap!

Sorry; I did the work yesterday\, but ran out of time to submit it then. My apologies for not getting to that sooner\, or otherwise indicating I was working on it\, and the resultant duplication of effort.

(And I say this as someone who had literally never looked at the internals of the debugger before now.)

Ditto. I'm wondering if I could've found and fixed this 4 years ago when I submitted the bug report — and saved everybody much time — though I doubt I would've known where to look without Jim's bisecting last week.

Accordingly\, I've attached a patch that fixes this regression\, including a test for the original behaviour; and I will apply it in a few days unless overruled by the pumpking.

Thank you!

Our patches are basically the same (and that we came up with them independently provides something like code review)​:

• Your match uses /x\, making it easier to read what it's doing. Mine has a comment attempting to explain the reasoning behind it.

• I used Jim's tests; you wrote a new test. I haven't checked whether his tests something that yours doesn't. Yours makes use of a file that's already there\, whereas Jim's adds a new file.

Seeing this twist of events where two patches were provided and they are not too intrusive\, I'd like to say that I am leaning towards applying one of them (or a third one that combines them) so the people who reported the problem and those who prepared the patches will be happier. Note though that I'd prefer if the regex read "\S\b" instead of ".\b" because "." is quite unspecific.

I still think this was an accidental misfeature but don't mind it being restored and supported. So +0.5 from me.

Regards\,

  Shlomi

Smylers

--- via perlbug​: queue​: perl5 status​: open https://rt-archive.perl.org/perl5/Ticket/Display.html?id=120174

--


Shlomi Fish http​://www.shlomifish.org/

I come to bury Caesar\, not to praise him.   — https://en.wikiquote.org/wiki/Julius_Caesar_%28play%29

p5pRT commented 7 years ago

From @Smylers

On Thu\, 07 Sep 2017 09​:11​:55 -0700\, shlomif@​shlomifish.org wrote​:

Seeing this twist of events where two patches were provided and they are not too intrusive\, I'd like to say that I am leaning towards applying one of them

Yay! Thanks\, Shlomi.

Note though that I'd prefer if the regex read "\S\b" instead of ".\b" because "." is quite unspecific.

Note the lines immediately preceding the change are​:

  $cmd =~ s/\A\s+//s; # trim annoying leading whitespace   $cmd =~ s/\s+\z//s; # trim annoying trailing whitespace

So /^./ can't possibly be whitespace here\, and as such is equivalent to /^\S/.

Aaron's patch has /^\w\b/ instead of /^.\b/ — I chose the latter cos it would also match perl's behaviour in not requiring a space between a punctuation operator and a following word.

Looking to see whether any such debugger commands exist\, it means that with my patch you can do things like this\, without a space after the = sign​:

  =help h

That doesn't work without the patch\, and presumably also doesn't work with Aaron's patch. I haven't checked whether it used to work.

The | and ! debugger commands don't seem to require a space even without my patch (presumably they are handled separately)\, so allowing = to work without the space would make = consistent with those.

Smylers

p5pRT commented 7 years ago

From @xsawyerx

From the desk of the pumpking​: (Sorry\, I was just role-dropped a few times\, I thought of injecting some humor into this.)

It is far more common for me to say "No" than "Yes\," despite enjoying seeing more happen than less. In this case I do not see yet why a "No" is a clear decision. Here is what I've seen so far​:

* This went unnoticed for a while\, but now it has. * This isn't documented\, but at least one report shows someone tripped on it\, enough to report it. (This could lend to assuming others have as well.) * The fix is not complicated\, nor does it complicate the code. * It stays in line with how you would expect one-liners to be written in Perl ("my$x=0").

Considering​: 1. We prefer backwards compatible when possible[1]. 2. The patches provided exhibit a limited-scope simple fix. 3. This does not add any load or complexity to the code (or abysmal one);

I really do not see why we shouldn't fix it and move on I would be happy to hear strong arguments as to why we shouldn't apply one of the available fixes\, keep backwards compatibility\, and make those affected[2] happy. Otherwise\, I'm in favor of applying one the patches.

S.

[1] And I know\, I've often been quoting as though I don't. But I do prefer it. [2] Especially Smylers!

On 09/07/2017 03​:01 PM\, Aaron Crane wrote​:

James E Keenan via RT \perlbug\-followup@&#8203;perl\.org wrote​:

Yes\, ultimately it is the pumpking's decision. In this case\, I hope he will perform the most difficult but most necessary function in the pumpking's role. I hope he will say\, "No."

My personal inclination is to keep the regression because​: 1. I feel it was an accidental misfeature. I agree. Type 'perldoc perldebug' in perl-5.16.3 -- the last production version where you could type a 'p' or an 'x' not followed by a wordspace -- and you will find that the wordspace-less version of the command is neither documented nor tested. You are correct is stating that it was an accidental misfeature. I disagree. This change in behaviour has caused sufficient annoyance that at least one user has felt motivated to put the effort into reporting it\, and to engaging with subsequent discussion about the issue. Furthermore\, the previous behaviour dates back to at least Perl 4.000\, as far as I can tell; we've certainly been implicitly teaching our users for a long time that they can rely on it. "We probably wouldn't intentionally do it like that today" doesn't seem like a strong argument in favour of retaining this regression.

Since it was an accidental misfeature\, restoring it to working condition should be the responsibility of those who want to restore it. That seems harsh. Wanting a feature (accidental or not\, mis- or not) to remain shouldn't be dependent on knowing how to implement it.

And it will be *a lot more* work for [those who want to restore it]!

3. It may complicate the code of perl5db.pl and introduce ugliness\, workarounds and edge cases. Will it ever! The debugger is an ugly beast\, but it was uglier still before your refactoring project. On the contrary\, fixing this regression involves making a trivial change to exactly one regex — it's much easier to fix it than to write a test for the behaviour (in either direction)! (And I say this as someone who had literally never looked at the internals of the debugger before now.)

Accordingly\, I've attached a patch that fixes this regression\, including a test for the original behaviour; and I will apply it in a few days unless overruled by the pumpking.

p5pRT commented 7 years ago

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

p5pRT commented 7 years ago

From @arc

Smylers via RT \perlbug\-followup@&#8203;perl\.org wrote​:

Attached are patches for Jim's tests plus my fix.

I've gone with Jim's tests (which are rather more extensive than mine) and your fix.

Thanks\, applied as 7fdd4f080863703d44282c6988834455d1290405 and 582a8ad99532af5c8db4e42c8880618fbce41c6d.

-- Aaron Crane