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

Remove spinner from all progress (allow auth prompts to use the terminal) #439

Merged
merged 4 commits into from
Sep 22, 2020

Conversation

mjcheetham
Copy link
Member

Ensure authentication prompts shown by Git and any configured credential
helpers can write to the terminal/TTY and capture information.

We don't really need the fancy spinner to show "Authenticating...";
this is only cosmetic.

Also explicitly pass-down the userInteractive=true argument down the
call stack to the construction of the Git process, and move the setting
of GIT_TERMINAL_PROMPT=0 into the if-not-user-interactive block.

Copy link
Contributor

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

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

We talked offline about some things. Just don't forget the Readme.md changes.

Scalar/CommandLine/ScalarVerb.cs Outdated Show resolved Hide resolved
Ensure authentication prompts shown by Git and any configured credential
helpers can write to the terminal/TTY and capture information.

Explicitly pass the userInteractive=true argument down the call stack to
the construction of the Git process, and move the setting of
GIT_TERMINAL_PROMPT=0 into the if-not-user-interactive block.
Remove all progress spinners from all commands. Git may need to prompt
for authentication or other prompts during a command invocation, and the
spinner interferes with this.
After the clone completes, add a final success (or failure) message.
@mjcheetham mjcheetham changed the title clone: ensure auth prompts can be shown over TTY Remove spinner from all progress (allow auth prompts to use the terminal) Sep 22, 2020
Copy link
Contributor

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

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

One small nit. Looks great!

}
else
{
this.Output.WriteLine("Complete with errors.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps "Process finished with errors." ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to also change "Complete" to something else? I was just trying to be consistent in the "done/complete/finished" wording. Happy to go with whatever.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your wording is fine. Ignore me and merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're probably having a nice, quiet evening. I'll merge ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! We can always change this at a later date if needs be.

@derrickstolee derrickstolee merged commit 944baa2 into microsoft:main Sep 22, 2020
@mjcheetham mjcheetham deleted the clone-auth-tty branch September 23, 2020 08:35
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Oct 11, 2020
The last callers of the IsConsoleOutputRedirectedToFile()
methods in the ScalarPlatform classes were removed in commit
4183579 in PR microsoft#439.

Therefore we can remove this method, which was never properly
implemented on POSIX anyway, per microsoft/VFSForGit#1355.
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Oct 11, 2020
The last callers of the IsConsoleOutputRedirectedToFile()
methods in the ScalarPlatform classes were removed in commit
4183579 in PR microsoft#439.

Therefore we can remove this method (which was never properly
implemented on POSIX anyway per microsoft/VFSForGit#1355) as
well as the native Windows system call wrappers and enums
which were used only by the WindowPlatform implementation of
this method.
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.

2 participants