Reduce deferred execution #326

sandr01d · 2023-10-15T16:55:37Z

Check list

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:

Preview functions (e.g. _forgit_add_preview) that are responsible for the fzf preview
Edit functions (e.g. _forgit_edit_add_file) that are responsible for editing files with your $EDITOR from within forgit functions
Git functions (e.g. _forgit_git_add) that make simple git commands reusable
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

Test environment

Shell
- bash
- zsh
- fish
OS
- Linux
- Mac OS X
- Windows
- Others:

sandr01d · 2023-10-15T17:06:28Z

The failing CI test is due to #327

carlfriedrich

@sandr01d Great work, thanks for starting on this! I would propose implementing this change for all forgit functions within this single PR and move all bugfixing stuff to later PRs. What do you think?

bin/git-forgit

sandr01d · 2023-10-27T20:06:44Z

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 · 2023-10-27T20:16:50Z

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 · 2023-10-28T10:49:41Z

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?

bin/git-forgit

carlfriedrich · 2023-11-03T19:49:09Z

Notes to _forgit_log:

Not sure how to correctly fix the shellcheck. When I quote $graph it does not show anything in my case.
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 · 2023-11-04T20:09:36Z

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 · 2023-11-04T21:32:25Z

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 · 2023-11-04T21:45:23Z

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 · 2023-11-04T23:26:25Z

@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 · 2023-11-05T11:37:41Z

@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.

bin/git-forgit

sandr01d · 2023-11-05T15:13:17Z

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 · 2023-11-05T16:46:57Z

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 · 2023-11-06T15:13:53Z

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

sandr01d · 2023-11-06T21:09:08Z

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

Works for me

bin/git-forgit

sandr01d · 2023-11-19T19:03:27Z

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 · 2023-11-28T07:18:18Z

@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 #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 · 2023-11-29T21:00:32Z

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.

Forgit allows specifying git options in environment variables that are passed along to the individual git commands. We currently treat those as strings. This commit adds a _forgit_parse_array function and uses it to parse all such environment variables into arrays instead. This will allow us to get rid of deferred code, since we can pass the parsed arrays directly to the git commands and don't have to rely on eval.

Removes the deferred code that is used for creating the fzf preview functions and replaces it with _forgit_*_preview functions instead. These functions are exposed as forgit commands so they can be invoked from the fzf subshell. We split the exposed commands into public_commands and private_commands. The only difference between them is that public_commands are mentioned in the help text. This commit 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.

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 the deferred code was used with xargs, which executes it on a subshell. To avoid having to expose the new git functions the same way we do with the preview functions, the usage of xargs in these cases is replaced with either a loop or a single command when possible.

We used to have a variable that was either undefined or contained a piece of deferred code that piped input through emojify when present on the system. To remove the deferred code here, this commit replaces the _forgit_emojify variable with a function that either pipes the input through emojify or through cat, depending on whether emojify is present.

We were using deferred code in git commands in some places without any reason. Each of these deferred code snippets was only executed a single time, so we can replace them with regular git commands. This commit changes how we handle the FORGIT_LOG_GRAPH_ENABLE environment variable. We previously used a variable that stored the --graph flag as a string and unset it, when FORGIT_LOG_GRAPH_ENABLE was set to anything other than true. We now create an empty array and add the --graph flag to it when FORGIT_LOG_GRAPH_ENABLE is unset or true. Doing it this way allows us to build a command line without having to use eval. The outcome is the same as before.

In _forgit_log and _forgit_enter it is possible to diff a single commit/file by pressing enter. We used to store the code that executes the diffs in variables and passed it to fzf as deferred code. This refactor reduces the amount of deferred code by using functions instead of variables.

_forgit_diff and _forgit_add allow editing the currently previewed file in the EDITOR. This used to be handled entirely using deferred code. This commit replaces the deferred code and binds the commands to functions instead.

Many commands allow copying the commit hash or stash name of the current selection to the clipboard. We previously used deferred code to do so. This commit replaces the deferred code and binds these commands to functions instead.

We used to store code that extracts the commit hash from a line in a variable. This commit replaces this variable with a function.

…l to _forgit_get_single_file_from_add_line

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 commit introduces a _forgit_quote_files function that ensures the resulting arguments are quoted properly.

sandr01d · 2024-03-17T20:03:16Z

Alright, I've rebased on top of master now and we're ready to start testing. Let me know when whether you find any issues.
@cjappl Could you pease double check whether your fix from #365 is still working as intended. It should (and I can confirm it does on Linux), but I want to be absolutely sure.

In regards to merging, I would strongly recommend we merge this PR (without squashing it) once it's tested. This way, we can be absolutely certain that the final state resembles exactly what we've been testing and we avoid having to rebase and merge 11 PRs and any mistakes that might arise by doing so.

bin/git-forgit

cjappl · 2024-03-18T00:46:21Z

Reporting back on mac, using fish shell. @sandr01d

Adding files

✅ Adding file[with]brackets
✅ adding "file with spaces.txt"
✅ adding normal file

Reset head

✅ file[with]brackets
✅ "file with spaces.txt"
✅ normal file

Git log

✅ without arguments
✅ with branch name as argument
✅ with file name as argument

Diffing files

✅ Diffing file[with]brackets
✅ Diffing "file with spaces.txt"
✅ Diffing normal file

Git ignore

✅ normal operation

Checkout file

✅ file[with]brackets
✅ "file with spaces.txt"
✅ normal file

Checkout branch

✅ Local branch
✅ Remote branch
✅ Local branch as argument
✅ New branch to be created

Delete branch

✅ normal operation
✅ With branch as argument

Git clean

✅ Adding dummy file, then deleting it through glcean

Git stash show/git stash push

✅ Normal operation adding things to the stash, dropping things from the stash
❌ You are able to enter into gsp with no changes in the directory, I feel we should early exit/not drop into fzf in this case (Able to drop into gsp with no changes in the directory #369)

This is not a regression, as confirmed in a later comment. See ticket linked above.

Steps to repro:

Have clean directory, no changes no added files

gsp

Expected:
Nothing happens, gsp exits early, just like if you ga with "Nothing to add"

Observed:
Fzf pops up but the list is empty, user can do nothing.

cherry pick

✅ Normal operation with no arguments
✅ WIth branch as argument

Rebase

✅ Squashing multiple commits together
✅ Rewording commits
✅ with branch name as argument

Fixup

✅ Working in normal operation (add some commits, stage some changes, apply changes to a commit)

Checkout commit

✅ With no arguments
✅ WIth commit sha as argument

Revert commit

✅ With no arguments
✅ WIth commit sha as argument

Blame

✅ Normal operation.

EDIT: THIS WORKS FINE! I had a configuration issue where I was using a non-standard FZF_DEFAULT_COMMAND

set -x FZF_DEFAULT_COMMAND "fd --color=always --exclude .git . \$dir"

Original text below:

Again, not sure if this is a regression. If it isn't, let's file a ticket and NOT fix it here.

Steps to repro:

Navigate to top level of our forgit repo

gbl

Expected:
Directories do not have a blame shown (what even is a git blame for a directory??)

Observed:
Both files AND DIRECTORIES show up as blames. I think we need to filter out the directories
Also: It would be nice if we could syntax highlight the blames, or put them through bat or our pager.

Checkout tag

✅ With no arguments
✅ With tag as argument

Environment variables

✅ FORGIT_FZF_DEFAULT_OPTS seems respected
✅ FORGIT_BLAME_FZF_OPTS seems respected
✅ FORGIT_BLAME_GIT_OPTS seems respected
✅ Various others (I wasn't 100% exhaustive here, we have a bunch of these and in the code they appear to be treated the same. I did 5 or so other ones and called it a day)

cjappl · 2024-03-18T00:47:58Z

Overall I give this a thumbs up on mac and fish on the basis of this "quick smoke test" this was all of the tests I could think to run, and everything seems generally solid.

I will continue playing with it and using it as my daily driver. Let me know if you saw any test cases I missed or something I should spend time focusing on. Great work @sandr01d , for such an insane code review very few regressions as far as I can tell

sandr01d · 2024-03-18T20:36:43Z

@cjappl First of all, thanks for the in depth testing!

Git stash show/git stash push

❌ You are able to enter into gsp with no changes in the directory, I feel we should early exit/not drop into fzf in this case

I can reproduce this in this branch, but so can I in the master branch, so not a regression. Definitively a bug though, I agree that we should open a separate issue.

Fixup

❓ not sure how to test this one, open to ideas

I think the easiest way to test this is to create a local repository, add a few files and create a few commits, then do a few changes, add them and use gfu afterwards. At least that is how I tested things when I was doing changes to it.

Blame

❌ Normal operation.

Again, not sure if this is a regression. If it isn't, let's file a ticket and NOT fix it here.

Steps to repro:

Navigate to top level of our forgit repo
gbl

Expected:
Directories do not have a blame shown (what even is a git blame for a directory??)

Observed:
Both files AND DIRECTORIES show up as blames. I think we need to filter out the directories

This one I can not reproduce. I've checked our implementation on how we generate the file list for this command and it is by simply executing fzf in the directory like this:

file=$(FZF_DEFAULT_OPTS="$opts" fzf --preview="$FORGIT blame_preview {} ${flags[*]}")

What happens when you execute fzf in the repository by itself, do you also see directories? Anything in your $FZF_DEFAULT_OPTS or $FORGIT_FZF_DEFAULT_OPTS that might cause this behavior? Would also be worth checking how this behaves in the master branch on your side. I think you might have the same behavior there too.

Also: It would be nice if we could syntax highlight the blames, or put them through bat or our pager.

I agree. Syntax highlighting would be super useful here.

I will continue playing with it and using it as my daily driver. Let me know if you saw any test cases I missed or something I should spend time focusing on.

Would be great if you could play around the environment variables we allow customizing the git commands with, such as FORGIT_BLAME_GIT_OPTS. Also passing arguments to the git commands that support it like gbl --color-by-age.

cjappl · 2024-03-18T21:54:30Z

Git stash show/git stash push

❌ You are able to enter into gsp with no changes in the directory, I feel we should early exit/not drop into fzf in this case
I can reproduce this in this branch, but so can I in the master branch, so not a regression. Definitively a bug though, I agree that we should open a separate issue

Filed here #369

For the future, definitely does not have to be taken on until this big review lands

Blame

Good intuition. This was a config issue on my side. Fixing my config fixed the issue, this is not a regression or anything to worry about.

I agree. Syntax highlighting would be super useful here.

#370

For the future, definitely does not have to be taken on until this big review lands

Would be great if you could play around the environment variables we allow customizing the git commands with, such as FORGIT_BLAME_GIT_OPTS. Also passing arguments to the git commands that support it like gbl --color-by-age.

Will put some time on my calendar to do this tomorrow, same with git fixup.

I have updated my big summary comment above with this info as well.

carlfriedrich · 2024-03-19T09:27:21Z

In regards to merging, I would strongly recommend we merge this PR (without squashing it) once it's tested. This way, we can be absolutely certain that the final state resembles exactly what we've been testing and we avoid having to rebase and merge 11 PRs and any mistakes that might arise by doing so.

@sandr01d You're probably right, that seems to be the safest way. I think GitHub should actually automatically close the 11 other PRs when we merge this, since they contain the same commits as in this branch.

@cjappl Great test work, thanks a lot! I will do some testing during the next days as well.

cjappl · 2024-03-20T23:08:34Z

I have completed my testing and deem this good to ship on fish shell and macOS. My complete testing is in a previous comment. (including test cases I didn't do before.

Let me know if there is anything else people can think to test that I have missed.

cjappl

🚀 🎸 🔥

sandr01d · 2024-03-23T00:41:26Z

Since we did not find any new bugs I'm planning to merge this on Sunday when I don't hear any objections.

carlfriedrich · 2024-03-23T19:36:59Z

@sandr01d Didn't have time for testing yet, but if you guys did not find anything then go for it! 🚀

Reintroduces the usage of eval for evaluating pagers. We removed all usage of eval with #326, which broke code defined by users in the $FORGIT_*_PAGER environment variables and git pagers. In this particular case, eval does exactly what we want, since we do want to execute code that is defined by the user outside of forgit.

sandr01d requested review from carlfriedrich, wfxr and cjappl October 15, 2023 16:57

sandr01d force-pushed the reduce-deferred-exec branch from edcfa7e to def8a51 Compare October 15, 2023 17:03

carlfriedrich requested changes Oct 16, 2023

View reviewed changes