Conversation
mislav
left a comment
There was a problem hiding this comment.
This workaround has merit but also some edge-cases where it doesn't work, which makes me doubt whether we are on the right track.
Could an alternate solution be to provide an escape hatch for gh injecting itself as a credential helper for every git invocation? Then we could tell Cygwin users to manually use the escape hatch, i.e. to shift the responsibility on them. The plus side would be that someone in the future who also has trouble with our credential helper could use that same mechanism too.
| } | ||
|
|
||
| func isCygwinTerminal(fd uintptr) bool { | ||
| func IsCygwinTerminal(fd uintptr) bool { |
There was a problem hiding this comment.
Since we're exporting a new function, maybe it's better to add it as a method to IOStreams? That way an IOStreams instance can just figure out the file descriptor bit without the caller having to pass it in.
| io := f.IOStreams | ||
| ghPath := f.Executable() | ||
| stdout, ok := io.Out.(*os.File) | ||
| if ok && iostreams.IsCygwinTerminal(stdout.Fd()) { |
There was a problem hiding this comment.
If stdout was redirected, then I think this would result in a false negative. So an invocation in cygwin like gh repo clone ... >/dev/null could start erroring out again due to the original bug.
The go-gh term package has an approach of inspecting the commanding terminal even when inputs and outputs were redirected: https://github.com/cli/go-gh/blob/86f3a6e29984a9cbbef8f8d8ce031a6d43739e3d/pkg/term/env.go#L122-L125
|
@mislav I agree. I think I have an alternative approach. Will close this PR and open up another. |
Fixes #6950