debugger: run last command on pressing enter by Trott · Pull Request #6090 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

debugger

Description of change

In earlier versions of Node.js, pressing enter in the debugger resulted in the previous command running again. At some point, this was (accidentally, it appears) removed. This reinstates that behavior.

Not sure about the semver-ness of this. Could be a bug fix. Could be a new feature. Could be a breaking change. Inclined to just shrug/table-flip and make it semver-major. The Peoples won't see it until October, then, but The Peoples have been without it for more than a year now, so I think The Peoples will be OK.

Fixes: #2895

LGTM tho I believe a few of the edits to repl were landed in another commit?

@jasnell Yes. That other PR has landed now. This one has been rebased against it. It just needs a doc update and consensus that this is a feature we want to restore.

CI: https://ci.nodejs.org/job/node-test-pull-request/2224/

@nodejs/collaborators This PR restores functionality that existed in the debugger in 0.10 but was (accidentally, I think) removed. Specifically, with this PR, if you are in the debugger and you press enter without entering a command, it will run the previous command. Useful if you don't want to type n 15 times in a row. See bug report at #2895.

Two questions:

Is this functionality we want to restore?
Is this semver-minor? major? patch?

semver: I think after a year, even if the change was accidental, the behaviour had enough time to become expected to anyone using it. IMHO I'd follow the spirit of semver, and make it a semver-major change. Also, this change will mostly break humans, and people tend to read up on changes in much more detail when they go up a semver-major number, so there's more chances they'll get notified. At the very least it's semver-minor.
Is this something we want back: this is 100% preference I think. My own is for a newline to do nothing (this is what most prompts do), not sure what the majority thinks.

@claudiorodriguez Maybe we can restore the behavior but not as the default? Put it behind a command line flag or some other way to turn it on. Also eliminates guesswork on the semver-ness, as that would be a clear semver-minor.

@Trott That sounds like a good compromise to me.

this is 100% preference I think. My own is for a newline to do nothing (this is what most prompts do),

If it matters, the gdb command line will repeat the last command if there is a blank line of input. https://sourceware.org/gdb/onlinedocs/gdb/Command-Syntax.html

'If'? I'm a bit uneasy about this check, it's not very robust. Doing setBreakPoint = 1 in the normal REPL will make this code think it's being called from the debugger.

@bnoordhuis You're right, of course. I think have a better solution that I've just pushed. PTAL.

Fixes: nodejs#2895

CI: https://ci.nodejs.org/job/node-test-pull-request/2235/

Er...make that: https://ci.nodejs.org/job/node-test-pull-request/2236/

Known flaky with a proposed fix pending is the CI failure.

Not sure how to best get consensus on this other than keep asking collaborators to weigh in:

Do we want to restore the debugger behavior that was accidentally broken/removed wherein a blank command in the debugger REPL repeats the last command, like it does in gdb?
If so, do we want to just do it or do we want to put the restored behavior behind a flag or otherwise make it opt-in?
If we restore it without making it opt-in, is there general agreement that that is semver-major?
If your opinion is "restore it and don't hide it behind a flag or anything like that", then does this PR LGTY?

LGTM. I'd just restore it as-is, it's just a bug fix.

I'm +1 on this change LGTM

Two questions:
Is this functionality we want to restore?
Is this semver-minor? major? patch?

+1 for restoring it. Looks semver-minor to me if it was missing in 4.x and 5.x.

I'm going to label this semver-minor. Speak up if you think that's an affront against semver-ness or something.

@claudiorodriguez Would you object to this landing as it is? (I want this to land, but I want every collaborator who has voiced an opinion to be reasonably comfortable with the choices we've made.)

@Trott no objections from me, I think we should go for what most people are comfortable with. LGTM. Also +1 on semver-minor

LGTM

PR-URL: nodejs#6090 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Fixes: nodejs#2895

Landed in 1df84f4

PR-URL: #6090 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Fixes: #2895

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

PR-URL: #6090 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Fixes: #2895

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behavior was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) nodejs#5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) nodejs#6279 * update ESLint to 2.7.0 (silverwind) nodejs#6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) nodejs#6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) nodejs#6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) nodejs#6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) nodejs#6090 src: * add SIGINFO to supported signals (James Reggio) nodejs#6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) nodejs#6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) nodejs#6069 PR-URL: nodejs#6322

PR-URL: #6090 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Fixes: #2895

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

I think this should be marked as dont-land-on-v4.x because it depends on #6071 which is marked as dont-land-on-v4.x too.

Trott added the debugger label Apr 6, 2016

Trott mentioned this pull request Apr 7, 2016

debugger: pressing enter doesn't run the latest action #2895

Closed

jasnell added the lts-watch-v4.x label Apr 7, 2016

Trott force-pushed the a-la-mode branch from d10a531 to dd55579 Compare April 8, 2016 21:01

Trott force-pushed the a-la-mode branch 2 times, most recently from 9dbb4dc to 3330e5f Compare April 8, 2016 22:11

bnoordhuis reviewed Apr 9, 2016
View reviewed changes

debugger: run last command on presssing enter

51fdc49

Fixes: nodejs#2895

Trott force-pushed the a-la-mode branch from 3330e5f to 51fdc49 Compare April 9, 2016 19:58

repl: handle blank lines in evalCmd

e6ccd04

silverwind changed the title debugger: run last command on presssing enter debugger: run last command on pressing enter Apr 13, 2016

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 13, 2016

Trott closed this Apr 14, 2016

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

MylesBorins mentioned this pull request May 16, 2016

Release Schedule for v4.4.5 and v4.5.0 nodejs/Release#102

Closed

This was referenced May 18, 2016

Backport debugger test commits (6205, 6246) #6831

Closed

test: move the debugger tests back to parallel #6246

Closed

Fishrock123 added the dont-land-on-v4.x label May 19, 2016

MylesBorins removed the lts-watch-v4.x label Jun 1, 2016

GeoffreyBooth mentioned this pull request Apr 17, 2017

failure "empty command evaluates to undefined" with nodejs 4.8.2 jashkenas/coffeescript#4502

Closed

Trott deleted the a-la-mode branch January 13, 2022 22:43

debugger: run last command on pressing enter by Trott · Pull Request #6090 · nodejs/node · GitHub

Uh oh!

Uh oh!

debugger: run last command on pressing enter - #6090

debugger: run last command on pressing enter#6090Trott wants to merge 2 commits intonodejs:masternodejs/node:masterfrom Trott:a-la-modeTrott/io.js:a-la-modeCopy head branch name to clipboard

Conversation

Trott commented Apr 6, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Apr 7, 2016

Uh oh!

Trott commented Apr 8, 2016

Uh oh!

Trott commented Apr 8, 2016

Uh oh!

Trott commented Apr 8, 2016

Uh oh!

claudiorodriguez commented Apr 8, 2016

Uh oh!

Trott commented Apr 8, 2016

Uh oh!

claudiorodriguez commented Apr 9, 2016

Uh oh!

Trott commented Apr 9, 2016

Uh oh!

bnoordhuis Apr 9, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Apr 9, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 9, 2016

Uh oh!

Trott commented Apr 10, 2016

Uh oh!

Trott commented Apr 10, 2016

Uh oh!

Trott commented Apr 11, 2016

Uh oh!

bnoordhuis commented Apr 11, 2016

Uh oh!

MylesBorins commented Apr 13, 2016

Uh oh!

ChALkeR commented Apr 13, 2016

Uh oh!

Trott commented Apr 13, 2016

Uh oh!

Trott commented Apr 13, 2016

Uh oh!

claudiorodriguez commented Apr 14, 2016

Uh oh!

cjihrig commented Apr 14, 2016

Uh oh!

Trott commented Apr 14, 2016

Uh oh!

santigimeno commented May 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

debugger: run last command on pressing enter#6090
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:a-la-mode