Refactor: Move git commands from deferred code into functions

sandr01d commented 5 months ago

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

We often used deferred code to encapsulate git commands and make them reusable. This change removes deferred code for git commands and replaces it with functions instead. Some of these new functions are used with xargs, which needs to execute them from a subshell. To make this possible, we now expose the concerned functions as forgit commands without advertising them in the help text.

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 #326 and resulted from discussions in #324

sandr01d commented 5 months ago

@carlfriedrich I did not include the deferred git commands that we did not end up creating functions for in this PR. Let me know whether you would like to have them included here.

A note regarding 2d81ed9: This was not originally included in #326. When I was revisiting this I came to the conclusion that a _forgit_git_branch_delete function should always pass the -D flag to git branch.

sandr01d commented 4 months ago

Haven't looked into everything in detail, yet, but first thing I noticed is that 7657444 reverts some of the changes of #338. The array parsing in the functions gets lost. Can you fix that?

The array parsing was moved into the _forgit_git_* functions were the arrays are being used. I think it is correct as is, can you verify?

carlfriedrich commented 4 months ago

@sandr01d Yes, they are correctly added in the first commit of this PR (1e8f79edefe17b35bad55725973ed387e910ce73), but the second commit (7657444632c8d0421d63838f318e03fa4dc35168) removes the calls from the _forgit_git_* functions again.

sandr01d commented 4 months ago

@sandr01d Yes, they are correctly added in the first commit of this PR (1e8f79e), but the second commit (7657444) removes the calls from the _forgit_git_* functions again.

Could you please point me to a line were this is happening, I do not understand. The parsing is added to the _forgit_* functions in https://github.com/wfxr/forgit/commit/1e8f79edefe17b35bad55725973ed387e910ce73 and moved to the _forgit_git_* functions in https://github.com/wfxr/forgit/commit/7657444632c8d0421d63838f318e03fa4dc35168. See for example, _git_forgit_add after https://github.com/wfxr/forgit/commit/7657444632c8d0421d63838f318e03fa4dc35168:

https://github.com/sandr01d/forgit/blob/2d81ed909fd799566fb7c377f09e6b23a2675173/bin/git-forgit#L183C1-L187C2

carlfriedrich commented 4 months ago

@sandr01d Oh, I'm sorry! I viewed the patch on my tiny mobile phone screen, which obviously is not a good idea, and also I obviously did not read your comment correctly. 🙈 So yes, on my desktop monitor I see now that the calls are moved and not removed. Will have a more detailed look soon.

sandr01d commented 4 months ago

@sandr01d Oh, I'm sorry! I viewed the patch on my tiny mobile phone screen, which obviously is not a good idea, and also I obviously did not read your comment correctly. 🙈 So yes, on my desktop monitor I see now that the calls are moved and not removed. Will have a more detailed look soon.

No worries and thanks for the clarification. I was only questioning my sanity for a brief second :laughing:

carlfriedrich commented 4 months ago

Just browsed through all the changes and the patch looks clean, great work! Also good catch with the -D flag in _forgit_git_branch_delete. I would suggest to squash that commit into its parent.

I wonder if we could replace the four xargs calls with for loops, so that we do not have to export these git functions as private functions to the forgit CLI. That would make the code more readable IMO, because we would remove a level of indirection there and call the functions directly. WDYT?

sandr01d commented 4 months ago

I would suggest to squash that commit into its parent.

Done

I wonder if we could replace the four xargs calls with for loops, so that we do not have to export these git functions as private functions to the forgit CLI. That would make the code more readable IMO, because we would remove a level of indirection there and call the functions directly. WDYT?

I think that is a great idea, but I think we should probably do so in a follow up PR once #326 has been merged. I see this as more of a general refactor and not related to deferred code and #326 is already pretty big. WDYT? Also implementing it in this PR is rather difficult for _forgit_stash_show, since xargs is currently being used in deferred code, that is replaced with _forgit_stash_show_preview later in #326. So replacing it later would be way easier.

carlfriedrich commented 4 months ago

I understand that this can be seen as a more general refactoring. I think, though, that it would make this specific commit better, so IMO it would be the right thing to do here. Concerning the complexity of this PR, it would add the xargs refactoring, but instead remove the introduction of private_commands here, so I think this specific PR wouldn't grow in complexity. And concerning the complexity of #326: everything we extract to these chunk-PRs will reduce the overall complexity of the final PR (what we're doing here doesn't need to be done in #326). I see, however, that everything we're adding here which wasn't part of #326 before, increases the difficulty of rebasing #326 on this PR. I would consider this a good invest, though, and I again want to offer my help with this. I know, rebasing can be a pain, but I still enjoy doing it when I know that it's worth for a good end result. So are you in? 🤗

sandr01d commented 4 months ago

I understand that this can be seen as a more general refactoring. I think, though, that it would make this specific commit better, so IMO it would be the right thing to do here.

That is totally reasonable and I agree with that. I am not against the change, but I don't really see a good way to implement it in this specific PR, let me elaborate a bit on this. I've refactored _forgit_reset_head, _forgit_checkout_commit and _forgit_branch_delete and they work fine. You can take a look at it here in case you're interested. The issue is with _forgit_stash_show. The usage of xargs is withing deferred code that is executed from an fzf subshell:

# xargs is within this deferred code
cmd="echo {} |cut -d: -f1 |xargs -I% $FORGIT git_stash_show % |$_forgit_diff_pager"
    opts="
        $FORGIT_FZF_DEFAULT_OPTS
        +s +m -0 --tiebreak=index --bind=\"enter:execute($cmd | $_forgit_enter_pager)\"
        --bind=\"ctrl-y:execute-silent(echo {} | cut -d: -f1 | tr -d '[:space:]' | ${FORGIT_COPY_CMD:-pbcopy})\"
# we use this for the fzf preview here
        --preview=\"$cmd\"
        $FORGIT_STASH_FZF_OPTS
    "

We can not call _forgit_git_stash_show from the subshell. We've introduced the private_commands to deal with this particular issue for the _forgit_*_preview functions. The code I've pasted above is later on replaced with a preview function and then it becomes trivial to get rid of xargs:

_forgit_stash_show_preview() {
-   echo "$1" | cut -d: -f1 | xargs -I% "$FORGIT" "git_stash_show" % | $_forgit_diff_pager
+   # we don't actually need a loop here because
+   # there is always just one stash previewed
+   local stash
+   stash=$(echo "$1" | cut -d: -f1)
+   _forgit_git_stash_show "$stash" | $_forgit_diff_pager
}

If we want to get rid of xargs and the private_commands in this PR that means we'd have to find a new solution to this. Which to me clearly indicates that we should isolate other chunks first and once the _forgit_*_preview functions are introduced, we could rebase this PR on top of that.

I see, however, that everything we're adding here which wasn't part of https://github.com/wfxr/forgit/pull/326 before, increases the difficulty of rebasing https://github.com/wfxr/forgit/pull/326 on this PR. I would consider this a good invest, though, and I again want to offer my help with this.

Appreciated, thanks. I think it would be great if we could take turns rebasing every now and then. :+1:

carlfriedrich commented 4 months ago

@sandr01d Ah, I see. Thanks for the detailed explanation. I actually didn't realize that it's used in deferred code at one place. In this case I agree, let's do this after #326. I'd consider this PR approved then. ✅

Shall I rebase #326 on this PR then?

sandr01d commented 4 months ago

Alternatively, to doing this after #326, we could keep this here as is for the moment and continue splitting out other chunks. Once the preview functions are introduced, we can rebase this PR on top of that and get rid of xargs here. I would convert this PR in a draft in this case. I think that would result in the cleanest commit history, so what do you think about that?

carlfriedrich commented 4 months ago

That sounds like a good idea! Then let's do the preview functions in the next chunk. I just took a quick look over them and think they would make a reasonable patch. WDYT?

sandr01d commented 4 months ago

I've now rebased this PR on #349 and replaced the usage of xargs, as previously requested by @carlfriedrich. Re-requesting you reviews @cjappl @carlfriedrich. There is one thing that I did not mention before, that I'd like to discuss with you. This PR does not replace all deferred git commands in all our main functions. It only replaces the occurrences that we introduced new functions for in #326. In some places this was not necessary, due to the git command only being used a single time, e.g. in _forgit_cherry_pick and _forgit_clean. I could include the code changes that remove the deferred code there here as well if you prefer.

sandr01d commented 3 months ago

Changes have been merged with #326

wfxr / forgit