search

wfxr / forgit

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

Refactor: Quote files passed to preview functions #368

Closed sandr01d closed 3 months ago

sandr01d commented 3 months ago

Check list

Description

In some cases we need to pass multiple files to preview functions. These files are treated as a single string that is evaluated by fzf and passed on to our preview functions as individual arguments. This PR introduces a _forgit_quote_files function that ensures the resulting arguments are quoted properly.

Type of change

Test environment

sandr01d commented 3 months ago

FYI: This is the last patch from #326 :rocket:

sandr01d commented 3 months ago

Just checking, this is only needed for fixup, rebase and log? Not any of the other file-based functions (add, reset head, for example)

Yes, add and reset head do exit early without showing a preview when arguments are passed. This is also only necessary for commands that allow specifying multiple files as arguments to filter what is displayed in the preview. So this is only relevant when picking commits. In most other commands we pick (and therefore preview) single files, so filtering the preview is not necessary.

cjappl commented 3 months ago

@sandr01d Great work! Thanks a lot for your patience and will to invest the time for splitting out the individual chunks. I think it was worth the work.

Big +1 on this. Good job @sandr01d !! And thank you @carlfriedrich for being the main reviewer on it. Good teamwork.

So I think next task is to rebase the branch on the current master, right? Shall we do some testing days afterwards? If so, I would suggest to freeze the master until we have merged this. Given the size of the branch I would also be fine with merging right away and fixing eventual regressions afterwards. We're in the middle of the month, so we have two weeks until the next regular release.

  1. I agree we should freeze master until after we merge this

I am split on the right approach for testing it. I think we should avoid at all costs shipping something that is broken in some way.

My gut is telling me we should do some in branch testing on our favorite OSes and workflows for a few days, trying to really hammer it and fix any bugs that fall out obviously, then merge. I agree with "Given the size of the branch I would also be fine with merging right away and fixing eventual regressions afterwards."

Open to any and all of this. I have some time to devote to some intensive testing, so just ping me with when the thing is ready and I'll put it through it's paces.

sandr01d commented 3 months ago

Thank you guys, appreciate it! And thanks for your reviews. I definitively agree that it was absolutely worth it.

So I think next task is to rebase the branch on the current master, right?

Yes, will do so shortly.

I agree that we should freeze the master branch until the refactor has been merged. I think we should take some extra time for testing. How about we all use #326 as our daily driver for a week? If we find any bugs I will try to fix them as soon as possible. In case we don't find any bugs that require big code changes we could merge once the week is over.

I have some time to devote to some intensive testing, so just ping me with when the thing is ready and I'll put it through it's paces.

@cjappl That would definitively be great! From my experience, when I break things it's usually for macOS users, due to slight differences in sed, the older bash version and my inability to test on this OS.

sandr01d commented 3 months ago

Changes have been merged with #326