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.

Sign up for GitHub

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

Jump to bottom

doskey macros broken for inputs with >1 consecutive whitespace #15736

Closed
lhecker opened this issue Jul 20, 2023 · 3 comments · Fixed by #17129
Closed

doskey macros broken for inputs with >1 consecutive whitespace #15736

lhecker opened this issue Jul 20, 2023 · 3 comments · Fixed by #17129
Assignees
@lhecker
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Cmd.exe The issue is related to the legacy command interpreter, CMD.exe. Product-Conhost For issues in the Console codebase
Milestone
Terminal v1.21

Comments

@lhecker
Copy link
Member

lhecker commented Jul 20, 2023 •
edited

Steps to reproduce

Run this:

doskey foo=echo start $3 $2 $1 end
foo a b c
foo a  b c

Expected Behavior

start c b a end
start c b a end

Actual Behavior

start c b a end
start b  a end

The issue is caused by this function:

terminal/src/host/alias.cpp

Lines 833 to 861 in 42e9ddc

// Routine Description:
// - Tokenizes a string into a collection using space as a separator
// Arguments:
// - str - String to tokenize
// Return Value:
// - Collection of tokenized strings
std::deque<std::wstring> Alias::s_Tokenize(const std::wstring& str)
{
std::deque<std::wstring> result;
size_t prevIndex = 0;
auto spaceIndex = str.find(L' ');
while (std::wstring::npos != spaceIndex)
{
const auto length = spaceIndex - prevIndex;
result.emplace_back(str.substr(prevIndex, length));
spaceIndex++;
prevIndex = spaceIndex;
spaceIndex = str.find(L' ', spaceIndex);
}
// Place the final one into the set.
result.emplace_back(str.substr(prevIndex));
return result;
}

For an input like "a b c" it'll return

"a"
""
"b"
"c"

The issue can either be fixed by fixing s_Tokenize itself or by reverting the code to the previous conhost v1 code. Either would likely be fine.
@lhecker lhecker added Product-Conhost For issues in the Console codebase Product-Cmd.exe The issue is related to the legacy command interpreter, CMD.exe. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) labels Jul 20, 2023
@lhecker lhecker added this to the Terminal v1.19 milestone Jul 20, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 20, 2023
@lhecker lhecker removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 20, 2023
@237dmitry
Copy link

In conhost the same behavior

@lhecker
Copy link
Member Author

lhecker commented Jul 21, 2023

For sure! The bug was introduced in 2018. 🙂

@zadjii-msft
Copy link
Member

Note to self: not fixed in #15783

@zadjii-msft zadjii-msft modified the milestones: Terminal v1.20, Backlog Oct 4, 2023
@lhecker lhecker self-assigned this Oct 4, 2023
@github-actions github-actions bot mentioned this issue Mar 14, 2024
Closed
@lhecker lhecker mentioned this issue Apr 25, 2024
Merged
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Apr 25, 2024
github-merge-queue bot pushed a commit that referenced this issue May 2, 2024
@lhecker
Fix doskey macros for inputs with >1 consecutive whitespace (#17129)
c52dca4 
Initially the PR restored the original v1 conhost code for
`MatchAndCopyAlias` which fixed the linked issue.
Afterwards, I've taken the liberty to rewrite the code to use modern
constructs again, primarily `string_view`. Additionally, the v1 code
first counted the number of arguments and then iterated through it
again to assemble them. This new code does both things at once.

Closes #15736

## Validation Steps Performed
The unit tests have been extended to cover multiple consecutive
spaces. All tests pass.
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Cmd.exe The issue is related to the legacy command interpreter, CMD.exe. Product-Conhost For issues in the Console codebase
Projects
None yet
Milestone
Terminal v1.21
Development

Successfully merging a pull request may close this issue.

Fix doskey macros for inputs with >1 consecutive whitespace microsoft/terminal
3 participants
@lhecker @zadjii-msft @237dmitry