rbong / vim-flog

A blazingly fast, stunningly beautiful, exceptionally powerful git branch viewer for Vim/Neovim.
748 stars 22 forks source link

[vim][windows] Issues with carriage returns (^M) #143

Closed DanSM-5 closed 1 day ago

DanSM-5 commented 5 days ago

Description

On vim (no neovim) in windows there seem to be a couple of issues related to carriage returns.

  1. All lines in the graph :Flog end with a ^M character

image

This is small visual effect.

  1. Pressing enter <CR> on any commit fails to open it in fugitive.

image

It looks like some carriage return is getting appended to the hash. This result on a vertical split with an empty buffer.

I tested neovim as well and everything seems to work fine there.

rbong commented 5 days ago

Thanks very much for reporting.

Are you using g:flog_use_internal_lua and using Lua that's compiled with Vim?

What do you get from :lua print(_VERSION) and :lua print(jit.version)?

Appears that Lua's io.popen is system-independent, which is probably what's causing this issue:

This function is system dependent and is not available on all platforms.


I don't have a raw Windows environment to compare against working Linux environments. If anyone can compare tell me how to detect a Windows environment in Lua that's going to include carriage returns, I'm happy to implement a fix. It appears Neovim works, so it might not be all Windows environments, please be careful not to include false matches. It might come down to LuaJIT vs Lua, but I don't know what versions are installed yet. PRs also accepted.

DanSM-5 commented 5 days ago

I'm using vim from scoop which is compiled with lua 5.4 (vim --version -DDYNAMIC_LUA_DLL=\"lua54.dll\")

:lua print(_VERSION)
Lua 5.4

I think I may have misinterpreted this part of the readme

In Vim, LuaJIT 2.1 must be installed. Lua 5.1 is also supported but less performant than LuaJIT.

as in thinking that the minimum version was lua 5.1 but searching about it makes it look like there are quite some differences between lua versions.


Are you using g:flog_use_internal_lua and using Lua that's compiled with Vim?

I didn't know about this variable, so no, I didn't use it. This version of vim needs the lua54.dll in the path (or same directory) or manually setting it with let &luadll = path/to/lua54.dll which I did some time ago.

Appears that Lua's io.popen is system-independent, which is probably what's causing this issue:

This function is system dependent and is not available on all platforms.

It seems that the function exists :lua print(io.popen) -- function: 00007ffbedb25df0. Is the output directly read from the file handle? Curious if that returns the carriage returns.


With all that, does it means that the plugin only supports lua 5.1?

rbong commented 5 days ago

Yes, the plugin only supports Lua 5.1 unfortunately, because this is the version used by LuaJIT and Neovim. I recommend using LuaJIT if you can, as it is much faster. It does appear to be supported on Windows. It is the only supported version because of the previously mentioned Neovim support and speed.

As you've noted, there are significant differences between Lua versions. This is why LuaJIT is still using Lua 5.1 - it is heavily optimized and it's very difficult to add so many changes.

Please let me know if using Lua 5.1 or LuaJIT resolves your issue.

DanSM-5 commented 5 days ago

I've experimented a bit and I have some interesting findings.

Using lua 5.4

Using my vim version that has lua 5.4 and setting g:flog_use_internal_lua = 1 works. The issue I was seeing was due to calling lua as an external process which sometimes have that effect of appending the carriage return character. At least is very common for me when calling system().

Using luajit

I tried installing luajit as suggested. It didn't work. Calling :Flog produces an error like this

C:\Users\daniel\scoop\apps\luajit\current\bin\luajit.exe: ...daniel\\vim-config\\plugged\\vim-flog/lua/flog/graph.lua:62
2: table overflow^M^@stack traceback:^M^@^I...daniel\\vim-config\\plugged\\vim-flog/lua/flog/graph.lua:622: in function
'flog_get_graph'^M^@^I...aniel\vim-config\plugged\vim-flog/lua/flog/graph_bin.lua:5: in main chunk^M^@^I[C]: at 0x7ff794
721fe0^M^@1^M
Error detected while processing function flog#cmd#Flog[19]..flog#floggraph#buf#Update[21]..flog#graph#vim#Get[14]..funct
ion flog#cmd#Flog[19]..flog#floggraph#buf#Update[21]..flog#graph#vim#Get[10]..FlogGetVimBinGraph[37]..flog#shell#Run:
line    4:
E605: Exception not caught: flog: encountered shell error

Curious that luajit encounter that error while lua 5.4 (as an external process) didn't.

Maybe it would be worth to document that on windows the ^M symbol can appear when calling lua externally.

For me using the embedded lua that solves the issue (although not the supported version). Thanks for the patience, I got a bit confused with all the lua differences.

Let me know if you would like help to investigate why luajit doesn't work. If not, feel free to close the issue.

rbong commented 5 days ago

Maybe it would be worth to document that on windows the ^M symbol can appear when calling lua externally.

Unfortunately it's likely not that simple and things may still be breaking invisibly if this happens. I can't think of a single feature that won't be broken in some way by this. So PRs and help still welcome for this bug. I'm going to leave it open, you're welcome to unsubscribe.

I tried installing luajit as suggested. It didn't work. Calling :Flog produces an error like this

Might have to do with recent changes to reduce reliance on Fugitive, which has better (and more complicated) shell escaping functions (that especially apply on Windows) which I'm still unravelling and learning from. If you can switch to the v3.0.0 tag and test again if it works I would appreciate it, but if not I understand that this issue is mostly solved for you.

DanSM-5 commented 4 days ago

Tag 3.0.0 with luajit shows flog buffer like this:

image

rbong commented 1 day ago

Tag 3.0.0 with luajit shows flog buffer like this

Thanks. FYI I believe the path issue that caused the LuaJIT error may be fixed. But I haven't tested it on Windows, and it's not very useful until I can fix the carriage return issue.

rbong commented 1 day ago

I pushed fixes that should reliably remove carriage returns on Windows. Let me know if there are still issues. Thanks again for reporting this bug.