search

wfxr / forgit

:zzz: A utility tool powered by fzf for using git interactively.
MIT License
4.32k stars 136 forks source link

diff preview not working on MacOS #362

Closed giovannilupi closed 3 months ago

giovannilupi commented 3 months ago

Check list

Environment info

Problem / Steps to reproduce

Hello,

I've recently ported my forgit configuration from a Linux machine to my MacOS laptop. I've noticed previews are not working for the gd command. For example, using the default configuration:

Screenshot 2024-03-09 at 13 58 22

Other commands using the same pager, such as ga, seem to work as expected:

Screenshot 2024-03-09 at 14 00 27

Did anyone else encounter the same issue?

Cheers

cjappl commented 3 months ago

I can confirm I'm seeing the same thing, both in fish and zsh.

Thanks for the report, I'll see what I can figure out!

cjappl commented 3 months ago

Just did a little bisect and it seems like #354

@sandr01d do you think this is worth fixing now, or wait for your big refactor to land? Do you have a second to check on linux if this is also true.

sandr01d commented 3 months ago

@cjappl I can not reproduce this on Linux on the current master branch. Does this happen with every preview for gd or just specific files? I think this would be worth fixing this now, as we'd need a similar fix for #326 anyway. My guess would be that we're using something that GNU sed supports, but the one on macOS does not. Since I don't have any device running macOS, I guess you'd have to go after this. Doing the fix in #326 is probably easier, since you don't have to worry about all the issues around escaping characters. For #354 I've done the work in #326 first and then backported it to the current master. You'd have to modify the sed commands in _forgit_get_files_from_diff_line and _forgit_get_single_file_from_diff_line. I found this an easier workflow then working in the current master branch directly. I'm attaching a patch to apply #354 to #326 in case you want to go that route.

354.txt

giovannilupi commented 3 months ago

From my limited testing, this seems to occur for every file on MacOS. I was unable to reproduce the issue on Linux as well.

cjappl commented 3 months ago

@sandr01d Thanks for that patch!! it was pretty easy to fix it in that deferred execution branch.

Seeing as it's in your fork, I can't apply it, but here is the diff to the reduce-deferred-exec branch:

missing_pipe.patch

Will you apply this to your branch, and we can close this issue when it lands? Any idea how close we are to merging? This is the first time I've worked in that branch and it is SO much easier to reason about, thank you again for taking on that work, huge maintenance improvement.

sandr01d commented 3 months ago

@cjappl Thanks for the patch, can confirm this works on Linux too.

Will you apply this to your branch, and we can close this issue when it lands?

I think this should be a separate PR and I see two options here. We could either backport your fix to master now (which I guess should be rather easy, given that it isn't a big change) and I apply the patch to #326 once I rebase on master or we create a new PR once #326 is merged. I think I would prefer the first option. WDYT?

Any idea how close we are to merging?

Pretty close I think. All the big changes have been reviewed and we should be able to push through the rest quickly. Btw. could you review #361, then I can follow up with the next PR shortly.

This is the first time I've worked in that branch and it is SO much easier to reason about, thank you again for taking on that work, huge maintenance improvement.

Awesome, glad to hear :slightly_smiling_face:

cjappl commented 3 months ago

I think this should be a separate PR and I see two options here. We could either backport your fix to master now (which I guess should be rather easy, given that it isn't a big change) and I apply the patch to https://github.com/wfxr/forgit/pull/326 once I rebase on master or we create a new PR once https://github.com/wfxr/forgit/pull/326 is merged. I think I would prefer the first option. WDYT?

Unfortunately I can't seem to find the issue in the master branch. It does not seem to be the same issue as in the deferred execution branch, and I'm lost in a sea of ///// trying to debug it. Do you easily see the issue?

I'm down for either option, but may need some help tracking down the bug in master.

I also just filed #363 so we can try to lock down some of these regressions in the future

maelp commented 3 months ago

I have the same issue, osX and zsh

maelp commented 3 months ago

When I checkout the latest commit on branch fix_mac_diff in the zsh plugin repo, the diff shows, but if I change files the view on the side stays on the same file (the diff doesn't change when I move to other files)

sandr01d commented 3 months ago

@maelp you're right I can reproduce that. Thanks for the input. @cjappl I think that the underlying issue wasn't the missing pipe. I'm pretty sure that the issue has something to do with the sed command, since this is the only thing I've changed in #354. This would also explain why we can't find the corresponding code in master. Would you be open to give this another shot?

maelp commented 3 months ago

Possibly need to use gnu sed on osx since the default sed from osx is not compatible with the gnu one?On Mar 11, 2024, at 18:20, sandr01d @.***> wrote: @maelp you're right I can reproduce that. Thanks for the input. @cjappl I think that the underlying issue wasn't the missing pipe. I'm pretty sure that the issue has something to do with the sed command, since this is the only thing I've changed in #354. This would also explain why we can't find the corresponding code in master. Would you be open to give this another shot?

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

cjappl commented 3 months ago

Yep let me spend some time today seeing if I can nail it down! I'll report back.

On Mon, Mar 11, 2024 at 10:20 AM, sandr01d @.***(mailto:On Mon, Mar 11, 2024 at 10:20 AM, sandr01d < wrote:

@.(https://github.com/maelp) you're right I can reproduce that. Thanks for the input. @.(https://github.com/cjappl) I think that the underlying issue wasn't the missing pipe. I'm pretty sure that the issue has something to do with the sed command, since this is the only thing I've changed in #354. This would also explain why we can't find the corresponding code in master. Would you be open to give this another shot?

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

cjappl commented 3 months ago

@maelp @giovannilupi please test on latest master, this should be fixed. If you two confirm I'll cut a tag and release it on the package managers

maelp commented 3 months ago

working now, thanks!

giovannilupi commented 3 months ago

Also works for me, thanks a lot @cjappl

cjappl commented 3 months ago

Awesome! Thanks for filing this @giovannilupi and thank you both (@maelp) for testing and reporting back. We appreciate it.