@iSazonov someone should look at why code factor is not respecting the rules set by PowerShell. You can see the failures from missing this. at https://www.codefactor.io/repository/github/powershell/powershell/pull/24711. There are other failures but now I have no idea if they are ok to ignore or whether they are actual changes to conform to the pwsh repo. I'm not going to spend time fixing things that turn out aren't rules pwsh cares about.

This problem has existed all the way back in September, see 6736622 for a change I made to satisfy code factor and this. I never had to do it before but if CI is failing then either we need to conform to the new rule or it should be fixed so it respects the pwsh rules.

Fix Start-Process -Wait polling - #24711 - PowerShell

Quality software, faster.

@jborean93 I filtered out two noise issues on CodeFactor site. It seems now they takes into account only .editorconfig file. At least their documentation points only to this file. I will open a new issue here so that someone migrates the Settings.StyleCop rules to this file.

Also Code Factor check is optional. Usually maintainers ask contributors to fix some CodeFactor issues. All mandatory rules are in .globalconfig file and violations of the rules will break the build forcing contributor to fix the code.

Also Code Factor check is optional. Usually maintainers ask contributors to fix some CodeFactor issues. All mandatory rules are in .globalconfig file and violations of the rules will break the build forcing contributor to fix the code.

Thanks for looking into that. If this is optional then really the check should be removed. While this isn't probably the best place to talk about it having what looks to be a failing CI check can be annoying for contributors trying to get their PR in a ready for review state. An optional check is ultimately not a check at all and is extra noise and work for everyone involved. It should either be enforced or just removed altogether.

As for the other comments, I'll push a commit too add the safe handles back in. I've replied to some of the other review comments you've made though as to why I think they shouldn't change. Appreciate you looking through this.

@jborean93 Before we continue the code review in details I want to share some conceptual thoughts.

If this is optional then really the check should be removed.

We can not remove CodeFactor because it does useful work for us. For example, look issues for XML documentation comments. Historically pwsh has thousands of empty XML doc comments. If we make CodeFactor mandatory we block whole project since it is not really fix all such issues. We can not disable such issues because we want contributors add XML doc comments in new code. So CodeFactor as "optional" check is a compromise. Our workflow is quite gentle - if the maintainer sees significant CodeFactor notices, it will ask contributor to fix them to follow the repository rules.
Also issues the CodeFactor reports comes from StyleCop analyzer. The analyzer works locally for you too. Main problem is CodeFactor has false positives from time to time. We can't influence it. So this is another argument to make it optional.
I hope this information will help make your work here a little more comfortable.

Looking on a piece of the method we change here, it is conceptually as simple as:

• create Process object
• write it in pipeline if needed
• wait it if needed

But the real code, due to the previously added workarounds, has become more confusing, and with the current fixes, the situation has become even worse.
We have already fixed this code more than once, and we can assume that this will continue.
In this case, we would like to bring the code to a state more convenient for maintenance.
Now, in addition to the problems already mentioned, there are other amazing things.
For example, a JobCollection. Why is this a collection? Obviously, it was designed for use in pipeline, but we only use it only in BeginProcessing for years. It is worth not only renaming this type but also simplifying it.

Another amazing thing is the comment:

Next amazing thing is:

Perhaps I didn't see all the problems when I skimmed through this method.

And I wouldn't write all this if you had already fixed this code more than once and probably would again. So I would welcome converting this code into something cleaner and more maintainable.
At first glance, it would be possible to create a ProcessWating wrapper for Process with different implementations for Windows and Unix. This would make the main code very simple. The wrapper could be just a disposable struct.

and with the current fixes, the situation has become even worse.

While yes the current code is a bit of a mess I'm not sure I agree that this makes it worse. This change

• Moves out a nested class
• Moves out some PInvoke definitions, even some that was defined in another area for some reason
• Simplifies some nullability checks and object initialisation
• Removes some null condition checks around the wait handle to the more common cancellation token setup

Yes some work should be done to tidy the code up and probably unpick the attempts to share some of the code flow into their own specific paths but I don't think it should be done in the same PR. I'm happy to open another PR to try and clean some more stuff up later on though.

We have already fixed this code more than once, and we can assume that this will continue.

We've fixed similar code but not this issue, this specific issue around jobs polling every 1 second has always been around.

For some reason, this comment is located after the code it refers to.

The comment is directly for the code just below it

if (Wait)
{
    jobObject = new();
    jobAssigned = jobObject.AssignProcessToJobObject(processInfo.Process);
}

It is referring to the fact that AssignProcessToJobObject might fail unless we have the ALL_ACCESS token from creating the process. In the past we did

Process process = Process.GetProcessById(processInfo.ProcessId);
jobObject.AssignProcessToJobObject(process.SafeHandle);

This was problematic because the handle returned by process.SafeHandle may not have enough permissions in the case when we started the process with custom credentials.

The process is created by constructor (throw on error) and cannot be null. So no need to check this and write terminating error (after if (Wait.IsPresent) we do the same too).

I've not looked at this code and think it's unrelated to the problem at hand.

And I wouldn't write all this if you had already fixed this code more than once and probably would again. So I would welcome converting this code into something cleaner and more maintainable.

I certainly agree that this should be fixed but I don't think it should be rewritten at the same time as fixing an unrelated problem. Refactoring should only happen when you want to cleanup the code, not do bugfixes at the same time. Doing both at the same time is makes it harder for reviews as it has to deal with both behaviour changes and just general code changes. Doing them in smaller steps makes it easier on both the person implementing the change and the people who review the code.

/azp run

Please don't rebase without maintainer ask. Rebase kill commit history and resets previous reviews.

CI says that the branch is out of date and I rebase to bring it back up to date. Using a merge pull creates merge commits and while yes a rebase requires a force push it still preserves the commit history for the PR. If you have a better suggestion then please share, personally I'm getting sick of PowerShell's CI that marks PRs as red or not ready when some things seem to be optional.

I still see the reviews, I marked them as resolved because I responded to the latest comments on there, sorry if I missed anything.

@jborean93 MSFT team use a tool for reviewing commit by commit and they ask us to keep commit history. GitHub Web GUI also doesn't like rebase - it can hide some comments (and it is very tricky to find them).
Of course, there are situations (very rarely) when we are forced to do a rebase, but at the same time we must protect the time spent by others. So it's better to coordinate rebase.

"Update branch with merge commit" button is not useful for us since we always do "Rebase and Merge" finally. I have not permission to remove the option. "Update branch with rebase" could be useful for us (mainly maintainers) if we need to rebase for running CIs before merge of very old PR.

📣 Hey @jborean93, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms

Thanks for the review and merge.

@jborean93 Thanks for great work! I hope you will find the time to clean up the file(s) so that it will be easier for us to do the subsequent work.