Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle process substitution in bash #666

Open
edi9999 opened this issue Jul 25, 2021 · 5 comments
Open

Correctly handle process substitution in bash #666

edi9999 opened this issue Jul 25, 2021 · 5 comments

Comments

@edi9999
Copy link

edi9999 commented Jul 25, 2021

Hi,

I would expect the following script to show the diff successfully :

delta <(printf "1\n2\n3\n")  <(printf "1\n4\n3\n")

However, it shows, on my terminal at least :

dev/fd/63 ⟶   dev/fd/62

───┐
1: │
───┘
│ 1  │pipe:[3447202]                                │    │
\ No newline at end of file
│    │                                              │ 1  │pipe:[3447204]
\ No newline at end of file

If I run

diff <(printf "1\n2\n3\n")  <(printf "1\n4\n3\n")

or

icdiff <(printf "1\n2\n3\n")  <(printf "1\n4\n3\n")

It works as expected.

@edi9999
Copy link
Author

edi9999 commented Jul 25, 2021

See this https://superuser.com/questions/1059781/what-exactly-is-in-bash-and-in-zsh as an explanation of process substition

@Kr1ss-XD
Copy link
Contributor

Kr1ss-XD commented Jul 25, 2021

Hi @edi9999 - Note that git itself doesn't handle diffs of process substitutions (and these are implemented via named pipes, and thus can't be part of a git repository anyways) :

$ git --no-pager diff <(printf "1\n2\n3\n")  <(printf "1\n4\n3\n")
diff --git 1/dev/fd/63 2/dev/fd/62
index cad0470..0d8155b 120000
--- 1/dev/fd/63
+++ 2/dev/fd/62
@@ -1 +1 @@
-pipe:[3364850]
\ No newline at end of file
+pipe:[3364852]
\ No newline at end of file

Hence, I don't think it's a delta issue, right ?

But, you can still achieve your goal with

$ diff <(printf "1\n2\n3\n")  <(printf "1\n4\n3\n") | delta

😉

@dandavison
Copy link
Owner

dandavison commented Jul 25, 2021

Hi @edi9999, to expand slightly on @Kr1ss-XD's answer: firstly, delta file_A file_B is a wrapper around git diff --no-index (since delta of course contains no diffing algorithm). But! As @Kr1ss-XD says, it seems that git does not support process substitution; see https://stackoverflow.com/questions/22706714/why-does-git-diff-not-work-with-process-substitution. However, there is an argument that this is a bug in git, since git diff --no-index /wherever/file-A /somewhere/else/file_B is AFAIK intended to perform an diff on two arbitrary files with no involvement of a git repository. And in fact, in the SO thread is a link to a 2014 patch against git that claims to fix this, but seemed never to get any response.

So, one thing we could do, perhaps, would be to enquire in the appropriate git development forum as to whether that patch is appropriate and correct, or study the problem ourselves.

Another thing we could do is add an option for users to go back to what delta was doing previously, i.e. calling diff -u instead of git diff. But there were (IMO) good reasons for using git diff instead of diff -u (reasons given in #543).

@edi9999
Copy link
Author

edi9999 commented Jul 25, 2021

The patch seems so simple on the git repository that it looks like it would not really do any harm in my humble opinion.

One reason delta could overcome this would be to detect this case and create temporary files from the file descriptors in this case, and call git diff on the temporary files in that case (created in /tmp/delta_XXXXX with a random suffix and deleted after the diff is done)

@infokiller
Copy link

My current workaround is to use temporary files like @edi9999 mentions, something like this (the full version is in a script used in my dotfiles):

# git-diff doesn't work with process substitution [1] so we're copying the
# pipe to a temporary file.
# [1] https://github.com/dandavison/delta/issues/666
_git_diff() {
  local tmpdir
  tmpdir="$(mktemp -d -t 'git_diff.XXXXXX')"
  # NOTE: The path variable in trap must be expanded here because it may not
  # be defined when the trap is ran.
  # shellcheck disable=SC2064
  trap "rm -rf -- '${tmpdir}' &> /dev/null || true" EXIT ERR INT HUP TERM
  local f files=()
  for ((i = 1; i <= $#; i++)); do
    f="${!i}"
    if [[ -p "${f}" ]]; then
      local tmpfile="${tmpdir}/arg_${i}"
      cat -- "${f}" >| "${tmpfile}"
      files+=("${tmpfile}")
    else
      files+=("${f}")
    fi
  done
  git diff --no-index --color "${files[@]}"
}

_git_diff "$@" | delta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants