External diff command do not support `columnWidth` placeholder

[AFutureD]

Describe the bug

Using difftastic in external diff command with side-by-side display type, there is a view problem.
The lines in the right view unexpectedly wrapped.

To Reproduce
Simply scustom external diff config.

git:
  paging:
    externalDiffCommand: difft --color=always --display=side-by-side --syntax-highlight=off

Expected behavior
Pass window's width to external diff command like pager.

Screenshots

Version info:

> lazygit --version
commit=1d1b8cc01f87bb3495426ac8d81d97573f6840d4, build date=2023-10-27T04:14:57Z, build source=unknown, version=1d1b8cc0, os=darwin, arch=arm64, git version=2.39.2 (Apple Git-143)
> git --version
git version 2.39.2 (Apple Git-143)

Additional context

Note: please try updating to the latest version or manually building the latest master to see if the issue still occurs.

[stefanhaller]

Lazygit is already passing the window width to the external diff command by means of the COLUMNS environment variable, so most of the time the layout should be correct. There is a bug, however, when lazygit's view configuration changes, for example when you switch from a file that has only unstaged changes (one wide main view) to a file that has both staged and unstaged changes (main view is split into two more narrow ones). In this situation, lazygit will call the diff command with the old window width one time. If you then hit <shift-R> to refresh (or wait 10 seconds for an auto refresh), the display should be correct again.

You can confirm this behavior by setting your external diff command to externalDiffCommand: /bin/sh -c "echo $COLUMNS" for testing.

I'm not sure how easy it will be to change the code so that diff commands are always called with the up-to-date window dimensions after the view layout changed. I'll have a look.

[AFutureD]

Hi, I have tried set external diff command to externalDiffCommand: /bin/sh -c "echo $COLUMNS".
But, there is nothing shows on screen, and I have changed to echo other String which show well.

And I have also tried to wait (more than 10s) screen refresh by itself, there is nothing change.
By setting externalDiffCommand to date, I'm sure screen did refresh as expected, but COLUMNS was empty.

[stefanhaller]

Wow, interesting. That's a different problem then; we'll have to find out why COLUMNS isn't set on your system.

Anybody who is more experienced with terminal stuff and PTYs have an idea what could be wrong here? I don't know much about these topics.

I'd be curious: what does tput columns say in your shell, and what do you get in lazygit if you set externalDiffCommand: tput columns?

[AFutureD]

I'm using macOS, so I set the externalDiffCommand to tput cols.
The result is 80, and now change when I resize terminal window or do other things like hit <shift-R>.

[stefanhaller]

(Side note: tput cols and tput columns yield the same result for me (on Mac), but whatever. 😄)

So I have to admit that I have no idea why this doesn't work for you. Again, if anybody is more experienced with this stuff, I'd appreciate suggestions in what direction to dig.

One thing I learned while reading about it is that the COLUMNS thing was a red herring. COLUMNS isn't actually an environment variable, but a shell variable; some shells do export it to the environment, but not necessarily, so if this is empty for you, it doesn't have to mean anything. Also, programs like delta don't look at COLUMNS; they use ioctl with the TIOCGWINSZ parameter instead (same as what tput cols does), so this should really work, and I'm clueless why it doesn't for you.

Last question: what is your TERM environment variable set to?

[AFutureD]

I have some time yesterday, so I tried to debug lazygit.
Here is what I found:

1. I do not have any pager settings.
2. Command without pager will than use gui.newCmdTask instead of gui.newPtyTask
3. gui.newPtyTask depend on gui.desiredPtySize to refresh some info include view size.

So, when ShowCmdObj tries to run, all controllers using gui.newPtyTask to execute.
But when the task invoke, the if condition, find there is no page , so it using gui.newCmdTask instead, and view info now pass to the environment.

I try this change it works well.
But I do not now it's history, so this change just a test method.
I thought ptyTask or cmdTask should be explicit called, and should not Implicit, so remove the if condition seems better.

[AFutureD]

11b87a6

Question: Why lazygit set --no-ext-diff by default ? Does it has any historic reasons?

For now, difft can use be used directly without set --ext-diff. See: https://difftastic.wilfred.me.uk/git.html#regular-usage

[stefanhaller]

Great, thanks for investigating. This explains why difft (and tput col) was working correctly for me: I didn't bother commenting out my pager setting, so it was still using a pty for me. If I disable the pager config, I see the same problem as you do (columns is hard-coded to 80 in that case). This also means there's a work-around until we have a fix in lazygit: just set your git.pager config to an arbitrary string.

As for how to fix it: I do think that your test commit (the first one) is the correct fix. We want to use a pty if either a pager or an external diff command is configured. Do you want to make a PR out of that?

As for why we use --no-ext-diff: I still believe that this is correct and necessary, see here for an explanation.

[AFutureD]

PR #3120 is ready.

But I wonder if such problem can write some unit test, so that it will not happened again.

[stefanhaller]

Thanks. This PR is now very confusing, because the PR title and description no longer matches the change you are making. I thought it would have been easier to open a new PR instead, but I'd also be ok with changing the title and description to match.

[stefanhaller]

As for testing: I can't think of a good way to test this in a unit test, so I'd be ok with not adding one.

[stefanhaller]

Fixed by #3120.