dandavison / delta

A syntax-highlighting pager for git, diff, grep, and blame output
https://dandavison.github.io/delta/
MIT License
21.43k stars 361 forks source link

🐛 `rename` and `copy` operations produce empty output #1548

Open RuRo opened 9 months ago

RuRo commented 9 months ago

Running this reproduction script:

#!/usr/bin/env bash

cd "$(mktemp -d)"

export GIT_CONFIG_GLOBAL=/dev/null
export GIT_CONFIG_NOSYSTEM=1
export PAGER=cat

git -c init.defaultBranch=master init . >/dev/null
git config user.email "testing@example.com"
git config user.name "Best Tester"

echo -e "something\nblah\nblah\nlong\nfile" > here
echo -e "different\nlorem\nipsum\ndolor\nsit\namet" > there
git add .
git commit -m "initial commit" >/dev/null

mv here moved
cp there copied
git add .
git commit -m "second commit" >/dev/null

echo "==== regular diff ===="
git --no-pager          diff -C -C HEAD~ HEAD
echo "======================"

echo

echo "==== $(delta --version | tr -d '\n') ===="
git -c core.pager=delta diff -C -C HEAD~ HEAD
echo "======================"

Produces the following output:

==== regular diff ====
diff --git a/there b/copied
similarity index 100%
copy from there
copy to copied
diff --git a/here b/moved
similarity index 100%
rename from here
rename to moved
======================

==== delta 0.16.5 ====
======================
RuRo commented 9 months ago

This looks like it might be a regression from #392

imbrish commented 4 months ago

Hey! Piping the diff you provided produces correct output in current main. You may want to try the same?

image

RuRo commented 4 months ago

Okay, so this is a separate issue, but it seems that delta doesn't respect the GIT_CONFIG_GLOBAL and GIT_CONFIG_NOSYSTEM environment variables which is all kinds of wrong. So my "minimal" reproduction didn't work because delta was still reading my gitconfig.


After figuring that out, I was finally able to find the problematic option: file-style = omit. You could argue that this is working as intended, however this behaviour doesn't really make sense. My understanding is that the main reason for the existence of file-style = omit is to complement hunk-header-style = file line-number, but since renames and copies don't produce hunks, then it doesn't make sense to completely omit this information.

So IMHO, file-style = omit should only omit "empty" file section headers and keep the rename/copy information. Or at the very least there should be an option that does that.

Or alternatively, rename/copy operations should be represented by their own pseudo-hunks when file-style is set to omit.

dandavison commented 4 months ago

it seems that delta doesn't respect the GIT_CONFIG_GLOBAL and GIT_CONFIG_NOSYSTEM environment variables

What delta version? I thought that this was a libgit2 issue that had been solved.

RuRo commented 4 months ago

I patched the nixpkgs derivation of delta to compile from dcae5bcc2428d1fb6f5ff7b9cddd7f268d9a3735 (main at the time of writing). Though delta --version itself still reports the version as 0.16.5.

P.S. looking at the build logs, I can see that libgit2-sys v0.15.2+1.6.4 was compiled/used by cargo.

P.P.S. looking at the upstream libgit2 repo, it seems that the relevant PR https://github.com/libgit2/libgit2/pull/6544 was first released in version 1.7.0.

dandavison commented 4 months ago

Thanks @RuRo! I update git2 in main: https://github.com/dandavison/delta/pull/1647

RuRo commented 4 months ago

Great. Circling back to the original issue, what do you think about the current behaviour of file-style = omit + hunk-header-style = file line-number for renames/copies?

imbrish commented 4 months ago

For diffs of binary files, I was thinking about keeping the line Binary files a/foo and b/bar differ. Similarly standalone diff can report Files foo and bar are identical. Such line is not present for renames and copies of git diff, but I think it may be useful to have it.

@dandavison what do you think about adding such line for otherwise empty diffs? It could be hidden behind an option like --show-status-line that would be disabled by default, and perhaps enabled automatically along with file-style = omit.

@RuRo would something like that work in your case?

RuRo commented 4 months ago

@RuRo would something like that work in your case?

I think so. I guess, the specifics could use some bikeshedding, but I'd generally be okay with any solution that could show this information without showing the redundant (when used with hunk-header-style = file line-number) file "section" headers.