Refactor: Replace deferred code used for fzf preview with functions

[sandr01d]

Check list

• [X] 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

Removes the deferred code that is used for creating the fzf preview functions and replaces it with _forgit_*_preview functions instead. This PR changes the flags variable in _forgit_blame from a string to an array. This is necessary to allow the flags to be passed to _forgit_blame_preview as individual arguments. The only implementation detail I changed in comparison to #326 is that I removed xargs from from _forgit_stash_show_preview, as discussed in #343. xargs was actually not necessary here because we always preview only a single stash. This way we do not have to expose _forgit_git_stash_show as a private_command. I've included new functions we've added in #326, that are used inside the _forgit_*_preview functions. I regard them as part of the preview functions.

Type of change

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

Test environment

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

Note

This PR belongs to https://github.com/wfxr/forgit/pull/326 and resulted from discussions in https://github.com/wfxr/forgit/issues/324

[sandr01d]

I did all the changes I meant to do here. Awaiting your reviews @carlfriedrich @cjappl.

[cjappl]

Sounds good! A lot to review, but I'll take it through it's paces on fish as a system test today and report back on how that goes :)

[carlfriedrich]

@sandr01d Thanks for the patch! I will do a review in the following days. This time I'll make sure to view it directly on my desktop monitor. 😁

[cjappl]

squash all the changes into one commit

Is there an advantage to doing this before "squash and merge" when we merge the final one @carlfriedrich ? You keep suggesting it, which makes me think I'm overlooking some upside to the approach.

In my perspective, it seems like manually squashing and merging is unneeded extra work if the work will just be squashed-and-merged automagically by github later.

Is there a difference in the two approaches that makes the manual way better?

[carlfriedrich]

Is there an advantage to doing this before "squash and merge" when we merge the final one @carlfriedrich ? You keep suggesting it, which makes me think I'm overlooking some upside to the approach.

@cjappl Generally speaking, it gives us the possibility to review the commit message. This is not the case when GitHub's squash and merge feature is used, because that happens after the review. And since the commit message is used for the changelog in our release notes, it should be part of the review IMO, especially given the fact that it cannot be changed once it is merged to master.

In this specific case it is also crucial that we don't want to squash all the commits, since this PR also contains the base commit which will be merged in another PR. This makes it even more important to keep the commits tidy.

[cjappl]

Sweet, thanks for the explanation @carlfriedrich !

@cjappl Generally speaking, it gives us the possibility to review the commit message. This is not the case when GitHub's squash and merge feature is used, because that happens after the review. And since the commit message is used for the changelog in our release notes, it should be part of the review IMO, especially given the fact that it cannot be changed once it is merged to master.

Nice, good point, I hadn't considered that. "Make commit messages reviewable"

In this specific case it is also crucial that we don't want to squash all the commits, since this PR also contains the base commit which will be merged in another PR. This makes it even more important to keep the commits tidy.

Makes sense to me! I think in this specific review case (considering the complexity of it all) I'm all for anything that provides reviewability for it. I'm pro-squash for this review.

I don't necessarily think we need to do this for 100% of PRs in forgit, but we can maybe discuss on an Issue ticket if we want to implement that process change for every commit going forward, I think there are mostly "pros" for complex multi-file multi-commit branches, and mostly "cons" for simple changes.

[carlfriedrich]

Ah wait, we have #343 next in the line. @sandr01d Will you rebase that on top of this PR then first?

[sandr01d]

Yes, sure. Don't have the time today, but plan to do so tomorrow.

[sandr01d]

Changes have been merged with #326