Cleanup handling of AllOutput for processes #10262

gerhardol · 2022-10-15T21:44:25Z

Initiated from #10242
Some parts were initially in #10261 , separated to this PR

Proposed changes

AllOutput is combined use of stderr and stdout and should only be used to display messages for users.

Change a few commands to explicitly read stderr where required. Some tests had to be adapted (including MockExecutable).
Clarify usage that AllOutput is used for user popups. Most of these occurrences are related to FormCommit. The structure of these command was changed to return a bool for result and the output as a string, also if result/output were unused.
Set TODO on a few occurrences that were not investigated.

Test methodology

Tests updated

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

✒️ I contribute this code under The Developer Certificate of Origin.

RussKie

LGTM, though I haven't run this.

RussKie · 2022-10-16T03:58:08Z

GitCommands/Git/GitModule.cs

+        /// <summary>
+        /// Set/unset assumeUnchanged
+        /// </summary>
+        /// <param name="files">Files to change</param>
+        /// <param name="assumeUnchanged">Set/unset assumeUnchanged</param>
+        /// <param name="alloutput">stdout and stderr output</param>
+        /// <returns>true if no errors occurred</returns>


The docs should be in sentence case and end with a full stop. We should also strive to use xml-doc tags, where appropriate.

Suggested change

/// <summary>

/// Set/unset assumeUnchanged

/// </summary>

/// <param name="files">Files to change</param>

/// <param name="assumeUnchanged">Set/unset assumeUnchanged</param>

/// <param name="alloutput">stdout and stderr output</param>

/// <returns>true if no errors occurred</returns>

/// <summary>

/// Set/unset whether given items are assumed unchanged.

/// </summary>

/// <param name="files">The files to set the status for.</param>

/// <param name="assumeUnchanged">The status value.</param>

/// <param name="allOutput">stdout and stderr output.</param>

/// <returns><see langword="true"/> if no errors occurred; otherwise, <see langword="false"/>.</returns>

Ditto below.

Updated.
(Not too happy with the xml-doc, the primary intention is to mark that AllOutput is deliberate, not being parsed.)

RussKie · 2022-10-16T04:00:25Z

I presume we'd want this in v4, right?

vbjay

Not blocking comments but things I noticed.

vbjay · 2022-10-16T03:21:59Z

GitCommands/Git/GitGpgController.cs

@@ -232,13 +232,13 @@ public string GetCommitVerificationMessage(GitRevision revision)
                return null;
            }

-            var module = GetModule();
+            // '--raw' returns info in stderr


Rather it say verify-tag. It's not just raw that uses the error stream.

'git verify-tag --raw' returns the info in stderr, but the command itself is redundant here.
--raw returning in stderr is just an unexpected behavior that I want to document, commands should normally only use stderr for displaying errors.

I moved the comment to the return that accesses stderr instead.

#10261 (comment)

#10261 (comment)

My point is it doesn't matter if it is --raw or not.

https://github.com/git/git/blob/d420dda0576340909c3faff364cfbd1485f70376/gpg-interface.c#L656-L666

See how they output to both streams? It is not an error if error stream has output. That is what the error code provides. I have seen a few programs do this where they output a your summary or details in standard output and verbose logging in error or a different result sent in error. As seen above they have the potential to output a different verify output if the flag is set in standard output. The error is the output we expect. I certainly see the issue of they should have done it the other way around.

The comment is intended to describe why stderr is parsed instead of stdout.
In general commands should only use stderr to display error messages to the user. I tried to document why stderr was explicitly used with this commit.

Can you make a suggestion on how to describe this behavior?

https://github.com/git/git/blob/d420dda0576340909c3faff364cfbd1485f70376/gpg-interface.c#L661-662

if (flags & GPG_VERIFY_VERBOSE && sigc->payload) fwrite(sigc->payload, 1, sigc->payload_len, stdout);

Typically that is the case but it is not always the case. Looks like it outputs the raw payload to stdoutput if GPG_VERIFY_VERBOSE is set.

vbjay · 2022-10-16T07:39:44Z

GitCommands/Git/GitModule.cs

                foreach (var file in nonNewFiles)
                {
-                    sb.Append($" \"{file.Name.ToPosixPath()}\"");
+                    sb.Add($"\"{file.Name.ToPosixPath()}\"");
                }



Quote or QuoteNe extension method?

Are we sure removing the space still works as expected?

Nevermind. I get it. The add adds the space.

changed to QuoteNE

gerhardol · 2022-10-16T08:37:47Z

I presume we'd want this in v4, right?

This makes no functional change, it was a cleanup to ensure that there are no "unexpected" use of stderr as for git-verify-tag.
There are still possible use of stderr in GetOutput, but as throwOnErrorExit is not exposed, the usages throws an exception if Git fails.

But I find it safe to include.
We need to stop adding to 4.0 soon and release it...
But yet another beta.

vbjay

#10262 (comment)

GitCommands/Git/GitGpgController.cs

vbjay

LGTM

vbjay · 2022-10-16T10:03:10Z

I'll run and verify changes later on today for #10242

vbjay · 2022-10-16T15:00:05Z

gerhardol#5

vbjay · 2022-10-16T16:22:22Z

So testing of this pull and the other together are confirmed that #10242 is resolved if both #10261 and #10262 are merged. See gerhardol#5 for how I tested.

AllOutput is combined use of stderr and stdout and should only be used to display messages for users. * Change a few commands to explicitly read stderr where required. Some tests had to be adapted (including MockExecutable). * Clarify usage that AllOutput is used for user popups. Most of these occurrences are related to FormCommit. The structure of these command was changed to return a bool for result and the output as a string, also if result/output were unused. * Set TODO on a few occurrences that were not investigated. Co-authored-by: Jay Asbury <vbjay.net@gmail.com>

mstv

LGTM

mstv · 2022-10-16T20:19:24Z

UnitTests/CommonTestUtils/MockExecutable.cs

@@ -98,10 +98,10 @@ public IProcess Start(ArgumentString arguments, bool createWindow, bool redirect

        private sealed class MockProcess : IProcess
        {
-            public MockProcess([CanBeNull] string output, int? exitCode = 0)
+            public MockProcess([CanBeNull] string output, int? exitCode = 0, [CanBeNull] string error = null)


❔

Suggested change

public MockProcess([CanBeNull] string output, int? exitCode = 0, [CanBeNull] string error = null)

public MockProcess(string? output, int? exitCode = 0, string? error = null)

Removed the JetBrains annotation. There are a few other uses remaining, mostly in test files

AllOutput is combined use of stderr and stdout and should only be used to display messages for users. * Change a few commands to explicitly read stderr where required. Some tests had to be adapted (including MockExecutable). * Clarify usage that AllOutput is used for user popups. Most of these occurrences are related to FormCommit. The structure of these command was changed to return a bool for result and the output as a string, also if result/output were unused. * Set TODO on a few occurrences that were not investigated. (cherry picked from commit 2e2d79f)

ghost assigned gerhardol Oct 15, 2022

RussKie approved these changes Oct 16, 2022

View reviewed changes

vbjay reviewed Oct 16, 2022

View reviewed changes

gerhardol mentioned this pull request Oct 16, 2022

Git Executable: return stderr also if not throwOnErrorExit is suppressed #10261

Merged

vbjay suggested changes Oct 16, 2022

View reviewed changes

GitCommands/Git/GitGpgController.cs Outdated Show resolved Hide resolved

ghost added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity labels Oct 16, 2022

vbjay approved these changes Oct 16, 2022

View reviewed changes

vbjay added a commit to vbjay/gitextensions that referenced this pull request Oct 16, 2022

Fix gitextensions#10262

94f3f45

vbjay mentioned this pull request Oct 16, 2022

Fix #10262 gerhardol/gitextensions#5

Closed

vbjay added a commit to vbjay/gitextensions that referenced this pull request Oct 16, 2022

fixup! Fix gitextensions#10262

36c3915

gerhardol added this to the 4.0.0 milestone Oct 16, 2022

mstv approved these changes Oct 16, 2022

View reviewed changes

fixup! annotation

9c4a355

gerhardol force-pushed the feature/i10242-cleanup-stderr branch from 10b59aa to 9c4a355 Compare October 17, 2022 20:08

gerhardol merged commit 2e2d79f into gitextensions:master Oct 17, 2022

gerhardol deleted the feature/i10242-cleanup-stderr branch October 17, 2022 20:30

ghost modified the milestones: 4.0.0, vNext Oct 17, 2022

gerhardol modified the milestones: vNext, 4.0.0 Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup handling of AllOutput for processes #10262

Cleanup handling of AllOutput for processes #10262

gerhardol commented Oct 15, 2022

RussKie left a comment

RussKie Oct 16, 2022

gerhardol Oct 16, 2022

RussKie commented Oct 16, 2022

vbjay left a comment

vbjay Oct 16, 2022

gerhardol Oct 16, 2022

vbjay Oct 16, 2022

vbjay Oct 16, 2022 •

edited

gerhardol Oct 16, 2022

vbjay Oct 16, 2022 •

edited

vbjay Oct 16, 2022

vbjay Oct 16, 2022

vbjay Oct 16, 2022

gerhardol Oct 16, 2022

gerhardol commented Oct 16, 2022

vbjay left a comment

vbjay left a comment

vbjay commented Oct 16, 2022

vbjay commented Oct 16, 2022

vbjay commented Oct 16, 2022

mstv left a comment

mstv Oct 16, 2022

gerhardol Oct 17, 2022

	public MockProcess([CanBeNull] string output, int? exitCode = 0, [CanBeNull] string error = null)
	public MockProcess(string? output, int? exitCode = 0, string? error = null)

Cleanup handling of AllOutput for processes #10262

Cleanup handling of AllOutput for processes #10262

Conversation

gerhardol commented Oct 15, 2022

Proposed changes

Test methodology

Merge strategy

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented Oct 16, 2022

vbjay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbjay Oct 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbjay Oct 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented Oct 16, 2022

vbjay left a comment

Choose a reason for hiding this comment

vbjay left a comment

Choose a reason for hiding this comment

vbjay commented Oct 16, 2022

vbjay commented Oct 16, 2022

vbjay commented Oct 16, 2022

mstv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbjay Oct 16, 2022 •

edited

vbjay Oct 16, 2022 •

edited