delta incorrectly thinks that the input files are the same within git a hook🐛

[salmankhilji]

Within a git hook, pass two different files to delta. The exit code incorrectly indicates that the files are the same. If the same hook script is run manually, then it works correctly.

Steps to Reproduce

1. Save the following as a Bash script within a temporary directory. (Or download from here).

#!/bin/bash

set -e

write-file() (
  local -r dst="$1"; shift || return
  local -r src="$1"; shift || return

  if [[ -f "${dst}" ]]; then
    local -r tmp="$(mktemp)" && trap "rm -f '${tmp}'" EXIT || return
    cp "${src}" "${tmp}"
    echo -n 'file1: '; cat "${tmp}"
    echo -n 'file2: '; cat "${dst}"
    if delta --side-by-side --paging never --width "$(tput cols)" "${dst}" "${tmp}"; then
      echo -e "\e[36m=\e[0m Already up to date: \e[36m${dst}\e[0m"
    else
      echo -e "\n\e[33m≠\e[0m File contents changed: \e[33m${dst}\e[0m\n"
      cp -i "${tmp}" "${dst}"
    fi
  else
    cp "${src}" "${dst}" && echo -e "\e[94m✓\e[0m Successfully wrote: \e[94m${dst}\e[0m"
  fi
)

write-file foo <( sed 's/bar/baz/' foo )

2. Understand what the script does. The write-file routine takes two arguments. The first argument is the name of the destination file that we want to write. The second argument is a source file that we want to write to the destination. The routine uses delta to compare the contents of the files. If the destination file already exists and the contents of the file are different, then we ask for confirmation to overwrite an existing file. (Of course, the spew from delta is helpful in deciding if we want to overwrite the existing file.)
3. mkdir my_repo
4. cd $_
5. echo bar >foo
6. git init
7. git add .
8. git commit -m "initial commit"
9. cp ../delta-bug.bash .git/hooks/post-checkout
10. cat foo
11. git checkout -f

In Step 2, we wrote bar to the file named foo and committed the file in Step 8.

In Step 9, we then set up a post-checkout hook so our hook would get called after every checkout.

In Step 11, we expect delta to show that we are overwriting the contents bar with baz (Notice that our sed command in the script replaces bar with baz. However, we observe that delta incorrectly thinks that the files are the same because of the following in the transcript below:

= Already up to date: foo

We then manually run the Git hook. We observe that manually running the Git hook works as expected.

Screenshot

Version

$ delta --version
delta 0.12.0

[salmankhilji]

Further investigation reveals that the exit code from delta is wrong in this scenario. Both diff -u and git diff --no-index finish with an exit code of 1, whereas delta exits with 0.

I changed my write-file routine slightly to work around that. So if I call delta like this:

diff -u "${dst}" "${tmp}" | delta --side-by-side --paging never --width "$(tput cols)"

and then check for ${PIPESTATUS[0]}, then the script works correctly. One can print the entire {PIPESTATUS[@]} array and confirm that diff finished with an exit code of 1 whereas delta with 0.

See discussion on #626.

#!/bin/bash

set -e

write-file() {
  local -r dst="$1"; shift || return
  local -r src="$1"; shift || return

  if [[ -f "${dst}" ]]; then
    local -r tmp="$(mktemp)" && trap "rm -f '${tmp}'" RETURN || return
    cp "${src}" "${tmp}"
    diff -u "${dst}" "${tmp}" && echo SAME || echo DIFFERENT
    git diff --no-index "${dst}" "${tmp}" && echo SAME || echo DIFFERENT
    diff -u "${dst}" "${tmp}" | delta --side-by-side --paging never --width "$(tput cols)"
    if [ ${PIPESTATUS[0]} -eq 0 ]; then
      echo -e "\e[36m=\e[0m Already up to date: \e[36m${dst}\e[0m"
    else
      echo -e "\n\e[33m≠\e[0m File contents changed: \e[33m${dst}\e[0m\n"
      cp $([[ $- == *i* ]] && echo -i ]]) "${tmp}" "${dst}"
    fi
  else
    cp "${src}" "${dst}" && echo -e "\e[94m✓\e[0m Successfully wrote: \e[94m${dst}\e[0m"
  fi
}

write-file foo <( sed 's/bar/baz/' foo )

[dandavison]

Hi @salmankhilji, can we summarize what you're proposing as follows?

$ diff -u <(echo a) <(echo a) | (delta > /dev/null; echo $?)
0  # this is correct; no difference so exit code 0

$ diff -u <(echo a) <(echo b) | (delta > /dev/null; echo $?)
0 # you are suggesting that this is incorrect; files are different so exit code should be 1

[salmankhilji]

I'm afraid not. I suppose mentioning issue #626 was a mistake as that had to do with delta not working correctly with fds.

In summary if delta is invoked from within a Git hook with two file name arguments, then the hook runs within a modified environment such that delta incorrectly reports the exit code on regular files (not fds). Running the exact same command outside a Git hook, (i.e. manually by hand), works as expected.

In the repro steps filed above, we write the word bar to a file that is version controlled. Then we setup a post-checkout hook, which replaces the word bar with baz within the version controlled file. Now every time we execute:

git clean -fxd && git checkout -f

we expect to see side-by-side output similar to the one shown within the screenshot towards the end. That is because we clean up the repo and the post-checkout hook unconditionally makes the word replacement immediately thereafter. Since the before and after are different, we should see the side-by-side spew.

However, if you look closely, I manually ran the Git hook by hand at the very end within the screenshot. The second to the last execution was from within the Git hook itself and incorrectly produced the output = Already up to date.

[dandavison]

Hi @salmankhilji -- I know that you have gone to a lot of trouble explaining the situation with git hooks above, but I feel that it must be possible to simplify this! After all, delta is a command line executable whose inputs are (off the top of my head):

• stdin
• arguments
• git config, but not if you supply --no-gitconfig
• env vars

Can you provide a minimal self-contained example of a delta command that produces incorrect output or an incorrect exit code? I think that it should be possible to do this without needing to consider delta in the context of a git hook, right? Just a one line command supplying arguments and/or stdin and/or env vars.

I'm afraid not. I suppose mentioning issue #626 was a mistake as that had to do with delta not working correctly with fds.

The point of my example wasn't anything related to file descriptors or process substitution -- I just used <(echo a) out of convenience (since it works with diff). We can change my example as follows:

$ echo a > a
$ echo b > b
$ diff -u a a | (delta > /dev/null; echo $?)
0 # you're saying this is correct; no difference so exit code 0
$ diff -u a b | (delta > /dev/null; echo $?)
0 # you are suggesting that this is incorrect; files are different so exit code should be 1

What I'm trying to do here is distill what you wrote above down to its essence:

So if I call delta like this:

diff -u "${dst}" "${tmp}" | delta --side-by-side --paging never --width "$(tput cols)"

and then check for ${PIPESTATUS[0]}, then the script works correctly. One can print the entire {PIPESTATUS[@]} array and confirm that diff finished with an exit code of 1 whereas delta with 0.

[salmankhilji]

@dandavison :

Take 2: this is as simple as I can make it.

1. Download this script and execute it. The script creates a new Git repo named my_repo in the current directory.
2. cd my_repo.
3. Manually invoke delta:

   $ .git/hooks/post-checkout 
   SAME
   DIFFERENT
   

4. Invoke delta from within Git:

   $ git checkout -f
   SAME
   SAME
   

The expected output in Step 4 should be exactly the same as Step 3.

The script also sets up the post-checkout with the following contents:

#!/bin/bash

if delta bar bar >/dev/null; then
  echo SAME
else
  echo DIFFERENT
fi

if delta bar baz >/dev/null; then
  echo SAME
else
  echo DIFFERENT
fi

When Git runs the script, the environment is different. I manually recreated the exact same environment, and, disappointingly, the results are still the same:

$ GIT_EXEC_PATH=/usr/lib/git-core GIT_PREFIX= PATH=/usr/lib/git-core:$PATH .git/hooks/post-checkout 
SAME
DIFFERENT

(This is essentially Step 3 above with the environment that Git runs the hooks within.)

[dandavison]

Thanks, I can reproduce that. Nice mystery! It can't be too complicated what's going wrong there but not immediately obvious to me. I'll look into it if you don't get to it first (e.g. simply by putting print statement in the Rust code).

[dandavison]

This occurs because delta only operates in diff mode (delta a b) when it is used interactively from a terminal:

delta/src/main.rs

Line 134 in 1403fe2

if atty::is(atty::Stream::Stdin) {

Thus a minimal reproduction is echo | delta bar baz

I haven't thought through yet whether this should be changed.

[dandavison]

#995 changes things so that delta runs in "diff mode" iff it receives two positional arguments (previously it only tried diff mode when its input was a tty).

Can anyone think of any way this will go wrong / why I was doing it the previous way? The new way feels more correct: if there are two positional arguments then the author must have intended diff mode.

When using delta as a git pager, or a viewer for input on stdin, I can't think of a situation where the user would supply two positional arguments -- anyone else? We need to be slightly careful since it works today, so the change would break behavior for anyone who's using it today for some reason.

[salmankhilji]

#995 changes things so that delta runs in "diff mode" iff it receives two positional arguments (previously it only tried diff mode when its input was a tty).

Can anyone think of any way this will go wrong / why I was doing it the previous way? The new way feels more correct: if there are two positional arguments then the author must have intended diff mode.

When using delta as a git pager, or a viewer for input on stdin, I can't think of a situation where the user would supply two positional arguments -- anyone else? We need to be slightly careful since it works today, so the change would break behavior for anyone who's using it today for some reason.

Thanks for the fix! For my educational purposes, I'm curious as how the fix, which is based on the number of arguments passed, would compare with checking for certain $GIT* env vars that may only be set when delta is invoked as a pager from within Git?

[dandavison]

I'm curious as how the fix, which is based on the number of arguments passed, would compare with checking for certain $GIT* env vars that may only be set when delta is invoked as a pager from within Git?

I'd say:

We don't want delta bar baz to behave differently when it's involved in a git hook per se: that would be an odd sort of special casing that would suggest that something's wrong with the design. Instead, I think the real problem is that delta bar baz only works when its input is a terminal. But there's no clear reason for that, and your example shows that sometimes people want to use delta bar baz when the input isn't a terminal. I think I could imagine other situations in a shell script where stdin is a pipe, and someone is using delta bar baz to display a diff.

[dandavison]

In other words, delta should be a normal Unix executable whose behaviour is predictable given its arguments and standard input. Its behavior shouldn't depend on whether it has been invoked by git, or is involved a git hook.

(delta does violate that a bit! But hopefully justifiably.)

[ristillu]

@dandavison this timely fix unbeknownst to me saved a proof of concept I implemented yesterday. Details:

https://www.reddit.com/r/neovim/comments/tfcmyh/function_for_creating_local_diffs_in_a_scratch

Thanks!

[dandavison]

this timely fix unbeknownst to me saved a proof of concept I implemented yesterday

Excellent, glad to hear that! The special behavior based on tty detection wasn't the right thing to be doing here; I'm not sure why I was doing it :/