Fix issue where diff preview was broken on mac #365

cjappl · 2024-03-11T18:28:07Z

Closes #362

I need one of my favorite linux pals (@sandr01d , @wfxr , @carlfriedrich ) to ensure this still works on your favorite linux distro.

The test cases I would recommend:

File with normal name
File with spaces in the name
File with brackets in the name (my[test]file.txt)
File that has been renamed.

I have checked on my mac and docker image and everything seems happy.

Check list

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

Description

Type of change

Bug fix

Test environment

Shell
- bash
- zsh
- fish
OS
- Linux
- Mac OS X
- Windows
- Others:

cjappl · 2024-03-11T18:29:25Z

bin/git-forgit

@@ -137,10 +137,10 @@ _forgit_diff() {
    #   oldfile\0
    # We have to do a two-step sed -> tr pipe because OSX's sed implementation does
    # not support the null-character directly.
-    get_files="echo {} | sed 's/\\\\s*\\\\[.]\\\\s*//' | sed 's/  ->  /\\\n/' | tr '\\\n' '\\\0'"
+    get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z]\\\\][[:space:]]*//' | sed 's/  ->  /\\\n/' | tr '\\\n' '\\\0'"


For this, I changed it to "Remove everything that looks like a capital letter in brackets and a bunch of spaces following it"

Please sanity check if you agree that this makes sense.

I was seeing the pattern

[M] somefile.txt
[M] another[file].txt

This does the trick for me:

get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z0-9]*\\\\][[:space:]]*//' | sed 's/ -> /\\\n/' | tr '\\\n' '\\\0'"

The previous [A-Z] did not match the R100 for renamed files.

carlfriedrich · 2024-03-12T07:29:27Z

I can confirm that files with normal name, files with spaces in the name and files with brackets in the name all work on this branch.
Renamed files, however, are not correctly diffed. The diff preview displays the file as if it was added. This is because the get_files code does not correctly extract the two filenames from the line. We can check that by setting the preview_cmd to this:

preview_cmd="$get_files | xargs -0 -I% echo '%'"

The preview displays a list of the contained files in the change then. For a modified file, it displays just the single filename. For a renamed file, it should display both filenames, the old and the new one. In my case it displays:

[R100]  old_filename
new_filename

So the [R100] is not correctly removed from the beginning.

As I found out, this is the case since #354 already, so it wasn't introduced in this PR. However, I think we should fix it in this PR.

carlfriedrich · 2024-03-12T07:38:30Z

bin/git-forgit

@@ -137,10 +137,10 @@ _forgit_diff() {
    #   oldfile\0
    # We have to do a two-step sed -> tr pipe because OSX's sed implementation does
    # not support the null-character directly.
-    get_files="echo {} | sed 's/\\\\s*\\\\[.]\\\\s*//' | sed 's/  ->  /\\\n/' | tr '\\\n' '\\\0'"
+    get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z]\\\\][[:space:]]*//' | sed 's/  ->  /\\\n/' | tr '\\\n' '\\\0'"


This does the trick for me:

get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z0-9]*\\\\][[:space:]]*//' | sed 's/ -> /\\\n/' | tr '\\\n' '\\\0'"

The previous [A-Z] did not match the R100 for renamed files.

carlfriedrich · 2024-03-12T07:40:31Z

And I am extremely looking forward to covering these cases in a unit test. :-)

cjappl · 2024-03-12T12:55:08Z

@carlfriedrich 100% on the unit tests, so many regressions in this area..

Can you send me the exact "steps to repro" for your R100 case? I will run it on my mac and ensure your xargs suggestion works then update this PR

carlfriedrich · 2024-03-12T12:57:55Z

@cjappl Just call git mv old_filename new_filename and then forgit diff on it.

cjappl · 2024-03-12T13:43:09Z

@carlfriedrich would you please run through the test cases again. I took your suggestion, plus one extra modification that was necessary (.* around the arrow, matching get_file).

sandr01d · 2024-03-12T20:56:28Z

bin/git-forgit

@@ -137,10 +137,10 @@ _forgit_diff() {
    #   oldfile\0
    # We have to do a two-step sed -> tr pipe because OSX's sed implementation does
    # not support the null-character directly.
-    get_files="echo {} | sed 's/\\\\s*\\\\[.]\\\\s*//' | sed 's/  ->  /\\\n/' | tr '\\\n' '\\\0'"
+    get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z0-9]*\\\\][[:space:]]*//' | sed 's/.*->  //' | tr '\\\n' '\\\0'"


This still does not work for me when diffing renamed files that were added. To reproduce:

mv LICENSE LICENSE1

git add .

gd --staged

The preview will look like LICENSE1 has been added. This can be fixed like this:

Suggested change

get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z0-9]*\\\\][[:space:]]*//' | sed 's/.*-> //' | tr '\\\n' '\\\0'"

get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z0-9]*\\\\][[:space:]]*//' | sed 's/.*-> //' | tr '\\\n' '\\\0'"

get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z0-9]*\\\\][[:space:]]*//' | sed 's/ -> /\\\n/' | tr '\\\n' '\\\0'"

The second sed command is what has changed here. I realized that I accidentally modified it with #354. Sorry you for that and another strong yes for unit tests.

I think I see what you're seeing, but your suggestion doesn't work on my mac. In my current code I have on this branch, I get this:

Your suggestion leads to a blank preview (on both ubuntu and mac):

That is using this line:

get_files="echo {} | sed -e 's/^[[:space:]]*\\\\[[A-Z0-9]*\\\\][[:space:]]*//' | sed 's/ -> /\\\n/' | tr '\\\n' '\\\0'"

I realize my "working" version says "file added" at the top, but I'm not sure how it is supposed to look. Can you post a screenshot of what the working version looks like on your machine?

Without my change it looks like the first one. With the change, it looks like this for me:

That the diff is empty is correct, since the content of the file did not change. Could you check how this behaved before #354 for you? My suggestion simply adds back the code that has been replaced in #354 by accident, so it surprises me that this would break something. What happens when you add a line to the file aswell? Looks like this for me:

Alright, I cannot explain it but after going in and copy/pasting from the original pre-#345 version (which is identical to the code you pasted above) I can confirm this works on both Mac and Ubunutu. I pushed to this branch.

Please test the test cases again :) Thanks for the help as always

sandr01d · 2024-03-12T21:04:01Z

I also noticed that files with a leading white space fail to get a preview. This seems to have been the case before your changes and even before #354. So we could leave this open for a follow up PR IMO. Other than this and the change I've suggested above, this is working fine for me.

sandr01d

This works for me as expected now. That is, there are still some file names that do not work, but that never actually worked. The ones I've found so far are file names that start with a white space (probably because we remove leading white space in the sed command) and files that have a backslash in their name (probably because it being interpreted as an escape character).
I think we can only fixes those by implementing a completely new approach, so clearly out of scope for this PR. So approved from my side.
Thanks @cjappl!

carlfriedrich · 2024-03-14T13:41:13Z

Works for me!

carlfriedrich · 2024-03-14T13:44:20Z

@cjappl Please add a note in the final commit message that this also fixes diff preview for renames, which was broken since forgit-24.03.0.

carlfriedrich · 2024-03-14T14:00:12Z

@cjappl Didn't read my last comment? That's why I prefer squashing before merging. :-/

cjappl · 2024-03-14T14:04:10Z

@cjappl Didn't read my last comment? That's why I prefer squashing before merging. :-/

Sorry I missed it, I had the page loaded before merging and I didn't see it.

cjappl · 2024-03-15T17:47:01Z

@carlfriedrich I'm going to do a release to get this out to the package managers, do you want me to mention this in the release notes: "fixes diff preview for renames"

Currently the "auto generated" release notes are:

## What's Changed
* Delete brew workflow by @cjappl in https://github.com/wfxr/forgit/pull/360
* Fix issue where diff preview was broken on mac by @cjappl in https://github.com/wfxr/forgit/pull/365


**Full Changelog**: https://github.com/wfxr/forgit/compare/24.03.1...24.03.2

Normally I haven't varied from the auto generated, but in this case I could add a line to be:

## What's Changed
* Delete brew workflow by @cjappl in https://github.com/wfxr/forgit/pull/360
* Fix issue where diff preview was broken on mac by @cjappl in https://github.com/wfxr/forgit/pull/365
* Fixed regression where diff preview for renames was broken by @cjappl in https://github.com/wfxr/forgit/pull/365


**Full Changelog**: https://github.com/wfxr/forgit/compare/24.03.1...24.03.2

carlfriedrich · 2024-03-15T19:41:54Z

@cjappl Yep, that would be great, thanks a lot!

cjappl · 2024-03-15T19:50:26Z

@carlfriedrich No problem. Apologies again on missing your comment. I'll do that release now.

cjappl added 5 commits March 11, 2024 11:01

Add temporary test file

09c5368

add another test file

6435a6d

Added fix for git diff being off

6d2604e

Add test files again

8a3792e

Final update with rename workflow

b30b38e

cjappl commented Mar 11, 2024

View reviewed changes

cjappl requested review from sandr01d, carlfriedrich and wfxr March 11, 2024 18:29

carlfriedrich requested changes Mar 12, 2024

View reviewed changes

cjappl added 2 commits March 12, 2024 06:24

Test rename

a202d3a

Remove text files, add final version

424acaa

cjappl requested a review from carlfriedrich March 12, 2024 13:43

sandr01d reviewed Mar 12, 2024

View reviewed changes

Sandroid suggestion for new sed

3a036f9

sandr01d approved these changes Mar 12, 2024

View reviewed changes

carlfriedrich approved these changes Mar 14, 2024

View reviewed changes

cjappl merged commit 2436fc4 into master Mar 14, 2024
7 checks passed

cjappl deleted the fix-sed-wackiness branch March 14, 2024 13:55

sandr01d mentioned this pull request Mar 17, 2024

Reduce deferred execution #326

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue where diff preview was broken on mac #365

Fix issue where diff preview was broken on mac #365

cjappl commented Mar 11, 2024

cjappl Mar 11, 2024

carlfriedrich Mar 12, 2024

carlfriedrich commented Mar 12, 2024

carlfriedrich Mar 12, 2024

carlfriedrich commented Mar 12, 2024

cjappl commented Mar 12, 2024

carlfriedrich commented Mar 12, 2024

cjappl commented Mar 12, 2024

sandr01d Mar 12, 2024

cjappl Mar 12, 2024

sandr01d Mar 12, 2024 •

edited

Loading

cjappl Mar 12, 2024

sandr01d commented Mar 12, 2024

sandr01d left a comment

carlfriedrich commented Mar 14, 2024

carlfriedrich commented Mar 14, 2024 •

edited

Loading

carlfriedrich commented Mar 14, 2024

cjappl commented Mar 14, 2024

cjappl commented Mar 15, 2024

carlfriedrich commented Mar 15, 2024

cjappl commented Mar 15, 2024

	get_files="echo {} \| sed -e 's/^[[:space:]]\\\\[[A-Z0-9]\\\\][[:space:]]//' \| sed 's/.-> //' \| tr '\\\n' '\\\0'"
	get_files="echo {} \| sed -e 's/^[[:space:]]\\\\[[A-Z0-9]\\\\][[:space:]]//' \| sed 's/.-> //' \| tr '\\\n' '\\\0'"
	get_files="echo {} \| sed -e 's/^[[:space:]]\\\\[[A-Z0-9]\\\\][[:space:]]*//' \| sed 's/ -> /\\\n/' \| tr '\\\n' '\\\0'"

Fix issue where diff preview was broken on mac #365

Fix issue where diff preview was broken on mac #365

Conversation

cjappl commented Mar 11, 2024

Check list

Description

Type of change

Test environment

cjappl Mar 11, 2024

Choose a reason for hiding this comment

carlfriedrich Mar 12, 2024

Choose a reason for hiding this comment

carlfriedrich commented Mar 12, 2024

carlfriedrich Mar 12, 2024

Choose a reason for hiding this comment

carlfriedrich commented Mar 12, 2024

cjappl commented Mar 12, 2024

carlfriedrich commented Mar 12, 2024

cjappl commented Mar 12, 2024

sandr01d Mar 12, 2024

Choose a reason for hiding this comment

cjappl Mar 12, 2024

Choose a reason for hiding this comment

sandr01d Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

cjappl Mar 12, 2024

Choose a reason for hiding this comment

sandr01d commented Mar 12, 2024

sandr01d left a comment

Choose a reason for hiding this comment

carlfriedrich commented Mar 14, 2024

carlfriedrich commented Mar 14, 2024 • edited Loading

carlfriedrich commented Mar 14, 2024

cjappl commented Mar 14, 2024

cjappl commented Mar 15, 2024

carlfriedrich commented Mar 15, 2024

cjappl commented Mar 15, 2024

sandr01d Mar 12, 2024 •

edited

Loading

carlfriedrich commented Mar 14, 2024 •

edited

Loading