Repository navigation
shell: remove only trailing line ends from a quoted command substitution #45339
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
Open
robobun
wants to merge
1
commit into
main
Choose a base branch
from
robobun/6d873e57/shell-cmdsubst-keep-trailing-whitespace
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Scripts that store a command's output in a variable and later compare it to, or reuse it beside, a fresh quoted
"$(...)"of the same command now get a mismatch when the output ends in a blank, where the base branch and bash both match. In child_done at src/runtime/shell/states/Expansion.rs:667-676 the assign_ctx arm trims\t\r\nwhile the word arm keeps the blank, soX="$(docker ps)"; [[ "$(docker ps)" == "$X" ]]is false andF="$(cmd)"; echo hi > "$(cmd)"; cat "$F"misses the file. Fix: give both forms the same bytes while keepingping $IPworking, e.g. store the exact value and drop trailing blanks only when an unquoted$Xis expanded (SimpleAtom::Var at Expansion.rs:492).Why this was flagged
A shell script runs a command whose output ends in a blank before its newline, which
hostname -I,docker ps(tabwriter-padded columns) and Windows table tools such aswmicalways produce, and uses the output both through an assignment and through a quoted substitution in a word. Expansion::child_done takes the assign_ctx arm at src/runtime/shell/states/Expansion.rs:667-673 forX="$(cmd)"and truncates,\t,\r,\n, but for"$(cmd)"in a command word, redirect target or[[ ]]operand it takes the arm at Expansion.rs:674-676 and strip_trailing_line_ends at Expansion.rs:557-564 keeps the blank. On the base branch both arms ran the same trim loop (the deleted lines in this hunk), so the words were equal; bash keeps the blank in both, so they are equal there too. No existing safeguard applies: SimpleAtom::Var at Expansion.rs:492-502 copies the stored bytes verbatim and has no quoted flag to trim on.Verification: In src/runtime/shell/states/Expansion.rs:666-677 (
child_done), underno_split, theif me.assign_ctxarm (lines 667-673) truncates every trailing,\n,\r,\t, while theelsearm (line 675) callsstrip_trailing_line_ends(lines 557-564), which only pops\n/\rand only when the last byte is\n, so a trailing space/tab survives.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed. The PR body lists this mismatch under Downsides, and the Notes show both cells. I did not change it, for these reasons.
The exact value in the variable was the first version of this PR. The earlier review thread found what it breaks:
IP=$(hostname -I); ping $IPpasses172.30.0.2with the space, where main and bash pass none.The rule proposed here (exact value, and drop the blanks at an unquoted
$X) must know that$Xis unquoted.SimpleAtom::Varhas no such flag: the lexer pushes the sameToken::Varfor$Xand"$X"(src/shell_parser/parse.rs:2659). #44904 adds that flag (QuotedVar) and is a draft. This PR does not copy its parser change.With the flag, that rule still has a case that matches neither main nor bash. I checked both on main and on bash 5.2:
$X:X="a "; Y=b; echo $X$Yprintsa bon main and in bash. It would printab.X=$(echo 'a '); touch $X.txtcreatesa.txton main, andaand.txtin bash. It would createa .txt.So each partial rule keeps one such case: the exact value (an unquoted
$X), this version (a variable next to a fresh quoted substitution), and the proposed rule (a padded variable followed by text in an unquoted word). Only field splitting of unquoted expansions removes all three. That is a larger change, and #44904, #45968 and #43861 each hold a part of it.This PR keeps the version whose case is the rarest: it needs output that ends in a blank, and a script that uses that output through a variable and through a quoted substitution. If a maintainer prefers the exact value in variables, remove the
assign_ctxarm here. Then thetodotest passes, and the test above it pins what breaks.