terminal icon indicating copy to clipboard operation
terminal copied to clipboard

Fixes command-line args parsing when one-character argument is followed by `;`

Open serd2011 opened this issue 3 years ago • 0 comments
trafficstars

Summary of the Pull Request

Changes the way _addCommandsForArg determines if the delimiter was at the beginning of the argument so that it accounts for the fact that match includes the last character of the string before it.

PR Checklist

  • [x] Closes #13277
  • [x] CLA signed. If not, go over here and sign the CLA
  • [ ] Tests added/passed
  • [ ] Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • [ ] Schema updated.
  • [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #13436

Validation Steps Performed

wt -p "u"; nt -p "u" does not cause an error

serd2011 avatar Aug 09 '22 05:08 serd2011

Would it be ok add them to ParseTrickyCommandlines or ParseSimpleCommandline? Is testing that one-character argument parsed correctly enough or are there more test cases that need to be added?

serd2011 avatar Aug 11 '22 05:08 serd2011

ParseTrickyCommandlines seems fine to me ; the wt -p "u"; nt -p "u" test case seems good enough of a test to me 😄

zadjii-msft avatar Aug 11 '22 20:08 zadjii-msft

(Future improvement:) Looking at _commandDelimiterRegex, I get the feeling that this code would be a lot more robust if we'd just find(L';') and check if the preceding character is != L'\\'.

Yes and no.

What if the character before the \ is \? \\; is not the same as \;!

DHowett avatar Aug 12 '22 17:08 DHowett

What if the character before the \ is \? \\; is not the same as \;!

The regex is currently (^;|[^\\];) and described as

Either a ; at the start of a line, or a ; preceded by any non-\ char.

There's nothing in the regex that would handle such a case. If we handle it, it must be elsewhere or not at all...

lhecker avatar Aug 12 '22 17:08 lhecker

This vaguely worries me, actually. Why are we using a regex to match a character followed by ; when what we want to know is whether there is a ;? Is it just because we need to match anything except a \?

Leonard's right, we should be using a more direct/implementation-specific string parser rather than a regex. It would give us control over exactly when we match and when we "vend". :)

DHowett avatar Aug 12 '22 22:08 DHowett

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

msftbot[bot] avatar Aug 12 '22 22:08 msftbot[bot]

:tada:Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

msftbot[bot] avatar Sep 13 '22 17:09 msftbot[bot]