Implement --diff-args

[dandavison]

Allow extra arguments to git diff to be passed from the delta commands line, as suggested by @calestyo in #1644 (comment)

E.g.

delta --diff-args=-U99 /tmp/a /tmp/b
delta -@=-U99 /tmp/a /tmp/b

Note that the = is required in this case, due to the - that comes next.

[dandavison]

This is almost ready.

An example of how delta reports an external process error:

$ delta -@-U-1 <(cat b.rs) src/paint.rs
diff: invalid context length '-1'
diff: Try 'diff --help' for more information.
'diff' process failed with exit status 2. Command was: diff -U3 -U-1 -- /dev/fd/11 src/paint.rs

One issue is that I see that git does now support process substitution https://stackoverflow.com/questions/22706714/why-does-git-diff-not-work-with-process-substitution Which is great, but now it would be nice to improve the fallback logic. We could detect the git version as we do for less, or we could try git diff and fall back to diff in the case of a certain error exit code and via_process_substitution(file) returning true. We could address this in a separate PR.

[dandavison]

@th1000s I think this PR is almost there. I would like the following to be true:

if a user has git >= 2.42 then delta will use git diff, even if they use process substitution.

(So that their git config is honored and they can use all the git diff options).

Instead of trying to detect the user's git version, the current state of this PR always tries git diff and falls back to diff if it seems to have failed due to process substitution. After all, this problem should become rarer and rarer as people upgrade git.

This seems to be working on the two Mac laptops I tested on (one with old git and one new), since with the old git I am getting an error (not merely printing of filenames):

$ git diff --no-index <(cat [b.rs](http://b.rs/)) src/[paint.rs](http://paint.rs/)
error: /dev/fd/11: unsupported file type
fatal: cannot hash /dev/fd/11
$ echo $?
128
$ git --version
git version 2.39.3 (Apple Git-146)

However it sounds like from your comment that you encountered incorrect output with a success (< 2) exit code:

When called as delta <(echo foo) <(echo bar), then git < 2.42 just prints the diff of the filenames which were created by the process substitution and does not read their content.

If so, then I think my PR needs to do a bit more.

Any thoughts? (and any chance you can test out this PR??)

[th1000s]

Indeed, on Linux git sees e.g. /proc/self/fd/16, then reads and diffs nonsense like pipe:[15699060]. But I would just document that this requires git v2.42 and not call and parse git --version first.

[dandavison]

Indeed, on Linux git sees e.g. /proc/self/fd/16, then reads and diffs nonsense like pipe:[15699060]. But I would just document that this requires git v2.42 and not call and parse git --version first.

Just to make sure I understand, are you saying we should

1. just not bother calling diff at all, now that git supports process substitution? If the user is trying process substitution then they should use a recent git that supports it.

or

2. that we should now always invoke git twice, i.e. the first time call git to parse the version and the second time call git or diff to compute the diff?

[th1000s]

I'd be okay with 1. because this will work everywhere.. eventually. But if 2. is not too much work now (and I remember that parsing less --version takes just 4ms) that would be nicer of course.

[dandavison]

Switched to parsing the git version. I think this PR is ready now.

[th1000s]

Why not #[derive(PartialEq)] (and Debug for good measure)? Then just == is possible.

[dandavison]

Done, thanks.

[th1000s]

Indeed, on Linux git sees e.g. /proc/self/fd/16, then reads and diffs nonsense like pipe:[15699060]. But I would just document that this requires git v2.42 and not call and parse git --version first.

[dandavison]

This isn't very well tested, the excuse being the dependency on the presence and installed versions of external software.

Some commands I've used in local testing

git show HEAD~100:src/paint.rs > b.rs

delta --diff-args='-U1 --no-color' <(cat b.rs) src/paint.rs
delta <(cat b.rs) src/paint.rs

# This works on a machine with git >= 2.42, since it calls git diff,
# which tolerates a negative value
delta --diff-args='-U-1 --no-color' <(cat b.rs) src/paint.rs

# But on a machine with 2.39.3 it yields the following,
# because the process substitution cause it to use diff, which does not tolerate
# the negative value

'diff' process failed with exit status 2. Command was: diff -U3 -U-1 --no-color -- /dev/fd/11 src/[paint.rs](http://paint.rs/) 

(exit code 2)

$ delta -@='--no-index --nonsense' <(cat b.rs) src/paint.rs
error: unknown option `nonsense'
'git' process failed with exit status 129. Command was: git diff --no-index --color --no-index --nonsense -- /dev/fd/11 src/paint.rs

[dandavison]

FWIW, this takes 6.5ms on an M2 Mac (hyperfine's estimate based on 1000 calls in a loop), so slightly slower than the 4ms you found for less. (But, if neither file uses process substitution then it's not done at all) EDIT: actually, it's now always doing version detection; it uses it to determine whether git is installed at all.

[th1000s]

Looks good. One mistake I made when testing was using -@U1 instead of -@-U1, maybe the former can be silently converted, all (git) diff arguments other than the files start with a dash anyhow.

$ delta -@U1 <(cat a) b
diff: extra operand 'b'
diff: Try 'diff --help' for more information.
'diff' process failed with exit status 2. Command was: diff -U3 U1 -- /proc/self/fd/16 b

[dandavison]

One mistake I made when testing was using -@U1 instead of -@-U1, maybe the former can be silently converted, all (git) diff arguments other than the files start with a dash anyhow.

Good call, that seems like a nice usability improvement. Done. (For the first argument only)

[dandavison]

There's still this minor mystery, as was the case 3 years ago: #546 (comment)

We have to skip this case

#[case("/dev/null", "/dev/null", ExpectDiff::No)]

Because it fails on (ubuntu-latest, i686-unknown-linux-gnu, true), but passes on all other builds.

I.e., apparently, on (ubuntu-latest, i686-unknown-linux-gnu, true) our test sees git diff --no-index /dev/null /dev/null returning exit status 1 (I think it's resolving to git diff; until my last commit there was an error if git wasn't installed)

[dandavison]

One final change: it now works if git isn't available. This means we're doing git version lookup in all code paths (let me know if you question wisdom of this or have a better idea). (Incidentally, I was misled by the name of grep_cli::resolve_binary -- it does nothing on non-Windows).

[th1000s]

.. all code paths when diffing, that seems okay. And we will just leave the /dev/null mystery for later :)

[dandavison]

Great, thanks for all the reviewing and discussion @th1000s.