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

Debugger command regression: Now requires more spaces #13342

Closed
p5pRT opened this issue Oct 10, 2013 · 24 comments
Closed

Debugger command regression: Now requires more spaces #13342

p5pRT opened this issue Oct 10, 2013 · 24 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Oct 10, 2013

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

Searchable as RT120174$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2013

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​: 01582e5
  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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 3, 2017

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 68cd360
Author​: Jesse Vincent <jesse@​bestpractical.com>
Date​: Tue Apr 24 19​:02​:34 2012 -0400
-$VERSION = '1.36';
+$VERSION = '1.37';

commit 1799399
Author​: Ricardo Signes <rjbs@​cpan.org>
AuthorDate​: Fri Jun 7 11​:51​:45 2013 -0400
Commit​: Ricardo Signes <rjbs@​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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 3, 2017

From @jkeenan

120174-0001-Add-tests-for-p-and-x-commands-without-subsequent-wh.patch
From 9cbf7940aed97cb73313c28dc2ba2fd85cc19458 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 3, 2017

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 3, 2017

From @jkeenan

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

Add diff.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 3, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 3, 2017

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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 3, 2017

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 68cd360
Author​: Jesse Vincent <jesse@​bestpractical.com>
Date​: Tue Apr 24 19​:02​:34 2012 -0400
-$VERSION = '1.36';
+$VERSION = '1.37';

commit 1799399
Author​: Ricardo Signes <rjbs@​cpan.org>
AuthorDate​: Fri Jun 7 11​:51​:45 2013 -0400
Commit​: Ricardo Signes <rjbs@​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 d478d7a |cat
commit d478d7a
Author​: Shlomi Fish <shlomif@​shlomifish.org>
AuthorDate​: Sun Oct 14 12​:56​:53 2012 +0200
Commit​: Ricardo Signes <rjbs@​cpan.org>
CommitDate​: Mon Nov 12 09​:18​:40 2012 -0500

  Add more commands to the lookup table.

Inline Patch
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<S> - 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<X> - list variables in current package
 
 Since the C<V> command actually processes this, just change this to the
@@ -2784,8 +2788,6 @@ Uses C<dumpvar.pl> to dump out the current values for selected variables.
 
 =cut
 
-                $obj->_handle_V_command_and_X_command;
-
 =head4 C<x> - evaluate and print an expression
 
 Hands the expression off to C<DB::eval>, setting it up to print the value
@@ -2793,22 +2795,16 @@ via C<dumpvar.pl> instead of just printing it directly.
 
 =cut
 
-                $obj->_handle_x_command;
-
 =head4 C<m> - print methods
 
 Just uses C<DB::methods> to determine what methods are available.
 
 =cut
 
-                _DB__handle_m_command($obj);
-
 =head4 C<f> - 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 8f144df |cat
commit 8f144df
Author​: Shlomi Fish <shlomif@​shlomifish.org>
AuthorDate​: Sun Oct 14 13​:22​:45 2012 +0200
Commit​: Ricardo Signes <rjbs@​cpan.org>
CommitDate​: Mon Nov 12 09​:18​:41 2012 -0500

  Low hanging fruit is now in %cmd_lookup.

Inline Patch
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<DB::print_trace>.
 
 Just calls C<DB::cmd_w>.
 
-=cut
-
-                $obj->_handle_w_command;
-
 =head4 C<W> - watch-expression processing.
 
 Just calls C<DB::cmd_W>.
 
-=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<DB::system> to avoid problems with C<STDIN> and C<STDOUT>.
 
 Prints the contents of C<@hist> (if any).
 
-=cut
-
-                $obj->_handle_H_command;
-
 =head4 C<man, doc, perldoc> - look up documentation
 
 Just calls C<runman()> to print the appropriate document.
@@ -2980,36 +2980,19 @@ Just calls C<runman()> to print the appropriate document.
 Builds a C<print EXPR> 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<source> - read commands from a file.
 
 Opens a lexical filehandle and stacks it on C<@cmdfhs>; C<DB::readline> will
 pick it up.
 
-=cut
-
-                $obj->_handle_source_command;
-
 =head4 C<enable> C<disable> - enable or disable breakpoints
 
 This enables or disables breakpoints.
 
-=cut
-
-                $obj->_handle_enable_disable_commands;
-
 =head4 C<save> - send current history to a file
 
 Takes the complete history, (not the shrunken version you see with C<H>),
@@ -3017,11 +3000,6 @@ and saves it to the given filename, so it can be replayed using C<source>.
 
 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<R> - restart
 
 Restart the debugger session.
@@ -3030,12 +3008,6 @@ Restart the debugger session.
 
 Return to any given position in the B<true>-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<OUT> (the debugger's output filehandle) and C<STDOUT>
#####

Thank you very much.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 4, 2017

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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 4, 2017

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 68cd360
Author​: Jesse Vincent <jesse@​bestpractical.com>
Date​: Tue Apr 24 19​:02​:34 2012 -0400
-$VERSION = '1.36';
+$VERSION = '1.37';

commit 1799399
Author​: Ricardo Signes <rjbs@​cpan.org>
AuthorDate​: Fri Jun 7 11​:51​:45 2013 -0400
Commit​: Ricardo Signes <rjbs@​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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 5, 2017

From @shlomif

Hi all!

On Mon, 04 Sep 2017 01​:51​:18 -0700
"Smylers via RT" <perlbug-followup@​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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 5, 2017

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@​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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 5, 2017

From @shlomif

Hi James and all,

On Tue, 05 Sep 2017 13​:59​:21 -0700
"James E Keenan via RT" <perlbug-followup@​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@​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/

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 7, 2017

From @arc

James E Keenan via RT <perlbug-followup@​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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 7, 2017

From @arc

0001-RT-120174-regression-in-single-letter-debugger-comma.patch
From a763defe67e9e682795335b7f47260401b1069a2 Mon Sep 17 00:00:00 2001
From: Aaron Crane <arc@cpan.org>
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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 7, 2017

From @Smylers

Aaron Crane via RT writes​:

James E Keenan via RT <perlbug-followup@​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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 7, 2017

From @Smylers

0001-Add-tests-for-p-and-x-commands-without-subsequent-wh.patch
From 96a4037df59f332f3cb9affdc33718e114098227 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 7, 2017

From @Smylers

0002-perl-120174-Debugger-cmds-not-requiring-spaces.patch
From ae6c2f451db98ef95c3f9b7813fd9e2eec21d29e Mon Sep 17 00:00:00 2001
From: Smylers <Smylers@stripey.com>
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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 7, 2017

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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 7, 2017

From @shlomif

Hi all,

On Thu, 07 Sep 2017 07​:21​:53 -0700
"Smylers via RT" <perlbug-followup@​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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 7, 2017

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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 11, 2017

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@​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
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 30, 2017

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

@p5pRT p5pRT closed this Sep 30, 2017
@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 30, 2017

From @arc

Smylers via RT <perlbug-followup@​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 7fdd4f0 and
582a8ad.

--
Aaron Crane

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

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.