Fix redundant iteration while splitting lines #14851

hez2010 · 2021-02-19T06:07:57Z

PR Summary

Fix redundant iteration while splitting lines to prevent assert failure.

Reopened PR #14720 from different folk.

PR Context

We split a line to fit in a windows width and if on last iteration we did get zero length tail we did next iteration but we should stop.

Borrowed from @iSazonov, see #13551 (comment)

Fixes #13551

PR Checklist

iSazonov · 2021-02-19T13:30:03Z

@hez2010 Please try to rebase from your branch

get pull --rebase PowerShell master

hez2010 · 2021-02-19T13:50:13Z

@iSazonov Done

ghost · 2021-02-27T02:00:08Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

hez2010 · 2021-02-28T07:06:36Z

Any updates on this PR?

iSazonov · 2021-02-28T15:44:04Z

@hez2010 Please rebase to fix CI builds.

We split a line to fit in a windows width and if on last iteration we did get zero length tail we did next iteration but we should stop.

hez2010 · 2021-03-01T05:39:42Z

@iSazonov Done. I think CI failures are unrelated to this PR.

iSazonov · 2021-03-01T06:17:06Z

CI fails on https://github.com/dotnet/cli - the repo was move to https://github.com/dotnet/sdk.
There are 3 uses of the link in our md files.

GitHub
dotnet/cli
The .NET Core command-line (CLI) tools, used for building .NET Core apps and libraries through your development flow (compiling, NuGet package management, running, testing, ...). - dotnet/cli

GitHub
dotnet/sdk
Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI - dotnet/sdk

src/System.Management.Automation/FormatAndOutput/common/ComplexWriter.cs

Co-authored-by: Robert Holt <rjmholt@gmail.com>

hez2010 · 2021-03-10T05:34:22Z

Ping! Can this PR be merged? This issue blocks me to use PowerShell in some scenarios because PowerShell will crash without this PR.

iSazonov · 2021-03-10T05:38:40Z

/azp run PowerShell-CI-static-analysis

azure-pipelines · 2021-03-10T05:38:51Z

Azure Pipelines successfully started running 1 pipeline(s).

iSazonov · 2021-03-10T05:45:16Z

@rjmholt I believe we need to fix internal override int Length(string str, int offset) too (see my commit iSazonov@6a9bf10) Otherwise the PR is not full fix for #13551.

hez2010 · 2021-03-15T15:49:56Z

@iSazonov I've added the fix back.

xtqqczze · 2021-03-16T23:44:15Z

src/System.Management.Automation/FormatAndOutput/out-console/ConsoleLineOutput.cs

@@ -87,8 +87,15 @@ internal DisplayCellsPSHost(PSHostRawUserInterface rawUserInterface)

        internal override int Length(string str, int offset)
        {
-            Dbg.Assert(offset >= 0, "offset >= 0");
-            Dbg.Assert(string.IsNullOrEmpty(str) || (offset < str.Length), "offset < str.Length");
+            if (string.IsNullOrEmpty(str))


@iSazonov Why are changes to this method necessary to fix the issue? In particular do we need replace assert with exception in this internal method?

The change is not direct fix for the issue. It is related the issue. It makes the code more reliable.
If we don't change the method we will have different behavior for debug and release code that might confuse.

ghost · 2021-03-24T14:00:12Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

hez2010 · 2021-03-26T11:03:47Z

Any updates?

iSazonov · 2021-03-27T06:01:59Z

@rjmholt Do you ready to merge?

ghost · 2021-04-03T14:00:12Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

hez2010 · 2021-04-05T03:35:17Z

Can someone take a look at this PR?

iSazonov · 2021-04-13T04:05:40Z

@hez2010 Thanks for you contribution!

ghost · 2021-04-14T23:59:18Z

🎉v7.2.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

@davidBar-On

[7.2.0-preview.5] - 2021-04-14 * Breaking Changes - Make PowerShell Linux deb and RPM packages universal (#15109) - Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035) - Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!) * Experimental Features - `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692) * Engine Updates and Fixes - Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!) * General Cmdlet Updates and Fixes - Fix SSH remoting connection never finishing with misconfigured endpoint (#15175) - Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969) - Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048) - Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!) - Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!) - Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!) - Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!) - Allow `Set-Clipboard` to accept empty string (#14579) - Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943) - Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077) * Code Cleanup <details> <summary> We thank the following contributors! @xtqqczze, @iSazonov, @ZhiZe-ZG </summary> <ul> <li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li> <li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li> <li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li> <li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li> <li>Remove unnecessary <code>Array</code> -> <code>List</code> -> <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li> <li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li> <li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li> <li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li> </ul> </details> * Tools - Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!) * Tests - Add the missing tag in Host Utilities tests (#14983) - Update `copy-props` version in `package.json` (#15124) * Build and Packaging Improvements <details> <summary> We thank the following contributors! @JustinGrote </summary> <ul> <li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li> <li>Make package validation regex accept universal Linux packages (#15226)</li> <li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li> <li>Make MSI and EXE signing always copy to fix daily build (#15191)</li> <li>Sign internals of EXE package so that it works correctly when signed (#15132)</li> <li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li> <li>Update daily release tag format to work with new Microsoft Update work (#15164)</li> <li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li> <li>Treat rebuild branches like release branches (#15099)</li> <li>Update WiX to 3.11.2 (#15097)</li> <li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li> <li>Allow patching of preview releases (#15074)</li> <li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li> <li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li> <li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li> </ul> </details> * Documentation and Help Content - Merge `7.2.0-preview.4` changes to master (#15056) - Update `README` and `metadata.json` (#15046) - Fix broken links for `dotnet` CLI (#14937) [7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5

ghost assigned rjmholt Feb 19, 2021

rjmholt requested a review from SteveL-MSFT February 19, 2021 18:03

ghost added the Review - Needed The PR is being reviewed label Feb 27, 2021

ghost removed the Review - Needed The PR is being reviewed label Feb 28, 2021

iSazonov and others added 2 commits March 1, 2021 11:54

Fix GenerateLinesWithoutWordWrap()

c102315

We split a line to fit in a windows width and if on last iteration we did get zero length tail we did next iteration but we should stop.

Work with review feedback

0bc0827

rjmholt requested review from daxian-dbw and JamesWTruher March 1, 2021 19:32

xtqqczze approved these changes Mar 2, 2021

View reviewed changes

rjmholt reviewed Mar 2, 2021

View reviewed changes

src/System.Management.Automation/FormatAndOutput/common/ComplexWriter.cs Outdated Show resolved Hide resolved

Apply changes based on review feedback

892491c

Co-authored-by: Robert Holt <rjmholt@gmail.com>

rjmholt approved these changes Mar 3, 2021

View reviewed changes

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 10, 2021

Fix Length(string, int) override implementation

d1f3d7a

xtqqczze reviewed Mar 16, 2021

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label Mar 24, 2021

iSazonov approved these changes Mar 27, 2021

View reviewed changes

ghost removed the Review - Needed The PR is being reviewed label Mar 27, 2021

ghost added the Review - Needed The PR is being reviewed label Apr 3, 2021

rjmholt merged commit 8f5ae72 into PowerShell:master Apr 12, 2021

ghost removed the Review - Needed The PR is being reviewed label Apr 12, 2021

iSazonov added this to the 7.2.0-preview.5 milestone Apr 13, 2021

ghost mentioned this pull request Apr 14, 2021

Bug: PowerShell cannot handle a file with long non-ascii file name #13551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix redundant iteration while splitting lines #14851

Fix redundant iteration while splitting lines #14851

hez2010 commented Feb 19, 2021 •

edited

iSazonov commented Feb 19, 2021

hez2010 commented Feb 19, 2021

ghost commented Feb 27, 2021

hez2010 commented Feb 28, 2021

iSazonov commented Feb 28, 2021

hez2010 commented Mar 1, 2021

iSazonov commented Mar 1, 2021 •

edited by unfurl-links bot

hez2010 commented Mar 10, 2021

iSazonov commented Mar 10, 2021

azure-pipelines bot commented Mar 10, 2021

iSazonov commented Mar 10, 2021

hez2010 commented Mar 15, 2021

xtqqczze Mar 16, 2021

iSazonov Mar 17, 2021

ghost commented Mar 24, 2021

hez2010 commented Mar 26, 2021

iSazonov commented Mar 27, 2021

ghost commented Apr 3, 2021

hez2010 commented Apr 5, 2021

iSazonov commented Apr 13, 2021

ghost commented Apr 14, 2021

Fix redundant iteration while splitting lines #14851

Fix redundant iteration while splitting lines #14851

Conversation

hez2010 commented Feb 19, 2021 • edited

PR Summary

PR Context

PR Checklist

iSazonov commented Feb 19, 2021

hez2010 commented Feb 19, 2021

ghost commented Feb 27, 2021

hez2010 commented Feb 28, 2021

iSazonov commented Feb 28, 2021

hez2010 commented Mar 1, 2021

iSazonov commented Mar 1, 2021 • edited by unfurl-links bot

hez2010 commented Mar 10, 2021

iSazonov commented Mar 10, 2021

azure-pipelines bot commented Mar 10, 2021

iSazonov commented Mar 10, 2021

hez2010 commented Mar 15, 2021

xtqqczze Mar 16, 2021

Choose a reason for hiding this comment

iSazonov Mar 17, 2021

Choose a reason for hiding this comment

ghost commented Mar 24, 2021

hez2010 commented Mar 26, 2021

iSazonov commented Mar 27, 2021

ghost commented Apr 3, 2021

hez2010 commented Apr 5, 2021

iSazonov commented Apr 13, 2021

ghost commented Apr 14, 2021

hez2010 commented Feb 19, 2021 •

edited

iSazonov commented Mar 1, 2021 •

edited by unfurl-links bot