search

martinvonz / jj

A Git-compatible VCS that is both simple and powerful
https://martinvonz.github.io/jj/
Apache License 2.0
8.25k stars 277 forks source link

FR/Bug: For tools that wrap to terminal width, `jj log -p --tool` often has the tool overflow by the width of the graph #4161

Open ilyagr opened 1 month ago

ilyagr commented 1 month ago

Description

Cc: #4158

Steps to Reproduce the Problem

  1. Pick a repo with a wide graph and/or a narrow terminal
  2. Install difftastic or delta
  3. jj log -p --tool difft or jj log -p --tool delta

Actual Behavior

The tool thinks it has the entire terminal width to use, even though the graph takes up some columns.

image

Workaround

If less is ultimately the pager used, typing -S in it to chop long lines helps a lot.

Expected behavior

There are a few things jj could do. It's unclear if there is a perfect solution, though. Some options:

  1. We could (and probably should) have a $terminal_width variable in the syntax for diff tools, analogously to the $left/$right` variables that get replaced with file paths.

    One problem with this is that, in all cases except jj log -p --tool, letting the tool auto-detect the width is fine. With our syntax, it's hard to add an argument for terminal width to the tool arguments conditionally on whether $terminal_width is defined.

  2. We could (and probably should) define a $COLUMNS environment variable with the correct terminal width, and maybe $JJ_COLUMNS. Apparently, POSIX tools are supposed to use COLUMNS.

    However, neither difft not delta seem to respect the $COLUMNS variable in practice. I'm not sure whether Rust libraries like crossterm could or should be convinced to use it, especially on non-POSIX systems.

    Also, I think some versions of bash might reset the value of $COLUMNS (I think I experienced this with the old bash version that sh defaults to on Mac OS). This is why I suggested $JJ_COLUMNS.

  3. If 1 or 2 was implemented, we can have the user make (or pre-make) shell wrappers for difft or delta that fix the limitations described above. Supporting such wrappers for both sh and PowerShell might be annoying.

  4. Update: For completeness, another option is to create a PTY with the correct number of columns and run the tools in that. I don't know whether this is possible on Windows. I think https://github.com/sachaos/viddy uses this.

yuja commented 1 month ago

it's hard to add an argument for terminal width to the tool arguments conditionally on whether $terminal_width is defined.

I think the width/columns parameter should be set unconditionally. jj controls the terminal, so it makes sense that the diff backend respects the width provided by jj.

ilyagr commented 1 month ago

I'm mildly worried this could force wrapping when it's undesired, similarly to the problem I mentioned (and I think you referred to) in https://github.com/martinvonz/jj/issues/4160#issuecomment-2251867043.

BTW, I remembered an option 4 to add to the description, even though I'm not sure about it.

Update: This also applies to your point from #4160 about jj log --no-graph sometimes potentially used for machine-readable output. If jj log --no-graph always instructs the tool to wrap text, that would be harder.

yuja commented 1 month ago

I'm mildly worried this could force wrapping when it's undesired,

If the backing diff tool is difft --display=side-by-side for example, shouldn't it always do line wrapping no matter if --width=$width parameter is specified or not? If the user needs some conditional, I think they can use separate tool names.