difftastic icon indicating copy to clipboard operation
difftastic copied to clipboard

terminal size is not correctly determined when using git on windows

Open jessesna opened this issue 2 years ago • 6 comments

The terminal size is not correctly determined when using git on windows with difft. It uses the default of 80 from here because term_size doesn't get a valid io stream.

jessesna avatar Dec 07 '21 16:12 jessesna

I think this should be fixed now, let me know if not.

Wilfred avatar Dec 08 '21 07:12 Wilfred

Thanks for the fast reponse @Wilfred ! The issue is still there. I checked the way how the new crate is determining it and think it won't work that way because of how git.exe starts difft. Here's the situation: image image I think the only way to get the correct terminal size in this situation is to query the transitive parent somehow, but that's not easy on windows i think because there's no direct API for that. GetConsoleScreenBufferInfo works on a handle to a io stream and there's no easy way to get such from another process. I think creating a remote thread and injecting a dll to query that would be needed, but hopefully there's an easier way. Maybe a quick n dirty workaround for the moment would be to offer a command line parameter to specify the width?

EDIT: I tried the easy way with AttachConsole but that doesn't work at all. Also tried a dll injection and that works. But it also introduces a little delay. And of course the need to maintain a injection dll. And false positives from weak virusscanner heuristics. Imo still much better than manually specifying the width or living with the default of 80.

jessesna avatar Dec 08 '21 09:12 jessesna

OK, I've added a DFT_WIDTH environment variable that overrides the terminal width detection.

Wilfred avatar Dec 11 '21 22:12 Wilfred

I tinkered around a little and got a working version which is able to determine terminal sizes of any direct or transitive parent process and i hoped you could give it a try? It can be found here: https://github.com/jessesna/difftastic/commit/54206e863feacf31393942b4d609bce3507cae76

Basically i wrote a wrapper for terminal-size which, only for windows, injects a dll into parent processes recursively to be able to query their console infos. The dll is part of the lib and will be "installed" on first use because, as i had to find out, there's no possibility to let cargo do the job.

Note that since it's my first experience with Rust, it might not come close to how this would be correctly handled in Rust but i'd love to make some modifications and send a PR.

jessesna avatar Dec 12 '21 20:12 jessesna

It's possible to check all three streams with terminal_size but it has to be done manually.

Please note that term_size is discontinued:

Maintenance status This crate is no longer maintained. It works fine, but the maintainers don't have time and willingness to work on it or review future PRs anymore.

Consider using terminal_size instead.

jessesna avatar Dec 20 '21 08:12 jessesna

thanks Wilfred for that DFT_WIDTH var 👍🏼

This is my current workaround for anyone else on windows using powershell

$Env:DFT_WIDTH=$Host.UI.RawUI.WindowSize.Width; git diff

so that it works perfectly when you resize the terminal window. If you know powershell you could manage to put this mess in a function or something, I couldn't bother to figure it out.

PS: I can also confirm that 6f1baae23e9c99df246253e3425a39850f55a2fb did not fix the issue as written in your commit message

Omar-Elrefaei avatar Dec 21 '21 02:12 Omar-Elrefaei

This has probably been fixed by #341, let me know if you still have issues.

Wilfred avatar Aug 28 '22 22:08 Wilfred

Sorry :( this is still a problem on v0.35 which appears to include the fix from #341, both in classic cmd.exe and in Windows Terminal (either from MS Store or GitHub)

image

fredemmott avatar Sep 16 '22 13:09 fredemmott

Can confirm; neither term_size nor terminal_size are able to get the actual terminal size on Windows. any_terminal_size has not enough reputation but would work; see https://github.com/Wilfred/difftastic/pull/77

jessesna avatar Sep 16 '22 15:09 jessesna