Skip to content

fix(ProcessWrapper): Provide git error message instead of "External program returned non-zero exit code." #12085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Dec 8, 2024
Merged

fix(ProcessWrapper): Provide git error message instead of "External program returned non-zero exit code." #12085

merged 5 commits into from
Dec 8, 2024

Conversation

mstv
Copy link
Member

@mstv mstv commented Dec 3, 2024 •
edited
Loading

Proposed changes

  • feat(Git Command Log): Log StandardError, too, unless not redirected (initial draft by @gerhardol)
  • fix(ProcessWrapper): Provide git error message (if any) instead of "External program returned non-zero exit code."
  • fix(ExecutableExtensions.ExecuteAsync): Remove check for DubiousOwnershipSecurityConfigString because that error message is provided by ProcessWrapper now
  • fix(AsyncLoader): Do not swallow unhandled exceptions
  • fix(BlameControl): Show git error message and avoid outdated tooltips

Screenshots

Before

image

image image

After

image

image image

Test methodology

  • manual
  • adapt existing tests

Please do not squash merge

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

@mstv mstv self-assigned this Dec 3, 2024
gerhardol
gerhardol reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great change, especially the Git error presentation. This and other changes should be in a new release (another sidenote: limit marking for long lines?).
With this PR, the only open question I have is how the error output is handled at error and not error exit. I just do not see it as obvious right now.

@mstv
Copy link
Member Author

mstv commented Dec 3, 2024

Sorry, I thought the removal of the additional MemoryStream from ExecutableExtensions.ExecuteAsync could be done in a follow-up.
But it did not work since MemoryStream does not work as a pipe.
So I have changed the type of IProcess.StandardError from StreamReader to string.

@mstv
Copy link
Member Author

mstv commented Dec 3, 2024

how the error output is handled at error and not error exit

If the process is started with redirectOutput: true and/or throwOnErrorExit: true or unspecified, the error output is always read and traced in the Git Command Log.
If no output is redirected and throwOnErrorExit: false, there can be manual evaluation of the exit code by the caller, which has decided to ignore any output.

@mstv mstv marked this pull request as draft December 3, 2024 22:46
@mstv
Copy link
Member Author

mstv commented Dec 3, 2024

It has revealed hidden bugs. I am merging #12072, rebasing this PR and keeping the current state in fix/error_output-before_rebase for comparison.

@mstv mstv force-pushed the fix/error_output branch from 7bebabb to 0f381dc Compare December 3, 2024 23:27
@mstv mstv marked this pull request as ready for review December 3, 2024 23:41
@mstv mstv force-pushed the fix/error_output branch from 0f381dc to df94863 Compare December 3, 2024 23:47
@mstv mstv merged commit df94863 into gitextensions:master Dec 8, 2024
4 checks passed
@mstv mstv deleted the fix/error_output branch December 8, 2024 22:20
@mstv mstv added this to the v5.2 milestone Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gerhardol gerhardol gerhardol left review comments

@RussKie RussKie RussKie approved these changes

Assignees

@mstv mstv

Labels
None yet
Projects
None yet
Milestone
v5.2
Development

Successfully merging this pull request may close these issues.
3 participants
@mstv @RussKie @gerhardol