From f50bf1576e83a4b16d16a9328670eccb76e6aa89 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Thu, 9 Apr 2020 08:50:40 -0500
Subject: [PATCH 01/10] add a test for #5291, and a theory on why it's busted

---
 .../ConptyRoundtripTests.cpp                  | 96 +++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index e85fd243ddd..345351c5b26 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -188,6 +188,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
     TEST_METHOD(OutputWrappedLineWithSpace);
     TEST_METHOD(OutputWrappedLineWithSpaceAtBottomOfBuffer);
 
+    TEST_METHOD(BreakLinesOnCursorMovement);
+
     TEST_METHOD(ScrollWithMargins);
 
 private:
@@ -2186,3 +2188,97 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer()
     Log::Comment(L"========== Checking the terminal buffer state ==========");
     verifyBuffer(termTb, term->_mutableViewport.ToInclusive());
 }
+
+void ConptyRoundtripTests::BreakLinesOnCursorMovement()
+{
+    Log::Comment(L"This is a test for GH#5291. WSL vim uses spaces to clear the"
+                 L" ends of blank lines, not EL. This test ensures that conhost"
+                 L" properly treats those lines as _not actually wrapped_,"
+                 L" because if we leave them \"wrapped\", conpty won't newline"
+                 L" in between them.");
+    VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());
+
+    auto& g = ServiceLocator::LocateGlobals();
+    auto& renderer = *g.pRender;
+    auto& gci = g.getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer();
+    auto& hostSm = si.GetStateMachine();
+
+    auto& termTb = *term->_buffer;
+
+    _flushFirstFrame();
+
+    auto verifyBuffer = [](const TextBuffer& tb,
+                           const til::rectangle viewport) {
+        const auto lastRow = viewport.bottom<short>() - 1;
+        const til::point expectedCursor{ 5, lastRow };
+        VERIFY_ARE_EQUAL(expectedCursor, til::point{ tb.GetCursor().GetPosition() });
+        VERIFY_IS_TRUE(tb.GetCursor().IsVisible());
+
+        for (auto y = viewport.top<short>(); y < lastRow; y++)
+        {
+            TestUtils::VerifyExpectedString(tb, L"~    ", til::point{ 0, y });
+        }
+
+        TestUtils::VerifyExpectedString(tb, L"AAAAA", til::point{ 0, lastRow });
+    };
+
+    // We're _not_ checking the conpty output during this test, only the side effects.
+    _logConpty = true;
+    _checkConptyOutput = false;
+
+    // Lock must be taken to manipulate alt/main buffer state.
+    gci.LockConsole();
+    auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); });
+
+    // Use DECALN to fill the buffer with 'E's.
+    hostSm.ProcessString(L"\x1b#8");
+
+    Log::Comment(L"Painting the frame");
+    VERIFY_SUCCEEDED(renderer.PaintFrame());
+
+    Log::Comment(L"Switching to the alt buffer");
+    hostSm.ProcessString(L"\x1b[?1049h");
+    auto restoreBuffer = wil::scope_exit([&] { hostSm.ProcessString(L"\x1b[?1049l"); });
+    auto& altBuffer = gci.GetActiveOutputBuffer();
+    auto& altTextBuffer = altBuffer.GetTextBuffer();
+
+    // Go home and clear the screen.
+    hostSm.ProcessString(L"\x1b[H");
+    hostSm.ProcessString(L"\x1b[2J");
+
+    // Write out lines of '~' followed by enough spaes to fill the line.
+    hostSm.ProcessString(L"\x1b[94m");
+    for (auto y = 0; y < altBuffer.GetViewport().BottomInclusive(); y++)
+    {
+        {
+            std::wstringstream ss;
+            ss << L"\x1b[";
+            ss << y + 1;
+            ss << L";1H";
+            hostSm.ProcessString(ss.str());
+        }
+
+        // IMPORTANT! The way vim writes these blank lines is as '~' followed by
+        // enough spaces to fill the line.
+        // This bug (GH#5291 won't repro if you don't have the spaces).
+        std::wstring line{ L"~" };
+        line += std::wstring(79, L' ');
+        hostSm.ProcessString(line);
+    }
+
+    // Print the "Status Line"
+    hostSm.ProcessString(L"\x1b[32;1H");
+    hostSm.ProcessString(L"\x1b[m");
+    hostSm.ProcessString(L"AAAAA");
+
+    Log::Comment(L"========== Checking the host buffer state ==========");
+    verifyBuffer(altTextBuffer, altBuffer.GetViewport().ToInclusive());
+
+    Log::Comment(L"Painting the frame");
+    VERIFY_SUCCEEDED(renderer.PaintFrame());
+
+    Log::Comment(L"========== Checking the terminal buffer state ==========");
+
+    verifyBuffer(termTb, term->_mutableViewport.ToInclusive());
+}

From 54942c3e00106f9d539dcc0d6c5d3c77b3ba5fc9 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Thu, 9 Apr 2020 09:43:35 -0500
Subject: [PATCH 02/10] Well touching conhost is always terrifying

---
 .../ConptyRoundtripTests.cpp                   |  1 +
 src/host/screenInfo.cpp                        | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 345351c5b26..f45b3bfe3ea 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2217,6 +2217,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 
         for (auto y = viewport.top<short>(); y < lastRow; y++)
         {
+            VERIFY_IS_FALSE(tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
             TestUtils::VerifyExpectedString(tb, L"~    ", til::point{ 0, y });
         }
 
diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp
index b893c1962cd..6c9ba3c9cea 100644
--- a/src/host/screenInfo.cpp
+++ b/src/host/screenInfo.cpp
@@ -1663,6 +1663,24 @@ void SCREEN_INFORMATION::SetCursorDBMode(const bool DoubleCursor)
         return STATUS_INVALID_PARAMETER;
     }
 
+    // GH#5291 - If we're moving the cursor, we're definitely manually breaking
+    // the line. Check if the cursor is currently in the "delayed EOL wrap"
+    // state. If it is, then the current cursor line is being treated as
+    // wrapped, and the next char should go on the subsequent line.
+    //
+    // However, If we're moving the cursor manually, then we're certainly not
+    // wrapping the current line. In that case, clear the wrap flag from the
+    // current cursor position.
+    const COORD oldCursorPos = cursor.GetPosition();
+    if (cursor.IsDelayedEOLWrap())
+    {
+        ROW& prevRow = _textBuffer->GetRowByOffset(oldCursorPos.Y);
+        if (prevRow.GetCharRow().WasWrapForced())
+        {
+            prevRow.GetCharRow().SetWrapForced(false);
+        }
+    }
+
     cursor.SetPosition(Position);
 
     // if we have the focus, adjust the cursor state

From 12018e40c58195f51845e9ff16b840fb5be34597 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Thu, 9 Apr 2020 14:40:06 -0500
Subject: [PATCH 03/10] Bet you could spae this coming

---
 .../spell-check/dictionary/dictionary.txt        | 16 ----------------
 .../ConptyRoundtripTests.cpp                     |  2 +-
 2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/.github/actions/spell-check/dictionary/dictionary.txt b/.github/actions/spell-check/dictionary/dictionary.txt
index 10d5738d482..ca528f2f7df 100644
--- a/.github/actions/spell-check/dictionary/dictionary.txt
+++ b/.github/actions/spell-check/dictionary/dictionary.txt
@@ -384723,23 +384723,7 @@ spadonic
 spadonism
 spadrone
 spadroon
-spae
-spaebook
-spaecraft
-spaed
-spaedom
-spaeing
-spaeings
-spae-man
-spaeman
-spaer
-Spaerobee
-spaes
 spaetzle
-spaewife
-spaewoman
-spaework
-spaewright
 SPAG
 spag
 spagetti
diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index f45b3bfe3ea..3d8bec4ddab 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2248,7 +2248,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
     hostSm.ProcessString(L"\x1b[H");
     hostSm.ProcessString(L"\x1b[2J");
 
-    // Write out lines of '~' followed by enough spaes to fill the line.
+    // Write out lines of '~' followed by enough spaces to fill the line.
     hostSm.ProcessString(L"\x1b[94m");
     for (auto y = 0; y < altBuffer.GetViewport().BottomInclusive(); y++)
     {

From 238e61d16dc6c160fecd550a46a744563031eb0a Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Fri, 10 Apr 2020 10:30:03 -0500
Subject: [PATCH 04/10] Add a \r\n case to this test as well, why not

---
 .../ConptyRoundtripTests.cpp                  | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 3d8bec4ddab..7e4213a38f1 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2191,6 +2191,14 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer()
 
 void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 {
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1}")
+    END_TEST_METHOD_PROPERTIES();
+    constexpr int MoveCursorWithCUP = 0;
+    constexpr int MoveCursorWithNewline = 1;
+
+    INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP or newline/carriage-return");
+
     Log::Comment(L"This is a test for GH#5291. WSL vim uses spaces to clear the"
                  L" ends of blank lines, not EL. This test ensures that conhost"
                  L" properly treats those lines as _not actually wrapped_,"
@@ -2252,6 +2260,8 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
     hostSm.ProcessString(L"\x1b[94m");
     for (auto y = 0; y < altBuffer.GetViewport().BottomInclusive(); y++)
     {
+        // Vim uses CUP to position the cursor on the first cell of each row, every row.
+        if (cursorMovementMode == MoveCursorWithCUP)
         {
             std::wstringstream ss;
             ss << L"\x1b[";
@@ -2259,6 +2269,15 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
             ss << L";1H";
             hostSm.ProcessString(ss.str());
         }
+        // As an additional test, try breaking lines manually with \r\n
+        else if (cursorMovementMode == MoveCursorWithNewline)
+        {
+            // Don't need to newline on the 0'th row
+            if (y > 0)
+            {
+                hostSm.ProcessString(L"\r\n");
+            }
+        }
 
         // IMPORTANT! The way vim writes these blank lines is as '~' followed by
         // enough spaces to fill the line.

From 6ea74d6f35070bc0e868fede3b458cd39d1acb62 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Fri, 10 Apr 2020 11:07:54 -0500
Subject: [PATCH 05/10] Hey lets do it this way too, just to make sure

---
 .../ConptyRoundtripTests.cpp                     | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 7e4213a38f1..29eeab10346 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2192,10 +2192,11 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer()
 void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 {
     BEGIN_TEST_METHOD_PROPERTIES()
-        TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1}")
+        TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1, 2}")
     END_TEST_METHOD_PROPERTIES();
     constexpr int MoveCursorWithCUP = 0;
-    constexpr int MoveCursorWithNewline = 1;
+    constexpr int MoveCursorWithCRNL = 1;
+    constexpr int MoveCursorWithNLCR = 2;
 
     INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP or newline/carriage-return");
 
@@ -2270,7 +2271,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
             hostSm.ProcessString(ss.str());
         }
         // As an additional test, try breaking lines manually with \r\n
-        else if (cursorMovementMode == MoveCursorWithNewline)
+        else if (cursorMovementMode == MoveCursorWithCRNL)
         {
             // Don't need to newline on the 0'th row
             if (y > 0)
@@ -2278,6 +2279,15 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
                 hostSm.ProcessString(L"\r\n");
             }
         }
+        // As an additional test, try breaking lines manually with \n\r
+        else if (cursorMovementMode == MoveCursorWithNLCR)
+        {
+            // Don't need to newline on the 0'th row
+            if (y > 0)
+            {
+                hostSm.ProcessString(L"\n\r");
+            }
+        }
 
         // IMPORTANT! The way vim writes these blank lines is as '~' followed by
         // enough spaces to fill the line.

From 297fceabe1291c3808d545550ad6a15c6ada9eb7 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Fri, 10 Apr 2020 11:23:12 -0500
Subject: [PATCH 06/10] This is a more acceptable abbreviation

---
 .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp       | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 29eeab10346..29d41c8cb28 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2195,8 +2195,8 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
         TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1, 2}")
     END_TEST_METHOD_PROPERTIES();
     constexpr int MoveCursorWithCUP = 0;
-    constexpr int MoveCursorWithCRNL = 1;
-    constexpr int MoveCursorWithNLCR = 2;
+    constexpr int MoveCursorWithCRLF = 1;
+    constexpr int MoveCursorWithLFCR = 2;
 
     INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP or newline/carriage-return");
 
@@ -2271,7 +2271,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
             hostSm.ProcessString(ss.str());
         }
         // As an additional test, try breaking lines manually with \r\n
-        else if (cursorMovementMode == MoveCursorWithCRNL)
+        else if (cursorMovementMode == MoveCursorWithCRLF)
         {
             // Don't need to newline on the 0'th row
             if (y > 0)
@@ -2280,7 +2280,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
             }
         }
         // As an additional test, try breaking lines manually with \n\r
-        else if (cursorMovementMode == MoveCursorWithNLCR)
+        else if (cursorMovementMode == MoveCursorWithLFCR)
         {
             // Don't need to newline on the 0'th row
             if (y > 0)

From cb013988a30eb5eca7892de2af24e98e37c57d57 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Fri, 10 Apr 2020 11:56:50 -0500
Subject: [PATCH 07/10] more test cases for dustin

---
 .../ConptyRoundtripTests.cpp                  | 46 ++++++++++++++++---
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 29d41c8cb28..0f54b6591bd 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2192,13 +2192,15 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer()
 void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 {
     BEGIN_TEST_METHOD_PROPERTIES()
-        TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1, 2}")
+        TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1, 2, 3, 4, 5}")
     END_TEST_METHOD_PROPERTIES();
     constexpr int MoveCursorWithCUP = 0;
-    constexpr int MoveCursorWithCRLF = 1;
-    constexpr int MoveCursorWithLFCR = 2;
-
-    INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP or newline/carriage-return");
+    constexpr int MoveCursorWithCR_LF = 1;
+    constexpr int MoveCursorWithLF_CR = 2;
+    constexpr int MoveCursorWithVPR_CR = 3;
+    constexpr int MoveCursorWithCUB_NL = 4;
+    constexpr int MoveCursorWithCUD_CR = 5;
+    INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP, newline/carriage-return, or some other VT sequence");
 
     Log::Comment(L"This is a test for GH#5291. WSL vim uses spaces to clear the"
                  L" ends of blank lines, not EL. This test ensures that conhost"
@@ -2271,7 +2273,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
             hostSm.ProcessString(ss.str());
         }
         // As an additional test, try breaking lines manually with \r\n
-        else if (cursorMovementMode == MoveCursorWithCRLF)
+        else if (cursorMovementMode == MoveCursorWithCR_LF)
         {
             // Don't need to newline on the 0'th row
             if (y > 0)
@@ -2280,7 +2282,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
             }
         }
         // As an additional test, try breaking lines manually with \n\r
-        else if (cursorMovementMode == MoveCursorWithLFCR)
+        else if (cursorMovementMode == MoveCursorWithLF_CR)
         {
             // Don't need to newline on the 0'th row
             if (y > 0)
@@ -2288,6 +2290,36 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
                 hostSm.ProcessString(L"\n\r");
             }
         }
+        // As an additional test, move the cursor down with VPR, then to the start of the line with CR
+        else if (cursorMovementMode == MoveCursorWithVPR_CR)
+        {
+            // Don't need to newline on the 0'th row
+            if (y > 0)
+            {
+                hostSm.ProcessString(L"\x1b[1e");
+                hostSm.ProcessString(L"\r");
+            }
+        }
+        // As an additional test, move the cursor back with CUB, then down with LF
+        else if (cursorMovementMode == MoveCursorWithCUB_NL)
+        {
+            // Don't need to newline on the 0'th row
+            if (y > 0)
+            {
+                hostSm.ProcessString(L"\x1b[80D");
+                hostSm.ProcessString(L"\n");
+            }
+        }
+        // As an additional test, move the cursor down with CUD, then to the start of the line with CR
+        else if (cursorMovementMode == MoveCursorWithCUD_CR)
+        {
+            // Don't need to newline on the 0'th row
+            if (y > 0)
+            {
+                hostSm.ProcessString(L"\x1b[B");
+                hostSm.ProcessString(L"\r");
+            }
+        }
 
         // IMPORTANT! The way vim writes these blank lines is as '~' followed by
         // enough spaces to fill the line.

From 1a9fa14de7c30e78ace05df409e8ea3f24ed3c57 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Tue, 14 Apr 2020 08:55:53 -0500
Subject: [PATCH 08/10] I guess I didn't have to touch the _host_ bits of
 conhost at all

---
 .../ConptyRoundtripTests.cpp                  | 16 +++++++--
 src/host/screenInfo.cpp                       | 34 +++++++++----------
 src/renderer/vt/paint.cpp                     |  6 ++--
 src/winconpty/winconpty.cpp                   |  2 +-
 4 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 0f54b6591bd..2235995e614 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2219,8 +2219,12 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 
     _flushFirstFrame();
 
-    auto verifyBuffer = [](const TextBuffer& tb,
-                           const til::rectangle viewport) {
+    const bool expectHardBreak = (cursorMovementMode == MoveCursorWithLF_CR) ||
+                                 (cursorMovementMode == MoveCursorWithCR_LF) ||
+                                 (cursorMovementMode == MoveCursorWithCUB_NL);
+
+    auto verifyBuffer = [&](const TextBuffer& tb,
+                            const til::rectangle viewport) {
         const auto lastRow = viewport.bottom<short>() - 1;
         const til::point expectedCursor{ 5, lastRow };
         VERIFY_ARE_EQUAL(expectedCursor, til::point{ tb.GetCursor().GetPosition() });
@@ -2228,7 +2232,13 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 
         for (auto y = viewport.top<short>(); y < lastRow; y++)
         {
-            VERIFY_IS_FALSE(tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
+            // VERIFY_IS_FALSE(tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
+            const auto rowWrapped = (!expectHardBreak) || (y == lastRow - 1);
+            Log::Comment(NoThrowString().Format(
+                L"y, lastRow, expectHardBreak, rowWrapped=%d, %d, %d, %d", y, lastRow, expectHardBreak, rowWrapped));
+
+            // VERIFY_ARE_EQUAL(!expectHardBreak, tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
+            VERIFY_ARE_EQUAL(rowWrapped, tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
             TestUtils::VerifyExpectedString(tb, L"~    ", til::point{ 0, y });
         }
 
diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp
index 6c9ba3c9cea..8b2fed884f8 100644
--- a/src/host/screenInfo.cpp
+++ b/src/host/screenInfo.cpp
@@ -1663,23 +1663,23 @@ void SCREEN_INFORMATION::SetCursorDBMode(const bool DoubleCursor)
         return STATUS_INVALID_PARAMETER;
     }
 
-    // GH#5291 - If we're moving the cursor, we're definitely manually breaking
-    // the line. Check if the cursor is currently in the "delayed EOL wrap"
-    // state. If it is, then the current cursor line is being treated as
-    // wrapped, and the next char should go on the subsequent line.
-    //
-    // However, If we're moving the cursor manually, then we're certainly not
-    // wrapping the current line. In that case, clear the wrap flag from the
-    // current cursor position.
-    const COORD oldCursorPos = cursor.GetPosition();
-    if (cursor.IsDelayedEOLWrap())
-    {
-        ROW& prevRow = _textBuffer->GetRowByOffset(oldCursorPos.Y);
-        if (prevRow.GetCharRow().WasWrapForced())
-        {
-            prevRow.GetCharRow().SetWrapForced(false);
-        }
-    }
+    // // GH#5291 - If we're moving the cursor, we're definitely manually breaking
+    // // the line. Check if the cursor is currently in the "delayed EOL wrap"
+    // // state. If it is, then the current cursor line is being treated as
+    // // wrapped, and the next char should go on the subsequent line.
+    // //
+    // // However, If we're moving the cursor manually, then we're certainly not
+    // // wrapping the current line. In that case, clear the wrap flag from the
+    // // current cursor position.
+    // const COORD oldCursorPos = cursor.GetPosition();
+    // if (cursor.IsDelayedEOLWrap())
+    // {
+    //     ROW& prevRow = _textBuffer->GetRowByOffset(oldCursorPos.Y);
+    //     if (prevRow.GetCharRow().WasWrapForced())
+    //     {
+    //         prevRow.GetCharRow().SetWrapForced(false);
+    //     }
+    // }
 
     cursor.SetPosition(Position);
 
diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp
index 8ece3076288..8aca463664f 100644
--- a/src/renderer/vt/paint.cpp
+++ b/src/renderer/vt/paint.cpp
@@ -449,9 +449,9 @@ using namespace Microsoft::Console::Types;
     //
     // GH#5161: Only removeSpaces when we're in the _newBottomLine state and the
     // line we're trying to print right now _actually is the bottom line_
-    const bool removeSpaces = useEraseChar ||
-                              _clearedAllThisFrame ||
-                              (_newBottomLine && coord.Y == _lastViewport.BottomInclusive());
+    const bool removeSpaces = !lineWrapped && (useEraseChar ||
+                                               _clearedAllThisFrame ||
+                                               (_newBottomLine && coord.Y == _lastViewport.BottomInclusive()));
     const size_t cchActual = removeSpaces ?
                                  (cchLine - numSpaces) :
                                  cchLine;
diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp
index ed250b55a6e..53952574cc8 100644
--- a/src/winconpty/winconpty.cpp
+++ b/src/winconpty/winconpty.cpp
@@ -83,7 +83,7 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken,
     RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(signalPipeConhostSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT));
 
     // GH4061: Ensure that the path to executable in the format is escaped so C:\Program.exe cannot collide with C:\Program Files
-    const wchar_t* pwszFormat = L"\"%s\" --headless %s%s--width %hu --height %hu --signal 0x%x --server 0x%x";
+    const wchar_t* pwszFormat = L"\"%s\" %s%s--width %hu --height %hu --signal 0x%x --server 0x%x";
     // This is plenty of space to hold the formatted string
     wchar_t cmd[MAX_PATH]{};
     const BOOL bInheritCursor = (dwFlags & PSEUDOCONSOLE_INHERIT_CURSOR) == PSEUDOCONSOLE_INHERIT_CURSOR;

From cebdb8ffad1b3c61c80a1bc8a3e8ff0754fe198e Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Tue, 14 Apr 2020 09:23:12 -0500
Subject: [PATCH 09/10] Cleanup and format code for push.

---
 .../ConptyRoundtripTests.cpp                  | 25 +++++++++-------
 src/host/screenInfo.cpp                       | 29 ++++++++-----------
 src/renderer/vt/paint.cpp                     | 14 +++++++--
 src/winconpty/winconpty.cpp                   |  2 +-
 4 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 2235995e614..63c14780ca7 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2192,7 +2192,7 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer()
 void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 {
     BEGIN_TEST_METHOD_PROPERTIES()
-        TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1, 2, 3, 4, 5}")
+        TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1, 2, 3, 4, 5, 6}")
     END_TEST_METHOD_PROPERTIES();
     constexpr int MoveCursorWithCUP = 0;
     constexpr int MoveCursorWithCR_LF = 1;
@@ -2200,13 +2200,13 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
     constexpr int MoveCursorWithVPR_CR = 3;
     constexpr int MoveCursorWithCUB_NL = 4;
     constexpr int MoveCursorWithCUD_CR = 5;
+    constexpr int MoveCursorWithNothing = 6;
     INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP, newline/carriage-return, or some other VT sequence");
 
     Log::Comment(L"This is a test for GH#5291. WSL vim uses spaces to clear the"
-                 L" ends of blank lines, not EL. This test ensures that conhost"
-                 L" properly treats those lines as _not actually wrapped_,"
-                 L" because if we leave them \"wrapped\", conpty won't newline"
-                 L" in between them.");
+                 L" ends of blank lines, not EL. This test ensures we emit text"
+                 L" from conpty such that the terminal re-creates the state of"
+                 L" the host, which includes wrapped lines of lots of spaces.");
     VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());
 
     auto& g = ServiceLocator::LocateGlobals();
@@ -2219,6 +2219,8 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 
     _flushFirstFrame();
 
+    // Any of the cursor movements that use a LF will actaully hard break the
+    // line - everything else will leave it marked as wrapped.
     const bool expectHardBreak = (cursorMovementMode == MoveCursorWithLF_CR) ||
                                  (cursorMovementMode == MoveCursorWithCR_LF) ||
                                  (cursorMovementMode == MoveCursorWithCUB_NL);
@@ -2232,12 +2234,9 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 
         for (auto y = viewport.top<short>(); y < lastRow; y++)
         {
-            // VERIFY_IS_FALSE(tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
+            // We're using CUP to move onto the status line _always_, so the
+            // second-last row will always be marked as wrapped.
             const auto rowWrapped = (!expectHardBreak) || (y == lastRow - 1);
-            Log::Comment(NoThrowString().Format(
-                L"y, lastRow, expectHardBreak, rowWrapped=%d, %d, %d, %d", y, lastRow, expectHardBreak, rowWrapped));
-
-            // VERIFY_ARE_EQUAL(!expectHardBreak, tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
             VERIFY_ARE_EQUAL(rowWrapped, tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
             TestUtils::VerifyExpectedString(tb, L"~    ", til::point{ 0, y });
         }
@@ -2330,6 +2329,12 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
                 hostSm.ProcessString(L"\r");
             }
         }
+        // Win32 vim.exe will simply do _nothing_ in this scenario. It'll just
+        // print the lines one after the other, without moving the cursor,
+        // relying on us auto moving to the following line.
+        else if (cursorMovementMode == MoveCursorWithNothing)
+        {
+        }
 
         // IMPORTANT! The way vim writes these blank lines is as '~' followed by
         // enough spaces to fill the line.
diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp
index 8b2fed884f8..78e7650cfb3 100644
--- a/src/host/screenInfo.cpp
+++ b/src/host/screenInfo.cpp
@@ -1663,23 +1663,18 @@ void SCREEN_INFORMATION::SetCursorDBMode(const bool DoubleCursor)
         return STATUS_INVALID_PARAMETER;
     }
 
-    // // GH#5291 - If we're moving the cursor, we're definitely manually breaking
-    // // the line. Check if the cursor is currently in the "delayed EOL wrap"
-    // // state. If it is, then the current cursor line is being treated as
-    // // wrapped, and the next char should go on the subsequent line.
-    // //
-    // // However, If we're moving the cursor manually, then we're certainly not
-    // // wrapping the current line. In that case, clear the wrap flag from the
-    // // current cursor position.
-    // const COORD oldCursorPos = cursor.GetPosition();
-    // if (cursor.IsDelayedEOLWrap())
-    // {
-    //     ROW& prevRow = _textBuffer->GetRowByOffset(oldCursorPos.Y);
-    //     if (prevRow.GetCharRow().WasWrapForced())
-    //     {
-    //         prevRow.GetCharRow().SetWrapForced(false);
-    //     }
-    // }
+    // In GH#5291, we experimented with manually breaking the line on all cursor
+    // movements here. As we print lines into the buffer, we mark lines as
+    // wrapped when we print the last cell of the row, not the first cell of the
+    // subsequent row (the row the first line wrapped onto).
+    //
+    // Logically, we thought that manaully breaking lines when we move the
+    // cursor was a good idea. We however, did not have the time to fully
+    // validate that this was the correct answer, and a simpler solution for the
+    // bug on hand was found. Furthermore, we thought it would be a more
+    // comprehensive solution to only mark lines as wrapped when we print the
+    // first cell of the second row, which would require some WriteCharsLegacy
+    // work.
 
     cursor.SetPosition(Position);
 
diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp
index 8aca463664f..dd704a582bf 100644
--- a/src/renderer/vt/paint.cpp
+++ b/src/renderer/vt/paint.cpp
@@ -443,15 +443,23 @@ using namespace Microsoft::Console::Types;
     const bool useEraseChar = (optimalToUseECH) &&
                               (!_newBottomLine) &&
                               (!_clearedAllThisFrame);
+    const bool printingBottomLine = coord.Y == _lastViewport.BottomInclusive();
 
     // If we're not using erase char, but we did erase all at the start of the
     // frame, don't add spaces at the end.
     //
     // GH#5161: Only removeSpaces when we're in the _newBottomLine state and the
     // line we're trying to print right now _actually is the bottom line_
+    //
+    // GH#5291: DON'T remove spaces when the row wrapped. We might need those
+    // spaces to preserve the wrap state of this line, or the cursor position.
+    // For example, vim.exe uses "~    "... to clear the line, and then leaves
+    // the lines _wrapped_. It doesn't care to manually break the lines, but if
+    // we trimmed the spaces off here, we'd print all the "~"s one after another
+    // on the same line.
     const bool removeSpaces = !lineWrapped && (useEraseChar ||
                                                _clearedAllThisFrame ||
-                                               (_newBottomLine && coord.Y == _lastViewport.BottomInclusive()));
+                                               (_newBottomLine && printingBottomLine));
     const size_t cchActual = removeSpaces ?
                                  (cchLine - numSpaces) :
                                  cchLine;
@@ -547,7 +555,7 @@ using namespace Microsoft::Console::Types;
             RETURN_IF_FAILED(_EraseLine());
         }
     }
-    else if (_newBottomLine && coord.Y == _lastViewport.BottomInclusive())
+    else if (_newBottomLine && printingBottomLine)
     {
         // If we're on a new line, then we don't need to erase the line. The
         //      line is already empty.
@@ -566,7 +574,7 @@ using namespace Microsoft::Console::Types;
 
     // If we printed to the bottom line, and we previously thought that this was
     // a new bottom line, it certainly isn't new any longer.
-    if (coord.Y == _lastViewport.BottomInclusive())
+    if (printingBottomLine)
     {
         _newBottomLine = false;
     }
diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp
index 53952574cc8..ed250b55a6e 100644
--- a/src/winconpty/winconpty.cpp
+++ b/src/winconpty/winconpty.cpp
@@ -83,7 +83,7 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken,
     RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(signalPipeConhostSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT));
 
     // GH4061: Ensure that the path to executable in the format is escaped so C:\Program.exe cannot collide with C:\Program Files
-    const wchar_t* pwszFormat = L"\"%s\" %s%s--width %hu --height %hu --signal 0x%x --server 0x%x";
+    const wchar_t* pwszFormat = L"\"%s\" --headless %s%s--width %hu --height %hu --signal 0x%x --server 0x%x";
     // This is plenty of space to hold the formatted string
     wchar_t cmd[MAX_PATH]{};
     const BOOL bInheritCursor = (dwFlags & PSEUDOCONSOLE_INHERIT_CURSOR) == PSEUDOCONSOLE_INHERIT_CURSOR;

From 43ea3ce41ea9b7694b27d5c2a671222fe30f9622 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Tue, 14 Apr 2020 10:11:00 -0500
Subject: [PATCH 10/10] typos

---
 .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp       | 8 ++++----
 src/host/screenInfo.cpp                                   | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 63c14780ca7..be39bd199b8 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2198,7 +2198,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
     constexpr int MoveCursorWithCR_LF = 1;
     constexpr int MoveCursorWithLF_CR = 2;
     constexpr int MoveCursorWithVPR_CR = 3;
-    constexpr int MoveCursorWithCUB_NL = 4;
+    constexpr int MoveCursorWithCUB_LF = 4;
     constexpr int MoveCursorWithCUD_CR = 5;
     constexpr int MoveCursorWithNothing = 6;
     INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP, newline/carriage-return, or some other VT sequence");
@@ -2219,11 +2219,11 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
 
     _flushFirstFrame();
 
-    // Any of the cursor movements that use a LF will actaully hard break the
+    // Any of the cursor movements that use a LF will actually hard break the
     // line - everything else will leave it marked as wrapped.
     const bool expectHardBreak = (cursorMovementMode == MoveCursorWithLF_CR) ||
                                  (cursorMovementMode == MoveCursorWithCR_LF) ||
-                                 (cursorMovementMode == MoveCursorWithCUB_NL);
+                                 (cursorMovementMode == MoveCursorWithCUB_LF);
 
     auto verifyBuffer = [&](const TextBuffer& tb,
                             const til::rectangle viewport) {
@@ -2310,7 +2310,7 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()
             }
         }
         // As an additional test, move the cursor back with CUB, then down with LF
-        else if (cursorMovementMode == MoveCursorWithCUB_NL)
+        else if (cursorMovementMode == MoveCursorWithCUB_LF)
         {
             // Don't need to newline on the 0'th row
             if (y > 0)
diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp
index 78e7650cfb3..c9348270cf2 100644
--- a/src/host/screenInfo.cpp
+++ b/src/host/screenInfo.cpp
@@ -1668,7 +1668,7 @@ void SCREEN_INFORMATION::SetCursorDBMode(const bool DoubleCursor)
     // wrapped when we print the last cell of the row, not the first cell of the
     // subsequent row (the row the first line wrapped onto).
     //
-    // Logically, we thought that manaully breaking lines when we move the
+    // Logically, we thought that manually breaking lines when we move the
     // cursor was a good idea. We however, did not have the time to fully
     // validate that this was the correct answer, and a simpler solution for the
     // bug on hand was found. Furthermore, we thought it would be a more