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

Diff not using the full width of Git Bash #39

Open
Velociraptor45 opened this issue Sep 7, 2022 · 8 comments
Open

Diff not using the full width of Git Bash #39

Velociraptor45 opened this issue Sep 7, 2022 · 8 comments

Comments

@Velociraptor45
Copy link

Hey there,

thanks for creating that amazing tool.
However, I'm using the standard Git Bash that comes with the windows git installation and the diffs are not using the full window's width.

grafik

I'm starting the bash by default with a column count of 280 and did not resize it. I'll get a split diff up until a split-diffs.min-line-width of 60 - after that it's only inline.
At first, I suspected it's because of my monitor's 150% scaling, but reverting that to 100% did not change anything. And in general it works: doing a git diff in window's cmd works perfectly and uses the console's full width.

And ideas why it's not working with the Git Bash?

@banga
Copy link
Owner

banga commented Sep 8, 2022

Interesting, I use this library to figure out the terminal size in a cross platform manner: https://github.com/sindresorhus/term-size/blob/main/index.js. From reading the implementation, I'm guessing it's seeing a different value for process.platform than win32 and ending up falling through to the default of 80.

If you're comfortable with debugging this on your end, find your global node_modules (npm list -g will show you), then within it navigate to git-split-diffs/node_modules/term-size/index.js and add some console.log statements. Then if you run git diff again, those statements should print and show you where it is going wrong. Then we can maybe file an issue with https://github.com/sindresorhus/term-size/ to handle that edge case.

A less ideal workaround that might work is setting the COLUMNS and LINES environment variables that this library seems to read from. It's also possible that your mingw shell is setting these incorrectly and causing this issue.

@Velociraptor45
Copy link
Author

I did a little digging, and it seems like the library has a fallback (whatever this term-size.exe is doing) for windows if stderr.columns & stderr.rows is not filled. I opened an issue for Git Bash support with an implementation suggestion here sindresorhus/terminal-size#22

@banga
Copy link
Owner

banga commented Sep 8, 2022

Perfect, thanks!

@Velociraptor45
Copy link
Author

Hey there, the owner of terminal-size seems to have added support for xTerm / mintty. Would be great if you could update the package :)

@banga
Copy link
Owner

banga commented Nov 24, 2023

Hey thanks for chasing that down. The developer of terminal-size also forced everyone to use ESMs in a prior release (https://github.com/sindresorhus/terminal-size/releases/tag/v3.0.0), which is quite a bit of pain to migrate to, tbh. I managed to get the build working locally in https://github.com/banga/git-split-diffs/compare/esm-migration, but tests and benchmarks don't work yet, so I don't want to publish this yet. I'll come back to it.

Meanwhile, if you feel comfortable, you can try checking out this branch and building it locally (via yarn build:publish). Then you can edit your .gitconfig to use the locally built version (something like pager = node <path to checkout>/build/index.mjs --color | less -FX should work).

banga added a commit that referenced this issue Dec 29, 2023
This converts the package to generate an ESM module as the build output. The instigator was #39  but there were a number of ESM-only packages in the dependencies. The final result is as follows:
* Switched from `term-size` to `terminal-size` and upgraded `ansi-regex`, `chalk` and `diff`.
* Switched to [shikiji](https://github.com/antfu/shikiji) and patched the perf optimization for `setTheme` from shiki.
* Update min node version to 18.
* There's a ~35ms improvement in startup time.
* Switched from `yarn` to `npm`. `yarn v4` does pretty weird things that were hard to get to work esp on windows.
@banga
Copy link
Owner

banga commented Dec 29, 2023

Ok I finally migrated over to ESM, which lets me use the latest terminal-size version. I've just published v1.0.1 to npm. @Velociraptor45 please try it out and let me know if this fixes your issue.

@Velociraptor45
Copy link
Author

Ok, this is an interesting one. Foremost, thanks a lot for upgrading and maintaining this. Unfortunately, it's not really doing it in v1.0.1
I've debugged the terminal-size package again and it seems fine (at first). I added a console.log to output the result of the terminalSize() function, and it correctly recognizes the row/column count when I resize the window:

grafik

However, when I do my git diff, it still looks like my initial screenshot - but only for git bash. Cmd is fine.

After also debugging your code, I have a creeping suspicion. Git Bash opens the diff in the same window but in the less pager that you have to exit with 'q' to come back to the console input. It seems like this pager is opened, and then the terminal size is calculated (not the other way round). And terminal-size has an issue with this. I'll contact the maintainer of that repo again.

Just for completion, my settings:

core.pager=git-split-diffs --color | less -+LFX
split-diffs.highlight-line-changes=true
split-diffs.wrap-lines=true
split-diffs.min-line-width=60

@banga
Copy link
Owner

banga commented Dec 30, 2023

Oh interesting. I read the new issue you filed and though I don't understand why the fix works, it looks like we roughly know where the fix lies, so hopefully this should get addressed soon. I'll pull in the update when that happens.

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

No branches or pull requests

2 participants