support delta along with ANSI support

[ghost]

#542 (comment)

• All test passes.
• I haven't seen segmentation faults with newest versions of MacOS Big Sur, iTerm2, and delta. ncurses version is 6.2.
• It looks like no performance issues.
• I've only tested it with Terminal.app and iTerm on MacOS.

how to use delta in tig

It requires ncurse 6.1 or higher.

# if you don't have ncurses
curl -O ftp://ftp.gnu.org/gnu/ncurses/ncurses-6.2.tar.gz
tar -xzvf ncurses-6.2.tar.gz
cd ncurses-6.2
./configure --prefix=${where you like} --enable-pc-files --with-pkg-config-libdir=/usr/local/bin/pkgconfig --enable-sigwinch --enable-symlinks --enable-widec --with-shared --with-gpm=no --without-ada --enable-ext-colors # i set the path as /usr/local which is default
sudo make
sudo make install

cd ${path to tig of the branch of above PR}
make configure
./configure LDFLAGS=-L${where you download ncurses}/lib CPPFLAGS=-I{where you installed ncurses}/include  # i set the path as /usr/local/opt/ncurses which is installed by ncurses
make

echo "set diff-highlight = \"delta\"" >> ~/.tigrc

./src/tig

[hasufell]

Great stuff.

How is the current line highlighting expected to work? I found it a bit surprising:

[hasufell]

I also seem to get some line skew with very long lines:

[ghost]

Hi hasufell, thank you for catching. I'll fix those.

ps
I noticed that scrolling right breaks the beginning of the line as well.

[ronjouch]

@ulwlu hi, here to respond to your call for tests at #542 (comment) .

Your branch compiles and works for me (Arch Linux / Kitty 0.23.1, ncurses 6.2, delta 0.8.3) 🙂.

In addition to the weird line highlight already reported, I was surprised to see delta-within-tig uses a gray background. I'm not sure why, as pure delta outside of tig uses a black background. My delta config is empty, and my ~/.tigrc is empty except for the single line set diff-highlight = "delta". Is it a side-effect of the true color issue you mentioned? Feel free to ask for new tests with different color config.

Screencast below (pure delta at the beginning from 0:00 to 0:07, delta-in-tig afterwards):

delta-in-tig-screencast-ronjouch.mp4

[ghost]

@ronjouch Hi, thank you for catching and details. I'm still not sure if this is caused by coloring problem, but I'm thinking delta gave some background color info with '--trur-color ＝never' that is not close to real true-color.

I'll check it anyway. Thank you very much

[ronjouch]

Thank you very much

Much obliged; thank you for the work on this!

I'm still not sure if this is caused by coloring problem, but I'm thinking delta gave some background color info with '--trur-color ＝never' that is not close to real true-color.

@ulwlu I did a bit more monkeying around with my Kitty color config. The problem doesn't appear by default. But with my Kitty color config (pasted below), it does.

#: The color table {{{

#: The 256 terminal colors. There are 8 basic colors, each color has a
#: dull and bright version, for the first 16 colors. You can set the
#: remaining 240 colors as color16 to color255.

color0 #555753
color8 #777777
#: black

color1 #ef2929
color9 #cc0000
#: red

color2  #8ae234
color10 #4e9a06
#: green

color3  #fce94f
color11 #c4a000
#: yellow

color4  #729fcf
color12 #3465a4
#: blue

color5  #ad7fa8
color13 #75507b
#: magenta

color6  #34e2e2
color14 #06989a
#: cyan

color7  #eeeeec
color15 #d3d7cf
#: white

[pablospe]

Another details. Have you tried side-by-side mode? It seems to work but only until 80 chars. So, I was thinking to use delta --width 250, but this doesn't seem to work: set diff-highlight = "delta --width 250". How do you pass parameters?

I've also observed a weird highlighting, gray background. I am using konsole.

[pablospe]

Also noticed that when there are many files and you press enter to the filename (in the commit header), it doesn't jump to beginning of the file in the diff. Instead I received this error:

[diff] Press '<Enter>' to jump to file diff - line 11 of 88                                                                                                                                                                                 
Failed to find file diff

[ghost]

@hasufell @ronjouch

Hi. I've successfully fixed those, but I've come up with a better design. So I've decided to refactor it, which should take about a week.

@pablospe

Tig can't accept formatter options. I can fix that but it will be on another PR since it has some impact on another function. Currently could you set config locally?

[pablospe]

Tig can't accept formatter options. I can fix that but it will be on another PR since it has some impact on another function. Currently could you set config locally?

Apologies, I didn't understand. What do you mean by set config locally? Having an alias or something like that to avoid options?

[ghost]

@pablospe

Sorry for the lack of explanation, please do the following at this time.

git config delta.width 250

From your explanation, I think you don't want to set it to global. However, tig does not accept options now, so please set locally like this inside repositories that you use tig. Currently tig searches only executable binary, so alias doesn't work either. I will probably fix it to accept options if jonas agreed.

Also, in the original issue I asked what to do with the side-by-side option inside tig. If you have time, please feel free to post something like "allow tigrc to accept options and allow users to set width freely", so that someone may post an another idea.

#542 (comment)

[pablospe]

Also another problem I found is that in navigation mode for delta doesn't work:
n and N keybindings to move between files in large diffs, and between diffs in log -p views (--navigate). See in delta documentation (here)

I am just reporting the problems I found. Thanks a lot for the fantastic work, @ulwlu!

[krobelus]

I haven't checked the implementation details but this looks a bit weird on light terminals like gnome-terminal or xterm because it draws dark backgrounds.

[ghost]

@krobelus Hi. Yes that occured because of here.

https://github.com/jonas/tig/pull/1140/files#diff-3cd1efc51226a700b579928f3ee8477ed1a05715a60b2d6e0b1fc54e63969581R50

I noticed that and fixed it into default_bg. Thank you for catching.

[pablospe]

Currently I'm fixing a bit of functions which pablospe mentioned and more else.

Maybe some of the changes can be separated in other PRs and add basic delta support on master? Even if it is not perfect, it would be good to have the delta support.

[ghost]

@pablospe sorry to be late. I was busy with my work and time flies so fast. That sounds great, I wanna add at least one more feature then.

• handle delta output and draw correct color
• draw correct current cursor
• calculate the correct line length ( so that the line won't be overwrapped)
• handle correct pager <- i want to add at least this one (edit: done on 2/7)

These can be called basic feature, and we can add some features such as jumping to the file hunks.

I probably can add in this year, or at least next January

p.s. on 2022/1/21

I finally have some time to focus on this, so I start working on it.

I'm sorry for the delay, but I had a service launch on 1/16 at work and had been so busy until that, and in my personal life.

p.s. on 2022/2/4

i'm close to complete pager func. please give me a while.

[fengyichui]

Hi,
In diff view, the file jump feature does not work properly and show error message: "Failed to find file diff"

[ghost]

@fengyichui
please see the comment above, this supports only the basic feature.

Maybe some of the changes can be separated in other PRs and add basic delta support on master? Even if it is not perfect, it would be good to have the delta support.

[fengyichui]

@ulwlu Sorry, I don't quite understand, do you mean it's already supported or need other PRs ?

[ghost]

it needs an other PR.

[fengyichui]

@ulwlu Thanks for your answer.

[edubxb]

Looking forward to seeing this merged, awesome, thanks!

[crokobit]

Can we merge this?

[FdelMazo]

Hey @koutcher, any chance we can get this merged? I'd love this feature

[koutcher]

Yes, but don’t hold your breath, there is still much to do before it can make it to master. @ulwlu, thank you very much for this PR, it is a great work and I think it is really an important contribution for Tig. I’m sad I can’t find enough time these days to give it the review it deserves, and @jonas doesn’t seem to have more availabilities either. So, here are some first considerations, but I can’t guarantee there won’t be more:

• the PR allows to use delta instead of diff-highlight but claiming delta support in documentation would be a bit abusive at this stage as using delta breaks all Tig’s builtin shortcuts and we don’t want to be flooded with Issues that it doesn’t work.
• the PR should be rebranded as ANSI support rather than delta support. The following change should allow git diff --color-words | tig to work almost as expected (some ^[[m aren't handled properly):

diff --git a/src/draw.c b/src/draw.c
index fd183e5a..e6ba33ad 100644
--- a/src/draw.c
+++ b/src/draw.c
@@ -168,7 +168,7 @@ draw_text_expanded(struct view *view, enum line_type type, const char *string, i
                size_t pos = string_expand(text, sizeof(text), string, length, opt_tab_size);
                size_t col = view->col;

-               if (opt_diff_highlight && *opt_diff_highlight && strcmp(opt_diff_highlight, "delta") == 0 && strstr(string, "\033[") != NULL) {
+               if (strstr(string, "\033[") != NULL) {
                        if (draw_chars_with_ansi(view, type, text, -1, max_width, use_tilde))
                                return true;
                } else {

• Tig shall remain differ-agnostic, we don’t want to have references to delta in Tig source code, otherwise every single differ will come to ask for its command line to be added. If arguments are really needed this should be done through an helper script or we should have another PR to enable using arguments for diff-highlight in .tigrc.
• I’m not a huge fan of the 256x256 fixed array for colors.
• all these if (strcmp(... without elses, is it on purpose ?
• Tig has, so far, been C90 compliant. Until Jonas decides to change this, it shall continue.
• This PR won’t work for all ncurses version, we’ll have to add a few ifdefs.

[ghost]

Thank you @koutcher for reviewing.
Your personal time should always come first, so never mind.

I think your considerations are all reasonable and I want to get started on them (especially the 256x256 fixed array, which I thought same thing when I created it. I didn't see any performance degradation in memory or speed after creation, but I'll look again to see if there are any other workarounds).

Actually, I will start working for a new company next week and will be a bit busy, so I will start next week after a while.
In terms of below question,

all these if (strcmp(... without elses, is it on purpose?

Yes. I didn't put else where else is not necessary.

Thanks again for taking the time to review this.

[ghost]

@koutcher
Hi.

Tig shall remain differ-agnostic, we don’t want to have references to delta in Tig source code, otherwise every single differ will come to ask for its command line to be added. If arguments are really needed this should be done through an helper script or we should have another PR to enable using arguments for diff-highlight in .tigrc.

Currently it is necessary, however, it's still only one required option so we can ask users to add that option true-color=never in .gitconfig. After that, we will be able to work another PR to extend diff-highlight to accept arguments.

I already have the fixed code referring to your reviews (except 256x256 fixed array).

However, unfortunately, due to my current company rules, I can't continue to Open Source Projects anymore.
Can I email this fixed code to your email address which is written in your git commit?

[koutcher]

@ulwlu, please do. As I'm not a delta or any other fancy diff user, I'll create a feature branch to allow contributions until it is mature enough to be merged. Thanks again for your time.

[ghost]

@koutcher
just a note to let you know, I've already sent you a patch in email 12 days ago. (from [email protected])
you don't need to reply this if you've noticed it.

[ijoseph]

For Apple Silicon (M1)/ Homebrew-managed ncurses, I had to

./configure \
 LDFLAGS="-L/opt/homebrew/opt/ncurses/lib" \
 CPPFLAGS="-I/opt/homebrew/opt/ncurses/include" \
 PKG_CONFIG_PATH="/opt/homebrew/opt/ncurses/lib/pkgconfig"

(was getting init_extended_pair not declared error otherwise)

based on

╰─ brew info ncurses
==> ncurses: stable 6.3 (bottled) [keg-only]
Text-based UI library
https://invisible-island.net/ncurses/announce.html
/opt/homebrew/Cellar/ncurses/6.3 (3,968 files, 9.6MB)
  Poured from bottle on 2022-03-17 at 16:24:05
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/ncurses.rb
License: MIT
==> Dependencies
Build: pkg-config ✔
==> Caveats
ncurses is keg-only, which means it was not symlinked into /opt/homebrew,
because macOS already provides this software and installing another version in
parallel can cause all kinds of trouble.

If you need to have ncurses first in your PATH, run:
  echo 'export PATH="/opt/homebrew/opt/ncurses/bin:$PATH"' >> ~/.zshrc

For compilers to find ncurses you may need to set:
  export LDFLAGS="-L/opt/homebrew/opt/ncurses/lib"
  export CPPFLAGS="-I/opt/homebrew/opt/ncurses/include"

For pkg-config to find ncurses you may need to set:
  export PKG_CONFIG_PATH="/opt/homebrew/opt/ncurses/lib/pkgconfig"

[FdelMazo]

I'm not entirely sure. Does this PR helps towards adding --color-moved support? Issue #890

[ghost]

@FdelMazo
Yes, it works with color-moved option.

[vitornesello]

This MR is pretty cool. Does anyone know when it will be released?

[aberres]

Someone else just being here to ask if there are plans to get this in 😇

[pablospe]

In the meanwhile, I ended up using this shortcuts (in the .tigrc):

# Open delta with git show.
bind generic d >sh -c "DELTA_PAGER='less -RKc' git show %(commit)"

# https://github.com/jonas/tig/issues/542#issuecomment-1242039892
bind generic D @sh -c "\
  ( \
    tmux has-session -t \".{last}\" \
    && tmux respawn-pane -t \".{last}\" -k 'LESS= DELTA_PAGER=\"less -R\" git show %(commit)' \
  ) \
  || tmux split-window -l 50% 'LESS= DELTA_PAGER=\"less -R\" git show %(commit)'"

First shortcut d is a generic to see the diff in full-screen. If you are inside tmux, you could also try the D shortcut, it is more convenient since it opens a new pane and you can see the list of commits too, but usually I end up using d in general because it is full-screen and I see more. What it is missing is a way to auto-detect if you are inside tmux, and select the proper behavior, rather than having two shortcuts.

Obviously this is PR is a better solution, but for now this is a workaround that works for me. I hope this PR gets merged soon.

[pablospe]

Can this PR be re-open? @koutcher @jonas
(the author account was deleted)

Alternative I could open a new PR from my github fork. I will wait some time before creating the PR myself, in case this PR could be re-open easily .

[koutcher]

@pablospe, GH does not allow to reopen a PR from a deleted account. The latest status from the original author is already accessible in the ansi-support branch: https://github.com/jonas/tig/tree/ansi-support.

[pablospe]

In that case, I will create a new PR pointing: https://github.com/jonas/tig/tree/ansi-support.

Few questions:
* From where is the Rework after review commit is coming from, it doesn't seem to be part of this PR.
* What would be the next steps to get this PR in?

[adamency]

I'm leaving this comment here for people who want to try delta in tig now. (The author) @ulwlu's instruction were not very precise.

Installation Instructions

git clone https://github.com/jonas/tig/ 
cd tig
# Checkout branch of the project which contains the PR additions
git checkout ansi-support
make configure
# `ncurses` is almost always installable by your package manager, and almost always already installed on your system as it is needed for many TUI programs. The paths given here are the ones used by the package manager.
./configure LDFLAGS="-L/usr/lib" CPPFLAGS="-I/usr/include"
# This will install `tig` to ~/.local/bin, the recommended user path by the XDG specs. If needed, replace it by the path you want but leave out the `/bin/` part as this will be added by `make` already.
make prefix=$HOME/.local/
make install prefix=$HOME/.local/
# Configure `tig` to use `delta` in diffs
echo "set diff-highlight = \"delta\"" >> ~/.tigrc

This should work. However I have to note, (while the majority are displayed with delta) some commits are displayed with the normal diff output instead of delta's one. I don't know what's going wrong in theses cases.

[pablospe]

Let's move the discussion to:
PR #1298: Support delta along with ANSI support (continuation)

Notes:

1. Why? GH does not allow to reopen a PR from a deleted account.
2. @koutcher I didn't have permission to create a new PR from https://github.com/jonas/tig/tree/ansi-support, so I did it from my fork, hope this is ok with you. What are the next steps?