Throw on non-zero exit code and on stderror output #9056

mstv · 2021-04-08T00:33:11Z

Fixes #6527
Fixes #8263
Fixes #8632
Closes #8906 as obsolete
Closes #8961 as obsolete

Proposed changes

Throw ExternalOperationException if a started process exits with a non-0 exit code or if the process printed output to StandardError
Suppress this exception for commands whose errors are handled

Screenshots

Before

After

Test methodology

manual
existing NUnit tests

Test environment(s)

Git Extensions 33.33.33
Build 3bed1c8
Git 2.27.0.windows.1 (recommended: 2.30.0 or later)
Microsoft Windows NT 10.0.19042.0
.NET Framework 4.8.4300.0
DPI 96dpi (no scaling)

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

gerhardol · 2021-04-09T22:45:12Z

There are some more situations where errors are handled, where IsGitErrorMessage() parses the output.
Those should be changed though
(as well as ExitCode == 0 should be replaced with ExitedSuccessfull)

Some situations like "describe" and in RevParse() (that should have --quiet --verify and check status anyway) are handled in this PR.

diff --git a/GitCommands/Git/GitModule.cs b/GitCommands/Git/GitModule.cs
index 6aace..9124d 100644
--- a/GitCommands/Git/GitModule.cs
+++ b/GitCommands/Git/GitModule.cs
@@ -2507,14 +2507,12 @@ public IReadOnlyList<GitItemStatus> GetTreeFiles(ObjectId commitId, bool full)
             bool excludeAssumeUnchangedFiles = true, bool excludeSkipWorktreeFiles = true,
             UntrackedFilesMode untrackedFiles = UntrackedFilesMode.Default)
         {
-            var output = _gitExecutable.GetOutput(
-                GitCommandHelpers.GetAllChangedFilesCmd(excludeIgnoredFiles, untrackedFiles));
-
-            List<GitItemStatus> result = _getAllChangedFilesOutputParser.Parse(output).ToList();
-            if (IsGitErrorMessage(output))
+            var exec = _gitExecutable.Execute(GitCommandHelpers.GetAllChangedFilesCmd(excludeIgnoredFiles, untrackedFiles));
+            List<GitItemStatus> result = _getAllChangedFilesOutputParser.Parse(exec.StandardOutput).ToList();
+            if (!exec.ExitedSuccessfully)
             {
                 // No simple way to pass the error message, create fake file
-                result.Add(createErrorGitItemStatus(output));
+                result.Add(createErrorGitItemStatus(exec.StandardError));
             }
  
             if (!excludeAssumeUnchangedFiles || !excludeSkipWorktreeFiles)

RussKie

👍 with few comments

GitCommands/Git/Executable.cs

GitUI/BranchTreePanel/RepoObjectsTree.cs

gerhardol

+0
In general very much needed, good time to add this now.
Will use myself.
(only briely looked at the code now.)

Edit: Missing Working directory for exceptions
Also, the exception message do not have to be displayed in the log part, details is enough
(not checked if this PR really changed this)

I get an exception now when starting up, some dashboard repo does not exist I assume

gerhardol · 2021-04-19T21:19:18Z

GitCommands/Git/GitModule.cs

                "--pretty=format:\"%T\"",
-                "--max-count=1"
+                "--max-count=1",
+                $"-- {stashName}^3"


-- separates paths, stashNane is a ref
The order is correct now though

The commit message contains the git error:

Fixup git arguments for GetStashDiffFiles fatal: ambiguous argument 'stash@{0}^3': unknown revision or path not in the working tree. Use '--' to separate paths from revisions

sounds a bit confusing to me; but works well

stashName is not a path, so it should not be separated with --

Without "--", git outputs the above fatal error with non-zero exit code.

gerhardol · 2021-04-19T21:19:24Z

GitCommands/Git/GitModule.cs

                        !string.IsNullOrEmpty(secondRevision) &&
                        !string.IsNullOrEmpty(firstRevision) &&
                        !secondRevision.IsArtificial() &&
                        !firstRevision.IsArtificial()
                ? GitCommandCache
                : null;

-            var patch = _gitExecutable.GetOutput(
+            bool nonZeroGitExitCode = firstId == ObjectId.WorkTreeId && secondId is not null && !isTracked;


Should not be here but at the caller (may check arguments).
!isTracked cannot use git-diff (regardless of first/second commit).

repro:

create a new file (untracked)

select Working directory, then Ctrl+click the checked out commit, which turns the added (exit code 0) into a removed file from this perspective (exit code 1)

The caller should not request this information, it is meaningless.

We need to have at least a comment explaining this. It is not obvious what this check is and why it is needed.

Should not be here but at the caller (may check arguments).
!isTracked cannot use git-diff (regardless of first/second commit).

The caller should not request this information, it is meaningless.

Explain it in other words, please.

It is not obvious what this check is and why it is needed.

Would you better understand it with nonZeroGitExitCodeExpected?
It is used once - in the next statement.

mstv · 2021-04-20T19:37:22Z

I get an exception now when starting up, some dashboard repo does not exist I assume

cannot reproduce

mstv · 2021-04-20T19:48:01Z

Missing Working directory for exceptions

How did you get this?
Usually the directory is there (renamed to just "Directory":

gerhardol · 2021-04-20T21:26:18Z

Failure in repo without tags (similar to git-describe).
The problem during startup too.

Edit: fixed negated check

       public IReadOnlyList<IGitRef> GetRefs(GetRefsEnum getRef)
        {
            // We do not want to lock the repo for background operations.
            // The primary use of 'noLocks' is to run git-status the commit count as a background operation,
            // but to run the same in a foreground for FormCommit.
            //
            // Assume that all GetRefs() are done in the background, which may not be correct in the future.
            const bool noLocks = true;

            ArgumentString cmd = GitCommandHelpers.GetRefsCmd(getRef, noLocks, AppSettings.RefsSortBy, AppSettings.RefsSortOrder);
            ExecutionResult exec = _gitExecutable.Execute(cmd, throwOnErrorOutput: false);
            return exec.ExitedSuccessfully
                ? ParseRefs(exec.StandardOutput)
                : new List<IGitRef>();
        }

GitUI/CommandsDialogs/SettingsDialog/CheckSettingsLogic.cs

gerhardol · 2021-04-21T21:26:49Z

I have experienced a few more situations where Git exists with non-zero.
Pushed to a private branch, I believe you can pick them from there
(if you want a motivation let me know, I logged with stack trace while working today and have not investigated the details).

Edit: I also get warnings like git-diff --check is enabled, not recreated yet.

mstv · 2021-04-21T22:41:01Z

I have experienced a few more situations where Git exists with non-zero.
Pushed to a private branch, I believe you can pick them from there

Thank you. I have cherry-picked them to my work branch all. I will look at the details later.

mstv · 2021-04-22T20:43:21Z

(if you want a motivation let me know, I logged with stack trace while working today and have not investigated the details)

Yes, please, especially the exception text taken from StandardError. I find warnings or errors from these commands quite surprising.

gerhardol · 2021-04-22T22:56:37Z

One more push with a fix for git-diff with core.safecrlf

gerhardol · 2021-04-23T21:18:43Z

I still have an exception at startup. Just pushed a commit to be able to work.

The sidepanel is init without the working directory is set. That should be disallowed for basically all commands.
Have not found what causes this.

mstv · 2021-04-24T07:17:34Z

The sidepanel is init without the working directory is set. That should be disallowed for basically all commands.

Good catch! This might also be the cause why some of the ROT tests work unstable (and have been disabled).

mstv · 2021-04-29T21:11:20Z

(if you want a motivation let me know, I logged with stack trace while working today and have not investigated the details)

Yes, please, especially the exception text taken from StandardError. I find warnings or errors from these commands quite surprising.

I could not reproduce these exceptions. Could you provide details for the first four !fixup commits, please?

gerhardol · 2021-04-29T21:54:34Z

Added two commits where error is OK (this was the remaining fatal: reference I saw too)
Maybe all GetOutputLines() should be ignored

The commit messages has descriptions
(Some more time to explain the other four)

gerhardol · 2021-05-10T21:51:42Z

42e27c7

Line patches exit with error and error text if patches cannot be applied
To use Execute() instead of GetOutput() the input array need to be made to a stream.

gerhardol · 2021-05-18T21:25:18Z

git-mergetool can exit with error if user do not complete the merge
(pushed as well but my private branch is getting out of date)

diff --git a/GitCommands/Git/GitModule.cs b/GitCommands/Git/GitModule.cs
index e6ca8..6f2dd 100644
--- a/GitCommands/Git/GitModule.cs
+++ b/GitCommands/Git/GitModule.cs
@@ -880,7 +880,7 @@ public void RunMergeTool(string? fileName = "", string? customTool = null)
                 { !string.IsNullOrWhiteSpace(fileName), "--" },
                 fileName.ToPosixPath().QuoteNE()
             };
-            using var process = _gitExecutable.Start(args, createWindow: true);
+            using var process = _gitExecutable.Start(args, createWindow: true, throwOnErrorOutput: false);
             process.WaitForExit();
         }

RussKie

👍

ghost · 2021-07-28T10:33:24Z

Hello @RussKie!

Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request.

RussKie · 2021-07-28T10:34:18Z

I have few questions, disabling the auto-merge for now.

- Ignore StandardError output - Use it only as exception text on non-zero exit code makes gitextensions#9056 less intercepting, fixes gitextensions#9462

- Ignore StandardError output - Use it only as exception text on non-zero exit code makes gitextensions#9056 less intercepting, fixes gitextensions#9462 Except for the actual changes in Executable.cs, this is just renaming the argument to match the changed functionality.

- Ignore StandardError output - Use it only as exception text on non-zero exit code makes #9056 less intercepting, fixes #9462 Except for the actual changes in Executable.cs, this is just renaming the argument to match the changed functionality. Co-authored-by: Michael Seibt <mstv@gmx.net>

ghost assigned mstv Apr 8, 2021

mstv marked this pull request as draft April 8, 2021 00:33

RussKie reviewed Apr 10, 2021

View reviewed changes

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

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

GitUI/BranchTreePanel/RepoObjectsTree.cs Outdated Show resolved Hide resolved

This was referenced Apr 10, 2021

AutoComplete: Avoid Git for non tracked files #9058

Merged

Use exit status instead of IsGitErrorMessage() #9059

Merged

mstv force-pushed the fix/6527_git_errors branch 3 times, most recently from a129bc6 to 71b890b Compare April 18, 2021 21:20

gerhardol reviewed Apr 19, 2021

View reviewed changes

mstv force-pushed the fix/6527_git_errors branch from 71b890b to 94f6c35 Compare April 20, 2021 19:49

gerhardol reviewed Apr 20, 2021

View reviewed changes

GitUI/CommandsDialogs/SettingsDialog/CheckSettingsLogic.cs Outdated Show resolved Hide resolved

gerhardol reviewed Apr 20, 2021

View reviewed changes

GitUI/CommandsDialogs/SettingsDialog/CheckSettingsLogic.cs Outdated Show resolved Hide resolved

gerhardol mentioned this pull request May 13, 2021

LogCaptureCallStacks in AppSettings #9203

Merged

mstv mentioned this pull request May 16, 2021

Preparations for exceptions on git error #9212

Merged

mstv force-pushed the fix/6527_git_errors branch from 94f6c35 to a9b74b1 Compare May 19, 2021 20:48

RussKie reviewed May 20, 2021

View reviewed changes

RussKie removed the status: auto merge label Jul 28, 2021

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jul 28, 2021

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jul 30, 2021

RussKie approved these changes Jul 31, 2021

View reviewed changes

RussKie merged commit cea231d into gitextensions:master Jul 31, 2021

ghost added this to the 3.6 milestone Jul 31, 2021

gerhardol mentioned this pull request Jul 31, 2021

Rename detection for Advanced filter #9413

Merged

mstv deleted the fix/6527_git_errors branch July 31, 2021 16:52

mstv mentioned this pull request Aug 6, 2021

Fixes #8263 by catching file IO exception and showing message box #8535

Closed

This was referenced Aug 11, 2021

Deleting remote branch is broken #9462

Closed

Ignore warnings reading merged remote branches #9476

Closed

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Aug 12, 2021

wip gitextensions#9056 throwOnErrorOutput

7fcb918

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Aug 12, 2021

wip gitextensions#9056 throwOnErrorOutput

3d320d3

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Aug 12, 2021

wip gitextensions#9056 throwOnErrorOutput

25c5fe6

gerhardol mentioned this pull request Aug 12, 2021

Execution interface cleanup #9488

Merged

mstv added a commit to mstv/gitextensions that referenced this pull request Aug 12, 2021

Do not throw if process exits with 0

945f370

- Ignore StandardError output - Use it only as exception text on non-zero exit code makes gitextensions#9056 less intercepting, fixes gitextensions#9462

mstv mentioned this pull request Aug 12, 2021

Do not throw if process exits with 0 #9489

Closed

gerhardol mentioned this pull request Sep 8, 2021

[NBug] Invalid directory for submodule commands #9560

Closed

gerhardol mentioned this pull request Sep 8, 2021

Do not throw if process exits with 0 #9562

Merged

gerhardol mentioned this pull request Sep 9, 2021

NBug: Include GE specific exception info #9564

Merged

gerhardol mentioned this pull request Jan 15, 2022

Ignore nonexisting submodule paths #9819

Merged

gerhardol mentioned this pull request Aug 7, 2022

Do not start NBug if mergetool exits with other than 0 #10137

Merged

This was referenced Nov 5, 2022

Git Flow plugin crashes on first use #10334

Closed

GitFlow: Do not throw if git-flow is not init #10341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw on non-zero exit code and on stderror output #9056

Throw on non-zero exit code and on stderror output #9056

mstv commented Apr 8, 2021

gerhardol commented Apr 9, 2021

RussKie left a comment

gerhardol left a comment •

edited

Loading

gerhardol Apr 19, 2021

mstv Apr 20, 2021

gerhardol Apr 20, 2021

mstv Apr 20, 2021

gerhardol Apr 19, 2021

mstv Apr 20, 2021

gerhardol Apr 20, 2021

RussKie Jul 28, 2021

mstv Jul 30, 2021

mstv commented Apr 20, 2021

mstv commented Apr 20, 2021

gerhardol commented Apr 20, 2021 •

edited

Loading

gerhardol commented Apr 21, 2021 •

edited

Loading

mstv commented Apr 21, 2021

mstv commented Apr 22, 2021

gerhardol commented Apr 22, 2021

gerhardol commented Apr 23, 2021

mstv commented Apr 24, 2021

mstv commented Apr 29, 2021

gerhardol commented Apr 29, 2021 •

edited

Loading

gerhardol commented May 10, 2021

gerhardol commented May 18, 2021

RussKie left a comment

ghost commented Jul 28, 2021

RussKie commented Jul 28, 2021

Throw on non-zero exit code and on stderror output #9056

Throw on non-zero exit code and on stderror output #9056

Conversation

mstv commented Apr 8, 2021

Proposed changes

Screenshots

Before

After

Test methodology

Test environment(s)

gerhardol commented Apr 9, 2021

RussKie left a comment

Choose a reason for hiding this comment

gerhardol left a comment • edited Loading

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

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

mstv commented Apr 20, 2021

mstv commented Apr 20, 2021

gerhardol commented Apr 20, 2021 • edited Loading

gerhardol commented Apr 21, 2021 • edited Loading

mstv commented Apr 21, 2021

mstv commented Apr 22, 2021

gerhardol commented Apr 22, 2021

gerhardol commented Apr 23, 2021

mstv commented Apr 24, 2021

mstv commented Apr 29, 2021

gerhardol commented Apr 29, 2021 • edited Loading

gerhardol commented May 10, 2021

gerhardol commented May 18, 2021

RussKie left a comment

Choose a reason for hiding this comment

ghost commented Jul 28, 2021

RussKie commented Jul 28, 2021

gerhardol left a comment •

edited

Loading

gerhardol commented Apr 20, 2021 •

edited

Loading

gerhardol commented Apr 21, 2021 •

edited

Loading

gerhardol commented Apr 29, 2021 •

edited

Loading