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

Ensure duplicate commands with different exit codes show correct command decorations #143766

Closed
Tyriar opened this issue Feb 23, 2022 · 3 comments · Fixed by #154363
Closed

Ensure duplicate commands with different exit codes show correct command decorations #143766

Tyriar opened this issue Feb 23, 2022 · 3 comments · Fixed by #154363
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders linux Issues with VS Code on Linux terminal-shell-bash An issue in the terminal specific to bash terminal-shell-integration Shell integration, command decorations, etc. terminal-shell-zsh An issue in the terminal specific to zsh verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Feb 23, 2022

This is a follow up from #143707 which worked around the majority case of an issue.

See the workaround: 78a9a54

The proper fix will involve sending both the exit code and the history ID over to VS Code which will then do what's happening now but also have the correct exit code to set, instead of assuming it's the last command's exit code.

We should write a little script as part of this that alternates exit codes by setting/unsetting an environment variable so it's easier to test and verify.

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug linux Issues with VS Code on Linux terminal-shell-integration Shell integration, command decorations, etc. terminal-command-decoration labels Feb 23, 2022
@Tyriar Tyriar added this to the March 2022 milestone Feb 23, 2022
@Tyriar Tyriar added terminal-shell-bash An issue in the terminal specific to bash terminal-shell-zsh An issue in the terminal specific to zsh labels Jun 3, 2022
@Tyriar Tyriar modified the milestones: Backlog, July 2022 Jul 7, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2022

The issue being worked around is actually caused by ignoredups or ignoreboth being included in $HISTCONTROL.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2022

Good test case from #154343

python -c $'import random\nif (random.randint(0, 1000) % 2 == 0):\n raise NotImplementedError \n'

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2022

To verify:

  1. Run some commands
  2. Check Terminal: Run Recent Command, they should all show up in current session
  3. Press ctrl+c in between some commands, including typing out a command and hitting ctrl+c to abandon it
  4. Check Terminal: Run Recent Command, the lines with ctrl+c shouldn't show up
  5. Run python -c $'import random\nif (random.randint(0, 1000) % 2 == 0):\n raise NotImplementedError \n' several times, failed and success should match whether the exception was thrown.

@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jul 7, 2022
@connor4312 connor4312 added the verified Verification succeeded label Jul 28, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders linux Issues with VS Code on Linux terminal-shell-bash An issue in the terminal specific to bash terminal-shell-integration Shell integration, command decorations, etc. terminal-shell-zsh An issue in the terminal specific to zsh verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants