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

Fix doskey macros for inputs with >1 consecutive whitespace #17129

Merged
merged 2 commits into from May 2, 2024
Merged

Fix doskey macros for inputs with >1 consecutive whitespace #17129

merged 2 commits into from May 2, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Apr 25, 2024

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 Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. 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 labels Apr 25, 2024
DHowett
DHowett requested changes Apr 26, 2024
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem like we lost tests for the actual replacements (input redir, output redir, etc, etc), and didn't gain tests for the inputs with >1 consecutive whitespace... we probably should have those!

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 26, 2024
@lhecker
Copy link
Member Author

lhecker commented Apr 26, 2024 •
edited

It does seem like we lost tests for the actual replacements (input redir, output redir, etc, etc),

I'll check whether they were covered by the TestMatchAndCopy test. Since the many small helper functions don't exist anymore, I think it's more than fair if only the big TestMatchAndCopy test is left, since the corresponding MatchAndCopy (technically s_MatchAndCopyAlias) is the only function left on Alias as well.

and didn't gain tests for the inputs with >1 consecutive whitespace... we probably should have those!

Disable suppressed whitespace changes! There's a few more changes at the start of the AliasTests.cpp file.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 26, 2024
lhecker
lhecker commented Apr 26, 2024
View reviewed changes
src/host/ut_host/AliasTests.cpp
Comment on lines -185 to -217
TEST_METHOD(Tokenize)
{
std::wstring tokenStr(L"one two three");
std::deque<std::wstring> tokensExpected;
tokensExpected.emplace_back(L"one");
tokensExpected.emplace_back(L"two");
tokensExpected.emplace_back(L"three");

auto tokensActual = Alias::s_Tokenize(tokenStr);

VERIFY_ARE_EQUAL(tokensExpected.size(), tokensActual.size());

for (size_t i = 0; i < tokensExpected.size(); i++)
{
VERIFY_ARE_EQUAL(String(tokensExpected[i].data()), String(tokensActual[i].data()));
}
}

TEST_METHOD(TokenizeNothing)
{
std::wstring tokenStr(L"alias");
std::deque<std::wstring> tokensExpected;
tokensExpected.emplace_back(tokenStr);

auto tokensActual = Alias::s_Tokenize(tokenStr);

VERIFY_ARE_EQUAL(tokensExpected.size(), tokensActual.size());

for (size_t i = 0; i < tokensExpected.size(); i++)
{
VERIFY_ARE_EQUAL(String(tokensExpected[i].data()), String(tokensActual[i].data()));
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tokenization doesn't exist anymore as a separate function. The megamix test above is probably sufficient to cover tokenization however.

src/host/ut_host/AliasTests.cpp
Comment on lines -219 to -236
TEST_METHOD(GetArgString)
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:targetExpectedPair",
L"{"
L"alias arg1 arg2 arg3=arg1 arg2 arg3,"
L"aliasOnly="
L"}")
END_TEST_METHOD_PROPERTIES()

std::wstring target;
std::wstring expected;
_RetrieveTargetExpectedPair(target, expected);

auto actual = Alias::s_GetArgString(target);

VERIFY_ARE_EQUAL(String(expected.data()), String(actual.data()));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. All s_GetArgString ever did was split off the first "token" (the first word in the string).

src/host/ut_host/AliasTests.cpp
Comment on lines -243 to -251
L"1=one,"
L"2=two,"
L"3=three,"
L"4=four,"
L"5=five,"
L"6=six,"
L"7=seven,"
L"8=eight,"
L"9=nine,"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by identical tests above.

src/host/ut_host/AliasTests.cpp
Comment on lines -252 to -253
L"A=,"
L"0=,"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot possibly happen (see code in alias.cpp).

src/host/ut_host/AliasTests.cpp
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:targetExpectedPair",
L"{"
L"*=one two three,"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by L"bar $*=bar one two three four five six seven eight nine ten eleven twelve%," above.

src/host/ut_host/AliasTests.cpp
Comment on lines -311 to -390
TEST_METHOD(InputRedirMacro)
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:targetExpectedPair",
L"{"
L"L=<,"
L"l=<,"
L"A=,"
L"a=,"
L"0=,"
L"}")
END_TEST_METHOD_PROPERTIES()

std::wstring target;
std::wstring expected;
_RetrieveTargetExpectedPair(target, expected);

// if we expect non-empty results, then we should get a bool back saying it was processed
const auto returnExpected = !expected.empty();

std::wstring actual;
const auto returnActual = Alias::s_TryReplaceInputRedirMacro(target[0], actual);

VERIFY_ARE_EQUAL(returnExpected, returnActual);
VERIFY_ARE_EQUAL(String(expected.data()), String(actual.data()));
}

TEST_METHOD(OutputRedirMacro)
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:targetExpectedPair",
L"{"
L"G=>,"
L"g=>,"
L"A=,"
L"a=,"
L"0=,"
L"}")
END_TEST_METHOD_PROPERTIES()

std::wstring target;
std::wstring expected;
_RetrieveTargetExpectedPair(target, expected);

// if we expect non-empty results, then we should get a bool back saying it was processed
const auto returnExpected = !expected.empty();

std::wstring actual;
const auto returnActual = Alias::s_TryReplaceOutputRedirMacro(target[0], actual);

VERIFY_ARE_EQUAL(returnExpected, returnActual);
VERIFY_ARE_EQUAL(String(expected.data()), String(actual.data()));
}

TEST_METHOD(PipeRedirMacro)
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:targetExpectedPair",
L"{"
L"B=|,"
L"b=|,"
L"A=,"
L"a=,"
L"0=,"
L"}")
END_TEST_METHOD_PROPERTIES()

std::wstring target;
std::wstring expected;
_RetrieveTargetExpectedPair(target, expected);

// if we expect non-empty results, then we should get a bool back saying it was processed
const auto returnExpected = !expected.empty();

std::wstring actual;
const auto returnActual = Alias::s_TryReplacePipeRedirMacro(target[0], actual);

VERIFY_ARE_EQUAL(returnExpected, returnActual);
VERIFY_ARE_EQUAL(String(expected.data()), String(actual.data()));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests above contain various combinations of <, >, and |.

src/host/ut_host/AliasTests.cpp
Comment on lines -397 to -398
L"T=%,"
L"t=%,"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by L"run$tmultiple$tcommands=run%multiple%commands%,".

src/host/ut_host/AliasTests.cpp
Comment on lines -426 to -437
TEST_METHOD(AppendCrLf)
{
std::wstring actual;
size_t lineCountActual = 0;

const std::wstring expected(L"\r\n");
const auto lineCountExpected = lineCountActual + 1;

Alias::s_AppendCrLf(actual, lineCountActual);
VERIFY_ARE_EQUAL(String(expected.data()), String(actual.data()));
VERIFY_ARE_EQUAL(lineCountExpected, lineCountActual);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The s_AppendCrLf has been removed.

@lhecker
Copy link
Member Author

lhecker commented Apr 26, 2024

Yep, looks good. Seems like everything is still covered thankfully. 🙂

DHowett
DHowett reviewed Apr 26, 2024
View reviewed changes
src/host/alias.cpp Outdated
{
// If no read-ahead, just push this character and be done.
finalText.push_back(*ch);
buffer.append(args[0].data(), sourceText.data() + sourceText.size());
Copy link
Member

@DHowett DHowett Apr 26, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this deserves a comment. We're sure that args contains views into sourceText, so even though it looks like we're using a begin from one pointer and an end from a totally different one, it's OK

plus the understanding that args is only the arguments (e.g. it doesn't work like argv where [0] is the program name (!))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also it now works like argv.

@zadjii-msft zadjii-msft added this to the Terminal v1.21 milestone Apr 29, 2024
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label May 2, 2024
zadjii-msft
zadjii-msft approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a very early-in-career me thanks you for re-writing this in a far less verbose way. Sometimes, not everything needs to be an atomic function that has a unittest dedicated to it

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label May 2, 2024
@lhecker lhecker added this pull request to the merge queue May 2, 2024
Merged via the queue into main with commit c52dca4 May 2, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/15736-doskey branch May 2, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@DHowett DHowett Awaiting requested review from DHowett

Assignees
No one assigned
Labels
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. Needs-Attention The core contributors need to come back around and look at this ASAP. 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 this pull request may close these issues.

doskey macros broken for inputs with >1 consecutive whitespace
3 participants
@lhecker @DHowett @zadjii-msft