Join continuation lines in a builder - #379
Merged
Merged
Conversation
Code scanning reported the script reader in ReleaseVersionAgreementTests for concatenating the halves of a continued shell line with + inside its loop, alert 237 of the class cs/string-concatenation-in-loop. The pending half is a StringBuilder now, appended to and cleared rather than replaced, and what the reader returns does not move. The join is held by the route itself and not only by a fixture: the assembly step in publish.yaml reads its assignment across a continuation, so a reader that stopped joining was red on the real file. The space between the two halves is held by nothing, because every reading after the join tolerates its absence, and that is written down rather than given a fixture that would split a line where no script does. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Refs #375, the class
cs/string-concatenation-in-loop: one alert, in the continuation join of the script readerReleaseVersionAgreementTestswalks a step's shell with.Finishes: #375
The site
Read from the platform at the head this branch starts from:
The line is the
pending += ...insideScript, which glues the halves of a shell line ending in a backslash back together before the readings above it look for a refusal or an assignment.What changed
pendingis aStringBuilder. A continuation line appends its text and a space, a complete line is appended and the joined text taken out, and the builder is cleared rather than replaced. What the reader returns does not move, and nothing outside this one function is touched.Proved by breaking it
The suite is what says the join kept its meaning, on both lines:
Then the two ways this rewrite could go wrong, each done to the reader and restored, with the two route classes run on
net9.0:The first is red on the route itself rather than on a fixture: the assembly step in
publish.yamlreads its assignment across a continuation, so a reader that stops joining takes half a command as the assignment's origin and the refusal is read as comparing something else.The second stays green, and that is disclosed rather than repaired. Every reading downstream of the join tolerates the missing space: the refusal patterns allow zero whitespace at every place a continuation would split them, and an origin is a substring read. So the space is held by nothing, and a fixture that made it bite would have to split a line in a place no shell script splits one, which would be a fixture for the reader and not for the route. What holds the space is the reading.
The means
C#, in the file the alert names.
StringBuilderis in the base library and the test project already uses it in five other files. The alternative was a dismissal; the sibling board that met this class took the same repair, and the query read the site correctly.What this does not say
The alert closes when the analysis next runs over
master, not on this merge, so #375 stays open until that reading is pasted into it. Whether this class arrives again is operations#2176's question and not this change's.No second reader looked at this. The runs above are the evidence in place of one.