Fix empty diff preview on renames

[carlfriedrich]

Check list

• [x] I have performed a self-review of my code
• [x] I have commented my code in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation

Description

This had been fixed before in #189, but obviously the patch broke support for whitespaces in file names. That was fixed in #204, which in turn broke support for renames again.

Unfortunately we cannot support both cases together with the existing xargs implementation, because the arguments are interpreted either as one single file name (which breaks renames, because we have two files passed in this case) or as separate file names (which breaks whitespace support, because git would interpret each word of the file name as a separate file).

This patch moves away from xargs and stores the file names in a bash array instead. This makes it possible to pass them to git diff quoted one by one using the "${array[@]}" notation.

Test environment:

mkdir test && cd test
git init
touch file && git add file
git commit -m "Initial commit"

touch newfile && git add newfile
touch "file with spaces" && git add "file with spaces"
git mv file "another file"

git forgit diff --staged

Before:

After:

Type of change

• [x] Bug fix
• [ ] New feature
• [ ] Refactor
• [ ] Breaking change
• [ ] Documentation change

Test environment

• Shell
  • [x] bash
  • [ ] zsh
  • [ ] fish
• OS
  • [x] Linux
  • [ ] Mac OS X
  • [ ] Windows
  • [ ] Others:

[carlfriedrich]

@cjappl and @wfxr Can you check if this works for you?

[cjappl]

No go for me at least, may be an OSX thing?

[carlfriedrich]

@cjappl Ah shit, which version of bash is the default on Mac?

[carlfriedrich]

@cjappl I updated the branch with a potential fix for MacOS. Can you check again? Thanks for your help!

[cjappl]

Error fixed!! Here is my output, doesn't look exactly the same as yours yet

[carlfriedrich]

@cjappl Thanks for testing! That still does not look correct, unfortunately. Seems like I have to check on a Mac myself for debugging.

[cjappl]

If that’s too much of a pain I can look into it, it just may be a little bit!

On Thu, Feb 2, 2023 at 12:26 AM, Tim @.***> wrote:

@.***(https://github.com/cjappl) Thanks for testing! That still does not look correct, unfortunately. Seems like I have to check on a Mac myself for debugging.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[carlfriedrich]

@cjappl I pushed another commit. Can you check again please?

[cjappl]

No dice still

[carlfriedrich]

@cjappl Could you do me a favor and give me the output of the following two commands on your system?

bash --version
bash -c 'declare -a arr; arr+="one"; echo ${arr[@]}'

Thank you!

[carlfriedrich]

@cjappl And while you're at it, could you also check the behaviour on your machine with the updated branch?

[carlfriedrich]

@cjappl I pushed another version with a potential fix here: https://github.com/carlfriedrich/forgit/tree/fix-git-diff-on-renames-without-array-plus-syntax

Maybe you can check that as well? I'm very curious if any of these fix the problem in your environment.

Thanks for your help!

[cjappl]

bash --version
bash -c 'declare -a arr; arr+="one"; echo ${arr[@]}'

bash --version bash -c 'declare -a arr; arr+="one"; echo ${arr[@]}' GNU bash, version 3.2.57(1)-release (arm64-apple-darwin21) Copyright (C) 2007 Free Software Foundation, Inc. one

[cjappl]

@cjappl And while you're at it, could you also check the behaviour on your machine with the updated branch?

((9b342177))> mkdir test && cd test
                                                           git init
                                                           touch file && git add file
                                                           git commit -m "Initial commit"

                                                           touch newfile && git add newfile
                                                           touch "file with spaces" && git add "file with spaces"
                                                           git mv file "another file"
Initialized empty Git repository in /Users/chrisapple/code/personal/forgit/test/.git/
[main (root-commit) 2b798f0] Initial commit
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file
[I]|14:40|chrisapple@Chriss-MBP ~/c/p/f/test (main)> gd --staged
unknown action: execute(while IFS=
 read f; do files

[cjappl]

fix-git-diff-on-renames-without-array-plus-syntax

((9b342177))> git checkout carlfried/fix-git-diff-on-renames-without-array-plus-syntax
Previous HEAD position was 9b34217 Export default shell for fzf commands
HEAD is now at ff737b8 Do not use "+" syntax for bash arrays
[I]|14:41|chrisapple@Chriss-MBP ~/c/p/forgit ((ff737b85))> rm -rf test/
[I]|14:41|chrisapple@Chriss-MBP ~/c/p/forgit ((ff737b85))> mkdir test && cd test
                                                           git init
                                                           git add file
                                                           git commit -m "Initial commit"

                                                           touch newfile && git add newfile
                                                           touch "file with spaces" && git add "file with spaces"
                                                           git mv file "another file"
Initialized empty Git repository in /Users/chrisapple/code/personal/forgit/test/.git/
[main (root-commit) ca4641c] Initial commit
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file
[I]|14:41|chrisapple@Chriss-MBP ~/c/p/f/test (main)> gd --staged
unknown action: execute(while IFS=
 read f; do files=(${files[@]} $f); done < <(echo {} | sed 's/.*] *//' | sed 's/  ->  /\n/') && cd '/Users/chrisapple/code/personal/forgit/test' && git diff --color=always -U10 --staged -- "${files[@]}" | bash -c 'delta' | LESS='-r' less)

Still no luck :( Happy to keep testing if you keep throwing them over to me.

[carlfriedrich]

@cjappl Thanks a lot for your help!

bash --version bash -c 'declare -a arr; arr+="one"; echo ${arr[@]}' GNU bash, version 3.2.57(1)-release (arm64-apple-darwin21) Copyright (C) 2007 Free Software Foundation, Inc. one

Okay, so you definitely have the latest MacOS bash version and it supports the "+=" array syntax.

Then I don't see why you're getting this error when I use this syntax:

unknown action: execute(while IFS= read f; do files

The parsing of the execute string seems to stop on the "+" sign for no obvious reason.

On the alternative branch I replaced the "+=" syntax. This makes at least the parsing of the execute string complete:

unknown action: execute(while IFS= read f; do files=(${files[@]} $f); done < <(echo {} | sed 's/.] //' | sed 's/ -> /\n/') && cd '/Users/chrisapple/code/personal/forgit/test' && git diff --color=always -U10 --staged -- "${files[@]}" | bash -c 'delta' | LESS='-r' less)

Still we're getting the unknown action here, which I really don't understand.

I created a docker container with bash 3.2.57 and cannot reproduce this behavior. So I assume that the reason for this must be some other Mac specific package. Do you know if there's any other package that is significantly different compared to Linux systems?

[cjappl]

@carlfriedrich the problem seems to be with the newline, that's the thing preventing it from working and erroring

However, this results in the same output as above, it still doesn't match your original output.

Still looking into this a little today, I'll let you know if I get anywhere interesting

[cjappl]

Maybe this is a difference in git versions or something? Check this out:

> git diff --staged

renamed: file ⟶   another file

added: file with spaces

added: newfile

> git diff --staged -- another\ file

added: another file

> git diff --staged -- file

removed: file

But when I pass in both file and another file, I get what we want

> git diff --staged -- file another\ file

renamed: file ⟶   another file

It seems like get_files isn't actually grabbing the right thing. It needs to pass in both files

Still looking. Let me know if any of this is looking familiar

[cjappl]

If I change this to the following:

    preview_cmd="$get_files && \\\"\${files[@]}\\\""

Just to see what comes out,

It looks like it's just passing the result in straight up, meaning that the result is something like:

> git diff --staged -- file another file

removed: file

How do we get this to end up like

> git diff --staged -- "file" "another file"

renamed: file ⟶   another file

?

[carlfriedrich]

Hey @cjappl, thanks for checking this again.

But when I pass in both file and another file, I get what we want

Yes, that's exactly what I'm trying to do here. We have to pass both files as separate arguments to the git diff call in order to make it work.

If I change this to the following:

    preview_cmd="$get_files && \\\"\${files[@]}\\\""

Just to see what comes out,

It looks like it's just passing the result in straight up.

The ${files[@]} syntax expands the files array to a quoted list of its elements. That's what we need here to achieve the above, we "only" have to get the two files separately into the array. That turns out to be the tricky part.

the problem seems to be with the newline, that's the thing preventing it from working and erroring

So without the newline you at least don't get an error? That's interesting. The newline is important, though, for the correct function in this case, because I am replacing the --> string literal with a newline in the sed call:

echo {} | sed 's/.*] *//' | sed 's/  ->  /\\\n/'

...which gives a list of two files in our test scenario:

file
another file

...and then I try to read this list into the array:

while IFS="$'\n'" read f; do files=("'"${files[@]}" "\$f"'"); done

If it's the newline character causing the trouble, however, we might simply use a different delimiter, as I have chosen this arbitrarily. I will give it a try and update the branch accordingly.

[carlfriedrich]

@cjappl Thank you for your support, your comments really helped. Using a null-terminated list I was able to go back to the original xargs implementation, which makes the code so much easier. I added a block of explicit documentation in the code now for eventual future maintenance. I am very confident that this should work, because we're not using bash arrays anymore. Can you check again please?

[cjappl]

Ok, this didn't work for me (d*mmit OSX...) but it got us close. THIS VERSION WORKS FOR ME:

get_files="echo {} | sed 's/.*] *//' | sed 's/  ->  /\\\n/' | sed 's/$/\\\n/' | tr '\\\n' '\\\0'"

Basically translating to newlines, then translating to nulls. Just changing this line works as intended.

The latest code you posted (9f50100) just fails with an empty window:

If I change the following line:

    preview_cmd="cd '$repo' && $get_files | xargs -0 ls | $_forgit_diff_pager"

to just "ls" the files, this is what happens:

It appears either the /0 isn't being inserted correctly, or isn't being interpreted by xargs correctly.

Keep me posted on if you want me to try a new one. We are so close!!

[carlfriedrich]

@cjappl Oh yeah, we're getting there!

Ok, this didn't work for me (d*mmit OSX...) but it got us close. THIS VERSION WORKS FOR ME:

get_files="echo {} | sed 's/.*] *//' | sed 's/  ->  /\\\n/' | sed 's/$/\\\n/' | tr '\\\n' '\\\0'"

Okay, that has probably to do with MacOS's sed version. I think we had issues with that before, and as far as I remember it comes from a different package on MacOS compared to Linux.

Can you quickly check if sed -E or sed -r make any difference when using \x0 directly?

[cjappl]

    get_files="echo {} | sed 's/.*] *//' | sed -E 's/  ->  /\\\x0/' | sed -E 's/$/\\\x0/'"

Results in blank output in the screen (❌ )

Same issue with -r (my man page says under the hood they are the same flag, and they behave the same way in practice)

[carlfriedrich]

@cjappl Thanks for checking. Makes sense, the BSD sed implementation OSX is using, does not appear to support \x0 syntax at all.

I updated the branch accordingly. But I had to remove the middle sed 's/$/\\\n/' part (which added a newline at the end), because that led to another empty filename being passed to git diff now, which produced an error in my environment:

Without this part it works. Can you check if it still works for you?

[cjappl]

Testing your most recent commit (7cab014f) WORKS! 🎉

Please confirm the following screenshots are as you expect, but LGTM

Good job powering through this one @carlfriedrich - really messy one.

[carlfriedrich]

@cjappl Yay! Thanks for your help on this!