Fix issue where diff preview was broken on mac

cjappl commented 3 months ago

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
- [x] bash
- [ ] zsh
- [x] fish
OS
- [x] Linux
- [x] Mac OS X
- [ ] Windows
- [ ] Others:

carlfriedrich commented 3 months ago

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 commented 3 months ago

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

cjappl commented 3 months ago

@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 commented 3 months ago

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

cjappl commented 3 months ago

@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 commented 3 months ago

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.

carlfriedrich commented 3 months ago

Works for me!

carlfriedrich commented 3 months ago

@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 commented 3 months ago

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

cjappl commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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

cjappl commented 3 months ago

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

wfxr / forgit