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.

[sandr01d]

This PR is now ready for review. I've been daily driving this branch since we started working on it and in the past weeks things have been perfectly stable for me. @cjappl This branch has not seen any testing on macOS yet, so it would be especially interesting to know whether everything works as expected for you too.

[cjappl]

@sandr01d downloaded and will test for a little bit. I think I would want to daily drive it for a little considering how extensive the change is.

I'll keep you posted.

[cjappl]

In general, my testing environment should be assumed to be: MacOS Fish shell

EDIT: This is actually working, just had some other foo going on. Scrolling with option on mac is OK!

[cjappl]

Taking it through the actions I use the very most: ga, gcf, grh, glo, gct, gco gcb everything is working A-OK! Great work wrangling this complexity, beast of a PR.

I would propose we all drive this branch for a week or so and if no one has hit anything by Feb 5th, we submit.

Thoughts @sandr01d @carlfriedrich ?

[carlfriedrich]

@cjappl I would actually like to do a code review on this before it gets merged.

[cjappl]

@cjappl I would actually like to do a code review on this before it gets merged.

🤦 whoops, yeah and that! That seems important. I skipped over that pretty crucial step lol.

Code review + drive for a bit :)

[carlfriedrich]

I just took a first quick look at the commit history.

@sandr01d IMO the following functions should have dedicated commits as well:

• _forgit_extract_sha
• _forgit_is_file_tracked

Right now these are replaced piece by piece with each commit. I think each function should have its own commit (just like _forgit_emojify), and they should be the first in the commit stack, since they are helper functions. Would you mind extracting these changes? Otherwise I could do it. What do you think?

[cjappl]

I just took a first quick look at the commit history.

@sandr01d IMO the following functions should have dedicated commits as well:

* `_forgit_extract_sha`

* `_forgit_is_file_tracked`

Right now these are replaced piece by piece with each commit. I think each function should have its own commit (just like _forgit_emojify), and they should be the first in the commit stack, since they are helper functions. Would you mind extracting these changes? Otherwise I could do it. What do you think?

I would prefer we "squash and merge" by default in this repo to keep the history clean, making the individual commits irrelevant.

[carlfriedrich]

@cjappl We already discussed this earlier in this thread:

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 still think that we should keep the commit history here, given the size of the change. So I would vote for either a merge commit or merge by rebase.

Either way, separating commits into semantical entities will make the review process easier.

[cjappl]

Cool, works for me! sorry for missing that. In that case I agree, abstracting out the individual changes would be good.

[carlfriedrich]

@sandr01d IMO the following functions should have dedicated commits as well:

* `_forgit_extract_sha`
* `_forgit_is_file_tracked`

Digging deeper into the implementation, I noticed that my suggestion is not possible because these functions are used in preview expressions, so they would be unknown to the child process. Sorry for the noise. :-)

I still have the feeling that there is potential of extracting more atomic changes out of the PR, though, so I might come up with other proposals during the review.

[carlfriedrich]

So going through the single commits I see that we're still doing many things at once here. Which is good for the end result, but hard to capture all at once when reading the changes.

First of all: while this is tagged as a refactoring PR, it also fixes a few bugs along the way. These should definitely appear in the commit messages in the end, because we're using the commit history for our change log in the release notes.

To make this easier and more transparent, I would suggest to spot changes forming a logic entity and extract them into a separate PR, then rebasing this PR on top of that.

The goals of this are:

• Give forgit users the chance to understand what's happening in the changelog
• Track bug fixes
• Make this PR smaller
• Get better code

I would furthermore suggest to do this one after another, i.e.:

1. Spot one logic entity
2. Extract it to a dedicated PR
3. Rebase this PR
4. Go back to 1

We do this until this PR cannot be split to entities anymore. This requires some more rebasing effort, but I am convinced that it will make both the changelog and the code better and more readable. Does that sound like a good plan @sandr01d? I am happy to help doing all the rebasing, if you are not willing to put even more work into this. In order not to need to test all the PRs separately, I think we could stack them up and merge them all at once in the end.

The first entity I see we could extract is parsing all the FORGIT_*_GIT_OPTS into arrays using the new _forgit_parse_array function. I see this as an atomic change that is independent of the rest, and it is one that already includes a bugfix.

WDYT?

[sandr01d]

Doing the review process in separate smaller chunks like this sounds reasonable to me. Do I understand it correctly, that we'd focus on a single PR and only move forward with rebasing this branch and create the next PR once we're happy with the changes in the chunk? I think that splitting this PR into multiple PRs all at once would create a lot of friction. I expect doing it this way will take some time (there will be 22+ separate PRs), but we're not in a hurry.

In order not to need to test all the PRs separately, I think we could stack them up and merge them all at once in the end.

Could you please elaborate on this? Would we merge this PR once we've rebased upon all the smaller PRs and close down the smaller PRs?

The first entity I see we could extract is parsing all the FORGIT_*_GIT_OPTS into arrays using the new _forgit_parse_array function.

That makes sense and I can prepare a PR for that. I don't see a way to include the usage of the arrays we create with this though. Those are used exclusively in the forgit_git_* functions which were created afterwards. Would a PR that only adds the _forgit_parse_array function and the parsing itself be ok? This is what it would look like.

[carlfriedrich]

Doing the review process in separate smaller chunks like this sounds reasonable to me.

Thanks for understanding!

Do I understand it correctly, that we'd focus on a single PR and only move forward with rebasing this branch and create the next PR once we're happy with the changes in the chunk?

Yes, that's exactly how I meant it.

I think that splitting this PR into multiple PRs all at once would create a lot of friction. I expect doing it this way will take some time (there will be 22+ separate PRs), but we're not in a hurry.

Yes, it will take some time and chore work indeed, and I am happy to help with this. I am not sure if we really need that much PRs in the end, though. Let's go ahead and see how this PR shrinks on the way.

In order not to need to test all the PRs separately, I think we could stack them up and merge them all at once in the end.

Could you please elaborate on this? Would we merge this PR once we've rebased upon all the smaller PRs and close down the smaller PRs?

Exactly. When we prepare a chunk PR and merge it right away, we have to make sure that it is tested individually, which is needless work, given the fact that we already are ahead in the code. So we just leave it open and prepare the next PR based on the first PR's branch and then merge all of them when the task is completed.

The first entity I see we could extract is parsing all the FORGIT_*_GIT_OPTS into arrays using the new _forgit_parse_array function.

That makes sense and I can prepare a PR for that. I don't see a way to include the usage of the arrays we create with this though. Those are used exclusively in the forgit_git_* functions which were created afterwards. Would a PR that only adds the _forgit_parse_array function and the parsing itself be ok?

Thanks, that would be a good starting point. However, is there a reason why we couldn't put the arrays in the former deferred code snippets? IMO that would make a useful atomic commit.

[sandr01d]

Thanks, that would be a good starting point. However, is there a reason why we couldn't put the arrays in the former deferred code snippets? IMO that would make a useful atomic commit.

Nevermind, that is possible.

I think we're all set then, I've created #338 and am awaiting your reviews.

[carlfriedrich]

@sandr01d Thanks for submitting the patch. This branch can be rebased now on top of #338. Do you need help with that?

The next atomic chunk I see is changing all of the git calls from variables into functions, e.g.

 git_blame="git blame ${_forgit_blame_git_opts[*]}"

into

_forgit_git_blame() {
    git blame "${_forgit_blame_git_opts[@]}" "$@"
}

These functions are all called directly, so this should work independently of the rest. And as far as I can see this would be a no-op change as well, so refactoring only. Can you confirm this? Or does this change behavior in passing the command line opts?

[sandr01d]

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.

[cjappl]

Reporting back on mac, using fish shell. @sandr01d

Adding files

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

Reset head

1. ✅ file[with]brackets
2. ✅ "file with spaces.txt"
3. ✅ normal file

Git log

1. ✅ without arguments
2. ✅ with branch name as argument
3. ✅ with file name as argument

Diffing files

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

Git ignore

1. ✅ normal operation

Checkout file

1. ✅ file[with]brackets
2. ✅ "file with spaces.txt"
3. ✅ normal file

Checkout branch

1. ✅ Local branch
2. ✅ Remote branch
3. ✅ Local branch as argument
4. ✅ New branch to be created

Delete branch

1. ✅ normal operation
2. ✅ With branch as argument

Git clean

1. ✅ Adding dummy file, then deleting it through glcean

Git stash show/git stash push

1. ✅ Normal operation adding things to the stash, dropping things from the stash
2. ❌ 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 (https://github.com/wfxr/forgit/issues/369)

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

Steps to repro:

1. Have clean directory, no changes no added files
2. 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

1. ✅ Normal operation with no arguments
2. ✅ WIth branch as argument

Rebase

1. ✅ Squashing multiple commits together
2. ✅ Rewording commits
3. ✅ with branch name as argument

Fixup

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

Checkout commit

1. ✅ With no arguments
2. ✅ WIth commit sha as argument

Revert commit

1. ✅ With no arguments
2. ✅ WIth commit sha as argument

Blame

1. ✅ 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:

1. Navigate to top level of our forgit repo
2. 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

1. ✅ With no arguments
2. ✅ With tag as argument

Environment variables

1. ✅ FORGIT_FZF_DEFAULT_OPTS seems respected
2. ✅ FORGIT_BLAME_FZF_OPTS seems respected
3. ✅ FORGIT_BLAME_GIT_OPTS seems respected
4. ✅ 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]

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]

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