Skip to content
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.

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

Don't show console on Windows #1075

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

sheremetyev
Copy link
Contributor

@sheremetyev sheremetyev commented Aug 11, 2024

When used inside GUI application, git2 causes console window to appear temporarily, due to execution of sh.

Command needs special flags to prevent that. More details: https://stackoverflow.com/a/60958956

(For context - similar change in gitoxide: GitoxideLabs/gitoxide#1506)

When used inside GUI application, git2 causes console window to appear temporarily, due to execution of sh.

Command needs special flags to prevent that. More details: https://stackoverflow.com/a/60958956
@ehuss
Copy link
Contributor

ehuss commented Oct 26, 2024

@ChrisDenton does this make sense to you? Essentially, when configuring a credential helper on Windows, the git2 library executes it (first trying with sh). It sounds like in a GUI application this is popping open a console window. Is this the right way to suppress that?

@ChrisDenton
Copy link
Member

Sure.

The only downside of CREATE_NO_WINDOW I can think of is that it may be slightly slower to execute in some circumstances because it needs to create a new console session (albeit not a shell or a window) even if one already exists. However, I doubt that will have a measurable impact. Using DETACHED_PROCESS would fix that except if the child process itself spawns child processes then you may get a console window again.

Another option would be to merely hide any new window. But the Rust API is nightly only and on older versions of Windows it may still show a flash of the console window before it's hidden.

tl;dr CREATE_NO_WINDOW is probably the best option currently. An alternative would be to give library users some measure of control over process spawning. But that's more complicated and perhaps unnecessary.

@ehuss
Copy link
Contributor

ehuss commented Oct 26, 2024

OK, thanks! I'll go ahead and merge, and if we have any issues, we can revisit.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss added this pull request to the merge queue Oct 26, 2024
Merged via the queue into rust-lang:master with commit 8471723 Oct 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants