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 pass shell=True to core.run_command() #253

Merged
merged 1 commit into from May 9, 2014

Conversation

living180
Copy link
Contributor

On Windows, the Git.execute() method was passing shell=True to the core.run_command() function. This was to avoid a console window briefly popping up on the screen each time git-cola invoked git, which could happen if git-cola was invoked using start pythonw git-cola. But, using shell=True is generally undesirable. This change stops passing shell=True and instead creates a STARTUPINFO structure and configures it to ensure that any child process window is hidden.

Because the Windows shell is no longer being invoked, the replace_carot function is no longer needed and is removed.

On Windows, the Git.execute() method was passing shell=True to the
core.run_command() function.  This was to avoid a console window briefly
popping up on the screen each time git-cola invoked git, which could
happen if git-cola was invoked using "start pythonw git-cola".  But,
using shell=True is generally undesirable.  This change stops passing
shell=True and instead creates a STARTUPINFO structure and configures it
to ensure that any child process window is hidden.

Because the Windows shell is no longer being invoked, the replace_carot
function is no longer needed and is removed.

Signed-off-by: Daniel Harding <dharding@living180.net>
@davvid
Copy link
Member

davvid commented May 9, 2014

Nicely done. 👍

davvid added a commit that referenced this pull request May 9, 2014
Don't pass shell=True to core.run_command()
@davvid davvid merged commit 92ed6c3 into git-cola:master May 9, 2014
@living180 living180 deleted the no_shell_subprocess branch May 9, 2014 13:11
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.

None yet

2 participants