Reduce deferred execution

[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

We've removed deferred code and replaced it with functions instead. This includes all usages of eval. There are four different types of functions we've added:

1. Preview functions (e.g. _forgit_add_preview) that are responsible for the fzf preview
2. Edit functions (e.g. _forgit_edit_add_file) that are responsible for editing files with your $EDITOR from within forgit functions
3. Git functions (e.g. _forgit_git_add) that make simple git commands reusable
4. Utility functions (e.g. _forgit_yank_sha or _forgit_quote_files) that were created either out of necessity or implement functions that were previously stored as deferred code in variables.

We've exposed some of these functions as forgit commands. This includes all preview and edit functions and some of the utility functions, so they can be invoked from fzf's subshell. Additionaly some of the git functions were exposed to make it possible to use them with xargs.

In some places were we used deferred code before, we now make use of arrays to allow building commands while having control over globbing and word splitting. We do this for all the git opts environment variables, such as FORGIT_ADD_GIT_OPTS.

In places were we could not avoid deferred execution (e.g. when having to pass arguments to preview functions as we do in _forgit_log) we ensured variables are quoted properly. Variables not quoted properly were one of the main source of bugs previously.

Implementation Details

General

We now make use of the -a flag with read which does exist in bash, but not in zsh. This is fine, since we always use bash for executing /bin/git-forgit since #241.

gbl

It is now possible to pass more than one argument to gbl (e.g. gbl --color-by-age --color-lines). This was a bug previously and the fix is a side effect of us now using an array for the flags instead of relying on eval.

grc

I've removed passing the files variable to the preview, as well as passing parameters to the git log command with $*. These were creating issues in the new implantation and they were actually always empty, because _forgit_revert exits early when any arguments are passed. I copied them by mistake when I implemented _forgit_revert, they did never actually serve any purpose.

glo, grb & gfu

File arguments are now parsed using a different approach using _forgit_quote_files. The only difference in behavior to the sed command we had previously is that the string that is printed out has each file name wrapped in single quotes, so they can be used safely without having to worry about globbing or word splitting. There is no need to quote this string when passing it to a preview function, because the individual file names inside it are already properly quoted. I changed the files variable names to quoted_files to indicate this.

Type of change

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

Test environment

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

[sandr01d]

The failing CI test is due to #327

[sandr01d]

Since there haven't been any objections so far, I'm moving this forward and started refactoring the other functions. If anyone wants to jump in feel free to do so and send a PR to this branch.

[sandr01d]

A note regarding _forgit_revert: I've removed passing the files variable to the preview, as well as passing parameters to the git log command with $*. These were creating issues in the new implantation and they were actually always empty, because _forgit_revert exits early when any parameters are passed. I copied them by mistake when I implemented _forgit_revert, they did never actually serve any purpose.

I'll keep collecting implementation details as comments here, so we can combine them in a commit message when we merge this.

[carlfriedrich]

Since there haven't been any objections so far, I'm moving this forward and started refactoring the other functions. If anyone wants to jump in feel free to do so and send a PR to this branch.

@sandr01d Thanks a lot for keeping this going! I'll probably have some time by the end of next week and am motivated to take over some functions as well.

I will switch forgit to this branch for my daily work from now on in order to find regressions early while we're going. @cjappl @wfxr You maybe too?

[carlfriedrich]

Notes to _forgit_log:

1. Not sure how to correctly fix the shellcheck. When I quote $graph it does not show anything in my case.
2. I think the code was (and is) meant to show only the files in the preview that have been passed as arguments. This does not work, though, but it did not work before as well.

[carlfriedrich]

I found a way to correctly use the $graph variable. Still not sure how we correctly handle $FORGIT_LOG_GIT_OPTS, though. And files still not work either.

[carlfriedrich]

So I came to the conclusion that we just have to disable the specific shellcheck for passing $FORGIT_LOG_GIT_OPTS to the git call. The variable will be a space separated string, and thus we have to use it like that. Within bash, the clean way would be to use an array. This does not work for other shells, though. Any other thoughts on this?

And I think I have finished _forgit_log now. Passing files actually works when using -- (e.g. `git forgit log -- README.md). @sandr01d Can you do a quick review of my implementation?

[sandr01d]

So I came to the conclusion that we just have to disable the specific shellcheck for passing $FORGIT_LOG_GIT_OPTS to the git call. The variable will be a space separated string, and thus we have to use it like that. Within bash, the clean way would be to use an array. This does not work for other shells, though. Any other thoughts on this?

And I think I have finished _forgit_log now. Passing files actually works when using -- (e.g. `git forgit log -- README.md). @sandr01d Can you do a quick review of my implementation?

Your code looks good to me. I gave it a quick test and everything works as expected so far. The ability to pass in files is very neat, great work! Regarding the shellcheck, when we change the line to this it does not seem to complain:

git log "${graph[@]}" --color=always --format="$log_format" ${FORGIT_LOG_GIT_OPTS:+$FORGIT_LOG_GIT_OPTS} "$@" ${_forgit_emojify:+$_forgit_emojify}

But that is probably just hiding the issue and not doing anything better. Word splitting is intentional here so from my perspective disabling shellcheck as you do is fine. I think we should probably disable globbing and restore it after the line. I ran into the same issue with _forgit_fixup and _forgit_rebase and decided to also simply disable shellcheck there until we settled on a solution.

[sandr01d]

@carlfriedrich I noticed you changed the default of FORGIT_LOG_GRAPH_ENABLE to true in 521f349. I think this was by mistake, so I changed it back in 0fdbda3. Let me know in case this was intentional.

[carlfriedrich]

@carlfriedrich I noticed you changed the default of FORGIT_LOG_GRAPH_ENABLE to true in 521f349. I think this was by mistake, so I changed it back in 0fdbda3. Let me know in case this was intentional.

Actually I did not change the default value, I just switched the logic of evaluation and transferred the default value to the new variable _forgit_log_graph_enable because IMO that made the code more readable and consistent. I think you changed the default value to true now?

git log "${graph[@]}" --color=always --format="$log_format" ${FORGIT_LOG_GIT_OPTS:+$FORGIT_LOG_GIT_OPTS} "$@" ${_forgit_emojify:+$_forgit_emojify}

But that is probably just hiding the issue and not doing anything better. Word splitting is intentional here so from my perspective disabling shellcheck as you do is fine. I think we should probably disable globbing and restore it after the line. I ran into the same issue with _forgit_fixup and _forgit_rebase and decided to also simply disable shellcheck there until we settled on a solution.

Yes, that's definitley just a workaround and would clutter the code unnecessarily IMO. Disable globbing sounds reasonable. Maybe we could parse FORGIT_LOG_GIT_OPTS to an array _forgit_log_git_opts at the beginning of the code (and for all other git options variables as well) to have that in one single place. This would make the evaluation of user-configurable variables more consistent as well.

[sandr01d]

Actually I did not change the default value, I just switched the logic of evaluation and transferred the default value to the new variable _forgit_log_graph_enable because IMO that made the code more readable and consistent. I think you changed the default value to true now?

I did, but it was implicitly true by default before your change. Previously we only removed the --graph option when FORGIT_LOG_GRAPH_ENABLE was explicitly set to false and kept it when it was unset.

Disable globbing sounds reasonable. Maybe we could parse FORGIT_LOG_GIT_OPTS to an array _forgit_log_git_opts at the beginning of the code (and for all other git options variables as well) to have that in one single place. This would make the evaluation of user-configurable variables more consistent as well.

That does sound like a good approach to me.

[carlfriedrich]

I did, but it was implicitly true by default before your change. Previously we only removed the --graph option when FORGIT_LOG_GRAPH_ENABLE was explicitly set to false and kept it when it was unset.

Oh I see! Sorry, that was a misunderstanding. Thanks for correcting it!

[carlfriedrich]

@sandr01d I refactored most of _forgit_diff. Can you check if alt+e on a diffed file still works for you?

[sandr01d]

@sandr01d I refactored most of _forgit_diff. Can you check if alt+e on a diffed file still works for you?

Works for me

[sandr01d]

Notes regarding d23ec19:

I've exposed the new functions git_branch_delete and git_checkout_commit as forgit commands, so they can be used with xargs. I'm parsing the environment variables into arrays in _forgit_parse_array using read -r -a. The -a option does not exist in zsh, but from my understanding we always use bash for executing /bin/git-forgit since #241. I've tested it with bash, zsh and fish and it appears to be working fine. I'm also making use of nameref (local -n) in _forgit_parse_array, which is only supported from bash 4.3 upwards. I think this should be fine, since bash 4.3 has been released in 2014. Ubuntu 20.04 is using bash 5.0. @carlfriedrich could you do a quick review of these changes?

[carlfriedrich]

@sandr01d Thanks a lot for your effort on pushing this forward! I haven't had time for a detailed review, yet (might find time for it next week).

I'm parsing the environment variables into arrays in _forgit_parse_array using read -r -a. The -a option does not exist in zsh, but from my understanding we always use bash for executing /bin/git-forgit since https://github.com/wfxr/forgit/pull/241. I've tested it with bash, zsh and fish and it appears to be working fine.

Yes, this is correct, forgit always runs in bash.

I'm also making use of nameref (local -n) in _forgit_parse_array, which is only supported from bash 4.3 upwards. I think this should be fine, since bash 4.3 has been released in 2014. Ubuntu 20.04 is using bash 5.0.

This will not run on MacOS's bash, unfortunately, because MacOS uses ancient bash version 3.2.57 (I think due to licensing issues). Is there a different way to implement this?

[sandr01d]

This will not run on MacOS's bash, unfortunately, because MacOS uses ancient bash version 3.2.57 (I think due to licensing issues). Is there a different way to implement this?

Ah, I see thanks for the clarification. We could of course just set the IFS, parse all the arrays and unset it again like this:

# set IFS
${IFS+"false"} && unset old_IFS || old_IFS="$IFS"
# parse all the arrays here
_forgit_log_git_opts=()
IFS=" " read -r -a _forgit_log_git_opts <<< "$FORGIT_LOG_GIT_OPTS"
_forgit_diff_git_opts=()
IFS=" " read -r -a _forgit_diff_git_opts <<< "$FORGIT_DIFF_GIT_OPTS"
# ...
# reset IFS
${old_IFS+"false"} && unset IFS || IFS="$old_IFS"

But this is neither reusable nor pretty. I am not aware of a way to make this work in a function without a nameref, but I will do further research. I probably won't have time to work on it this week. In case you come up with something, let me know or feel free to push it to this branch.

[sandr01d]

@carlfriedrich I've removed the nameref from _forgit_parse_array in 88d928d. The downside of this is that arrays parsed with this function now are always global variables. I would be okay with this tough. Would love to hear your thoughts on this.

[sandr01d]

Note regarding ce37644 & e026a6d:

I changed how we parse file arguments for passing them along to the preview functions. I've added _forgit_escaped_files for this. The only difference in behavior to the sed command we had previously is that the string that is printed out has each file name wrapped in single quotes, so they can be used safely without having to worry about globbing or word splitting. There is no need to quote this string when passing it to a preview function, because the individual file names inside it are already properly quoted. I changed the files variable names to quoted_files to indicate this. I make use of it in _forgit_log where file names were not quoted previously and in _forgit_rebase and _forgit_fixup where all file names were quoted together (e.g. "LICENSE README.md" instead of "LICENSE" "README.md") by accident in one of my earlier commits in this PR.

[carlfriedrich]

@sandr01d Thanks again for keeping up the good work on this!

@carlfriedrich I've removed the nameref from _forgit_parse_array in 88d928d. The downside of this is that arrays parsed with this function now are always global variables. I would be okay with this tough. Would love to hear your thoughts on this.

That is totally fine for me. I don't see a problem having global variables here.

I changed how we parse file arguments for passing them along to the preview functions. I've added _forgit_escaped_files for this. The only difference in behavior to the sed command we had previously is that the string that is printed out has each file name wrapped in single quotes, so they can be used safely without having to worry about globbing or word splitting. There is no need to quote this string when passing it to a preview function, because the individual file names inside it are already properly quoted. I changed the files variable names to quoted_files to indicate this. I make use of it in _forgit_log where file names were not quoted previously and in _forgit_rebase and _forgit_fixup where all file names were quoted together (e.g. "LICENSE README.md" instead of "LICENSE" "README.md") by accident in one of my earlier commits in this PR.

Great work, and I also appreciate the rename to forgit_quote_files.

Are there any functions left to refactor now?

I am a little bit concerned about the size of the diff on this branch. We have changed quite a lot, which makes reviewing the code quite hard. Do you see a useful way of semantically sqashing commits? Can we maybe add an overview of all the things we did in this PR to the PR description?

I will continue to use the branch in my daily work, but I am not using git as heavily as in the past right now, so stumbling on bugs might take some time.

[sandr01d]

Are there any functions left to refactor now?

No, we're all set and from my side this would now be ready for review.

I am a little bit concerned about the size of the diff on this branch. We have changed quite a lot, which makes reviewing the code quite hard. Do you see a useful way of semantically sqashing commits?

I agree with you. I did not expect it to get this large when we started out with this. We could rebase and combine all commits that belong to a single forgit function. We would loose some of the history though and I'm not sure if that's worth it. I will mark this PR ready-for-review once we decided on this.

Can we maybe add an overview of all the things we did in this PR to the PR description?

Done.

I will continue to use the branch in my daily work, but I am not using git as heavily as in the past right now, so stumbling on bugs might take some time.

Given the size of this PR I will definitively leave it open for review for some time. I've also been using this branch for my daily work since I've created it. There are some functions that I rarely need, e.g. gi and grb and I will give them some extra testing aswell.

[carlfriedrich]

No, we're all set and from my side this would now be ready for review.

Great!

We could rebase and combine all commits that belong to a single forgit function. We would loose some of the history though and I'm not sure if that's worth it. I will mark this PR ready-for-review once we decided on this.

Usually we're squash-merging the PRs in this repo, so the history would be gone anyway. On such a huge change, however, it might be useful to keep the history, indeed. I am quite unsure about this. 55 commits in a single PR seems a bit overkill for sure, IMO we could at least squash the "fix shellcheck" commits into their parents, what do you think?

Can we maybe add an overview of all the things we did in this PR to the PR description? Done.

Great, thanks a lot!

Given the size of this PR I will definitively leave it open for review for some time. I've also been using this branch for my daily work since I've created it. There are some functions that I rarely need, e.g. gi and grb and I will give them some extra testing aswell.

That definitley sounds reasonable.

[sandr01d]

Usually we're squash-merging the PRs in this repo, so the history would be gone anyway. On such a huge change, however, it might be useful to keep the history, indeed. I am quite unsure about this. 55 commits in a single PR seems a bit overkill for sure, IMO we could at least squash the "fix shellcheck" commits into their parents, what do you think?

I think there was a misunderstanding here, I thought you meant squashing in this branch before we start the review process. I would actually squash the commits when merging with the usual "squash and merge" option, but keep them as is in the current branch. This way we kind of have both, a clean history on the master branch and the full history inside this PR. Does that sound reasonable to you?

[carlfriedrich]

I think there was a misunderstanding here, I thought you meant squashing in this branch before we start the review process. I would actually squash the commits when merging with the usual "squash and merge" option, but keep them as is in the current branch. This way we kind of have both, a clean history on the master branch and the full history inside this PR. Does that sound reasonable to you?

No, you actually got it right: I would really like to review this PR in detail, but given the fact that it is so large and so many commits, it is really hard to keep an eye on everything. That's why I am wondering how we could simplify the process. Squashing commits that semantically belong together is one option IMO, then we could review commit by commit. Maybe we could also split it into multiple PRs, then we could leave this branch untouched. However, I am not sure if this would cause all more work than necessary and we'd rather just test it in real life and then merge it as is. What do you think?

[carlfriedrich]

The longer I think and read about it, the more I am sure that we should squash commits semantically. Since you already spent a lot time refactoring, I can imagine that re-organizing commits doesn't sound like a motivating task. I can offer that I try to combine commits on a separate branch, which would also include reviewing them. That will take some time, though, since I am quite busy at the moment. What do you think?

[sandr01d]

The longer I think and read about it, the more I am sure that we should squash commits semantically. Since you already spent a lot time refactoring, I can imagine that re-organizing commits doesn't sound like a motivating task. I can offer that I try to combine commits on a separate branch, which would also include reviewing them. That will take some time, though, since I am quite busy at the moment. What do you think?

My main concern was loosing the original commit history, but I am not completely opposed to squashing commits semantically and I think you're probably right, that it could ease the review process. If we do so, squashing everything together that belongs to a single function would make sense to me. Don't worry about the additional workload, I'm fine with doing it. Given that I will be quite busy the next week too, I probably won't get to it before the next weekend.

[carlfriedrich]

My main concern was loosing the original commit history, but I am not completely opposed to squashing commits semantically and I think you're probably right, that it could ease the review process. If we do so, squashing everything together that belongs to a single function would make sense to me. Don't worry about the additional workload, I'm fine with doing it. Given that I will be quite busy the next week too, I probably won't get to it before the next weekend.

Thanks for your understanding and your will to invest more time. You can still keep the original history by copying your branch before squashing anything. Or you leave the original branch as it is, modify the copied branch and open a new (or multiple) tidied-up PRs for it.

[sandr01d]

@carlfriedrich I've now squashed commits together that belong to the same functions. Could you take a quick look and let me know whether that works for you? If so, we are ready start the review process. The original history can still be found here.

[carlfriedrich]

@sandr01d Thanks a lot for your further work on this. 22 commits is definitely a lot better than 55, and reviewing this PR commit by commit seems possible to me now. I will try to manage this during the following few weeks, okay?

[sandr01d]

@sandr01d Thanks a lot for your further work on this. 22 commits is definitely a lot better than 55, and reviewing this PR commit by commit seems possible to me now. I will try to manage this during the following few weeks, okay?

Great, looking forward to your review. In retrospect, cleaning up the commit history here was the right decision. Thanks for convincing me.