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.

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

Discard old duplicates needs to be case-sensitive #4186

Closed
cniggeler opened this issue Jan 12, 2020 · 8 comments · Fixed by #12700
Closed

Discard old duplicates needs to be case-sensitive #4186

cniggeler opened this issue Jan 12, 2020 · 8 comments · Fixed by #12700
Labels
Area-CookedRead The cmd.exe COOKED_READ handling 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) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. zInbox-Bug Ignore me!

Comments

@cniggeler
Copy link

Windows build number: [run `[Environment]::OSVersion` for powershell, or `ver` for cmd]
Windows Terminal version (if applicable):
Microsoft Windows [Version 10.0.18362.535]

Any other software? No, none required

Steps to reproduce

Command Prompt Properties - check "Discard Old Duplicates" in history section
Enable the Experimental Console (UNcheck "Use Legacy Console..."

Expected behavior

Run the two commands,
foo /v
foo /V

Then use up-arrow to view the history. You should see BOTH in the history. This is the case for the classic console.

Actual behavior

In the experimental console, only the FIRST instance (foo /v) is preserved. That is, the new console acting case-insensitive, but 'V' and 'v' can have significance to "foo"!

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 12, 2020
@cniggeler cniggeler changed the title Discard old duplicates is case-sensitive Discard old duplicates needs to be case-sensitive Jan 12, 2020
@DHowett-MSFT DHowett-MSFT added 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. Product-Conhost For issues in the Console codebase labels Jan 13, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 13, 2020
@DHowett-MSFT DHowett-MSFT added this to the 21H1 milestone Jan 13, 2020
@DHowett-MSFT DHowett-MSFT added Priority-2 A description (P2) and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jan 13, 2020
DHowett pushed a commit that referenced this issue May 28, 2020
For a radio button group to work properly, they need sequential IDs.
This moves the cursor radio buttons on the `conhost` property sheet to
be sequential.

## References
- Introduced with #2663
- Found while investigating #4186

## PR Checklist
* [x] Closes unfiled issue found while investigating #4186
* [x] I work here.
* [x] Manual test.
* [x] No documentation required.
* [x] Am core contributor.

## Detailed Description of the Pull Request / Additional comments
- `CheckRadioButton` takes a contiguous group of IDs. It will set one
  item in the list and then uncheck the rest. When a new one was added
  to the group, it was added to the end of the segment in the IDs file,
  but not immediately after the existing radio buttons. This means it
  accidentally turned off all the other buttons in the middle.
- To resolve this, I moved all the cursor buttons into their own
  sequential group number and I deprecated the old values.

## Validation Steps Performed
- [x] Ensured that the "Discard Old Duplicates" value was set in the
  registry, walked through debugger as `conhost` packed the `TRUE` value
  into the property sheet blob, walked through the property sheet
  `console.dll` as it unpacked the `TRUE`, then observed that the
  checkbox was actually set instead of getting unset by the
  `CheckRadioButton` call that went from 107 to 119 and accidentally
  unchecked number 112, `IDD_HISTORY_NODUP` even though I swear it was
  just set.
@zadjii-msft zadjii-msft modified the milestones: Windows vNext, 22H1 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, Terminal v1.14 Feb 2, 2022
@zadjii-msft zadjii-msft added the Area-CookedRead The cmd.exe COOKED_READ handling label Feb 23, 2022
@ghost ghost added the In-PR This issue has a related PR label Mar 15, 2022
@DHowett DHowett added the zInbox-Bug Ignore me! label Mar 18, 2022
@ghost ghost closed this as completed in #12700 Mar 18, 2022
ghost pushed a commit that referenced this issue Mar 18, 2022
The legacy console used to use case-sensitive history deduplication and
this change reverts the logic to restore ye olde history functionality.

This commit additionally changes the other remaining `std::equal` plus
`std::towlower` check into a `CompareStringOrdinal` call,
just because that's what MSDN suggests to use in such situations.

## PR Checklist
* [x] Closes #4186
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* Enter `test /v`
* Enter `test /V`
* Browsing through the history yields both items ✅
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Mar 18, 2022
DHowett pushed a commit that referenced this issue Mar 24, 2022
The legacy console used to use case-sensitive history deduplication and
this change reverts the logic to restore ye olde history functionality.

This commit additionally changes the other remaining `std::equal` plus
`std::towlower` check into a `CompareStringOrdinal` call,
just because that's what MSDN suggests to use in such situations.

## PR Checklist
* [x] Closes #4186
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* Enter `test /v`
* Enter `test /V`
* Browsing through the history yields both items ✅

(cherry picked from commit 6bc2b4a)
@zadjii-msft
Copy link
Member

Ruh-roh: 6bc2b4a#commitcomment-79599160

Somebody has reported that this changed how F8 expansion works . . . and now I'm wondering if we should have done this to begin with. Or, perhaps, should deduplication and expansion be split apart?

FWIW: The legacy console treats F8 as being case sensitive.

That person was in MSFT:40506961. Tagging for more discussion

@zadjii-msft zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 28, 2022
@DHowett
Copy link
Member

DHowett commented Jul 28, 2022

Notes from the reporter:

Windows in general is not case sensitive. For example, try to make a file named FOO.txt and one named foo.txt (doesn’t work). Then, use ‘del’ to remove FoO.TxT (works). The documentation for WSL also notes the case-insensitivity of the Windows filesystem.

I know that’s not conhost, but it still feels weird to match on sensitivity when many operations on the command line aren’t case sensitive. Anyway, just my $0.02.

Thanks,

@cniggeler
Copy link
Author

cniggeler commented Jul 29, 2022

A couple points to @DHowett comment:

  1. Filesystem is not the same as command line.
  2. You can make the filesystem case-sensitive a la fsutil file setCaseSensitiveInfo
  3. Just because you don't run commands that use case sensitivity, maybe other people do?
c:> cl /v
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27045 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

cl : Command line warning D9002 : ignoring unknown option '/v'
cl : Command line error D8003 : missing source filename

c:> cl /V
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27045 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

cl : Command line error D8004 : '/V' requires an argument

@cniggeler
Copy link
Author

cniggeler commented Jul 29, 2022

In reply to @zadjii-msft -

Why "ruh-roh"? I know what it means :-) but when I look at the related commitment link, there's not an issue there, at least not that pops up to me.

It seems to me that deduplication and F8-expansion are intertwined for this issue. If deduplicate buffers only upper-case commands, then lower- or mixed-cased commands simply do not exist to F8-expand, right? That is indeed the original behavior of the new terminal as opposed to the console and why I opened this case.

@zadjii-msft
Copy link
Member

(FWIW, I put the comments here because this thread had the original context. It's easier to re-open an issue for the sake of work tracking than it is to revert a PR, and reopen the original one. That's at least my thought. )

This is more of a we need to talk about this more. I think we initially instant-approved for the backlog because tHaTs hOw tHe lEgAcY CoNsOlE WoRkEd, but I don't think we ever took pause to ideate on "how should this work". Conhostv2 (and by extension, Terminal) gave us the opportunity to improve bits of the console experience. We should actually consider the ideal behavior here, rather than just the legacy behavior for legacy's sake. (That doesn't mean the legacy behavior isn't what we'll settle on, we just need to have more discussion and come to a definitive agreement here.)

@cniggeler
Copy link
Author

I'm all for no-kneejerk reactions, but c'mon, there's such a thing as a smell test. Unless you want Terminal to regress to CP/M, I humbly suggest there's no discussion to be had on this topic!

  • There are NO modern command shells that are not case sensitive.
  • MANY command line applications can have case-sensitive operands. I provided an example of Microsoft's own cl compiler above; gcc is the same.
  • Win10+ CAN have case-sensitive filenames.

So, let's discuss: what could possibly be a "con" to having a case-sensitive history buffer? There's not even a size issue because keeping case-sensitive versions of commands is confined to the user's proscribed history limit!

@DHowett
Copy link
Member

DHowett commented Jul 29, 2022

Just to be clear: nobody is advocating for reverting this fix and making the history buffer case insensitive.

Somebody reported that F8 history search and expansion became case sensitive, and we traced it back to this fix.

I think there is a fighting chance that history deduplication truly should be case sensitive, and that history search based on a prefix (which on Windows is very unlikely to be case sensitive for the amount of a command you'd likely type before searching, as executables on the PATH are not typically in case sensitive directories) should be case insensitive.

That is what there is to discuss. Do we believe that ease of use ("user can search a case-sensitive history with a case-insensitive needle") can be achieved with a rigorous data model ("history is always case-sensitive")?

@cniggeler
Copy link
Author

Appreciate your thoughts and clarification. If I understand correctly, you are now talking solely about the history search being case-insensitive? That is a fine-grained discussion indeed! I don't have an objection, as long as all prior unique commands (case preserved) are presented to me.

I fished around for a counter-example: assume case sensitivity turned on for the filesystem. With PATHEXT and/or file associations, I can enter any old filename, like myfile.docx, at the prompt, and have Word open that file. So if I typed my<F8> and got MySpreadSheet.xlsx as a match, would I be chuffed or huffed? If I took the time to reset the filesystem, answer would be huffed. For 95% of the Windows population, probably OK?

@zadjii-msft zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 22, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CookedRead The cmd.exe COOKED_READ handling 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) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. zInbox-Bug Ignore me!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants