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

windows output is formatted incorrectly after a clone is performed #4856

Closed
bchadwic opened this issue Dec 5, 2021 · 6 comments
Closed

windows output is formatted incorrectly after a clone is performed #4856

bchadwic opened this issue Dec 5, 2021 · 6 comments
Labels
bug Something isn't working help wanted Contributions welcome p2 Affects more than a few users but doesn't prevent core functions windows

Comments

@bchadwic
Copy link
Contributor

bchadwic commented Dec 5, 2021

Describe the bug

When gh repo clone or gh repo fork clones a repository, the output after the clone progress is uninterpretable when styled

Steps to reproduce the behavior

On Windows, using powershell or cmd.exe (inside or outside of Windows Terminal), run gh repo fork cli/cli

I originally discovered this using v2.3.0, but I downgraded to see if the release reminder would be affected as well and it is:

C:\Users\Ben\Desktop>gh repo fork cli/cli
! bchadwic/cli already exists
? Would you like to clone the fork? Yes
Updating upstream
remote: Enumerating objects: 703, done.
...
 * [new tag]           v2.3.0-pre.0           -> v2.3.0-pre.0
←[0;32m✓←[0m Cloned fork


←[0;33mA new release of gh is available:←[0m ←[0;36m2.2.0←[0m → ←[0;36mv2.3.0←[0m
←[0;33mhttps://github.com/cli/cli/releases/tag/v2.3.0←[0m

Expected vs actual behavior

expected behavior is to display the colored / styled text properly. It doesn't seem like it could be my terminal because I can run other commands that use colors / styles just fine.

check-mark

Additional Thoughts

Also the majority of the time I use this command, git and gh seem like they are fighting over the io:

? Would you like to clone the fork? Yes
Cloning into 'cli'...lone the fork? (Y/n) y

I think it might have something to do with this:
https://github.com/cli/cli/blob/trunk/git/git.go?plain=1#L353-L355
especially since it affects gh repo clone and gh repo fork:

Maybe there is a more graceful way to pass the output between git and gh so that both these issues can be resolved.

I'll start working on a pr 🤷‍♂️

@bchadwic bchadwic added the bug Something isn't working label Dec 5, 2021
@cliAutomation cliAutomation added the needs-triage needs to be reviewed label Dec 5, 2021
@mislav mislav added p2 Affects more than a few users but doesn't prevent core functions and removed needs-triage needs to be reviewed labels Dec 6, 2021
@mislav
Copy link
Contributor

mislav commented Dec 6, 2021

@bchadwic Thanks for looking into this!

git and gh seem like they are fighting over the io

That seems to be a fair assessment. However, they shouldn't "fight" over IO in a concurrent manner, since we always wait for git commands to finish before we resume output from gh. Something else is going on here, but I'm not sure what.

Note that both Survey and git clone tend to use the \r character (or the equivalent on Windows) to rewrite lines in the terminal with updated contents. Sometimes, when that is used inappropriately, it could cause overwriting the lines that you didn't wish to modify. On its own, Survey actually has a known bug that may or may not have effect here where the prompt is getting overwritten incorrectly: AlecAivazis/survey#384 (comment)

Also note that on Windows we wrap stdout & stderr streams with go-colorable:

Out: colorable.NewColorable(os.Stdout),
ErrOut: colorable.NewColorable(os.Stderr),
This makes ANSI escape sequences for colors transparently work in Windows terminal apps. However, the git clone commands gets access to the original (unmodified) stdout/stderr streams via os.Stdout and os.Stderr. Maybe the inconsistency is between these two types of streams (wrapped vs. unwrapped)?

@mislav mislav added help wanted Contributions welcome windows labels Dec 6, 2021
@bchadwic
Copy link
Contributor Author

bchadwic commented Dec 7, 2021

@mislav

Thank you for the leads!

I hate to throw this back in your court, but I'd like to hear your thoughts on this:

In hopes to easily pinpoint the problem, I crudely copied and simplified the code that makes up gh repo fork cli/cli over to main.go. Everything works as if you typed in the actual command, however it prints the green check just fine? 🤔 It does still create my perceived "fight" effect at the top between survey and git. That probably is due to \r as you had mentioned.

https://github.com/bchadwic/cli/blob/example/cmd/gh/main.go?plain=1#L53-L81

To be sure, I ran my custom binary vs my installed version a couple times and the problem persisted only for the vanilla v2.3.0

Maybe it's time I learn how to use delve, in the meantime let me know if you have any thoughts!

@mislav
Copy link
Contributor

mislav commented Dec 7, 2021

I think your approach of isolating the problem is sound. FWIW, the green check worked for me fine after regular gh repo fork in Windows Terminal, so that bug might be either just on your end or intermittent.

I don't think using a debugger is going to necessarily help here. I would double down on recreating a minimal reproduction case, ideally divorced from regular gh/git/network functionality so that you can keep re-running it indiscriminately and without consequences.

I'm open to pairing on this one of your mornings this week.

@bchadwic
Copy link
Contributor Author

bchadwic commented Dec 8, 2021

FWIW, the green check worked for me fine

Because of this, I installed gh and git on a new user on my machine. Oddly, the new user printed the check just fine. I then decided to test the combination of each user vs each installed gh using their absolute paths:

logged in...

as new user as main user
using new user gh bad
using main user gh bad

Is it possible, even after uninstalling / reinstalling git any non system setting could've persisted that would do something like this?

I'm open to pairing on this one of your mornings this week.

I'd love to find the root of this mystery, but I'd hate for you to spend time tracking down an issue that affects only one user profile on one machine.

Feel free to close this issue if this finding renders this issue out of the cli's wheelhouse

@mislav
Copy link
Contributor

mislav commented Dec 9, 2021

Hmm, I don't know how to explain the fact that you're seeing different output when logged in with a different user 😕

You're right, maybe this issue is too obscure/intermittent for us to spend time doing a deep dive on, but as a stab in the dark, I do think that stopping using os.Stdout for shelled-out git commands and instead of connecting them to the output streams of our IOStreams object would generally be better. Same for Survey: instead of letting it connect to the default os.Stdout, we would ideally route it through our own output stream. (In practice, this wouldn't be feasible with the current implementation of Survey because if we feed it a custom output stream, it won't be able to measure the width of the terminal.)

@bchadwic
Copy link
Contributor Author

Agreed, I stopped sourcing the problem for now. If I see anything else out of place with related output, I'll report back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Contributions welcome p2 Affects more than a few users but doesn't prevent core functions windows
Projects
None yet
Development

No branches or pull requests

3 participants