Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: Replace deferred code in enter commands with functions #358

Closed
wants to merge 6 commits into from

Conversation

sandr01d
Copy link
Collaborator

@sandr01d sandr01d commented Mar 3, 2024

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

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.

Note

This PR belongs to #326 and resulted from discussions in #324.
In comparison to #326 I've replaced the $enter_cmd variable in _forgit_diff with a function aswell. The part of it that cds into the repo directory has been removed because this is handled by _forgit_diff_view already.

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation change

Test environment

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

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.
@sandr01d
Copy link
Collaborator Author

sandr01d commented Mar 3, 2024

I've noticed there is still some deferred code left in the pager variables (e.g. $_forgit_enter_pager). I think we should replace these with functions as well. Not in this PR though. We might want to do it after all changes in #326 have been reviewed or even after #326 has been merged.

@carlfriedrich
Copy link
Collaborator

@sandr01d Great, thanks for the patch. I think next could be the execute_silent commands, right? What else have we left?

I've noticed there is still some deferred code left in the pager variables (e.g. $_forgit_enter_pager). I think we should replace these with functions as well. Not in this PR though. We might want to do it after all changes in #326 have been reviewed or even after #326 has been merged.

That's a good idea, and I indeed think we can do it after we finished the ongoing review.

@sandr01d
Copy link
Collaborator Author

sandr01d commented Mar 3, 2024

@carlfriedrich

I think next could be the execute_silent commands, right?

I think we should do _forgit_extract_sha first because that's used in the execute-silent commands a few times.

What else have we left?

Apart from those two, I think only _forgit_quote_files is left. I might split the execute_silent commands into multiple PRs though (edit, yank commit and yank stash).

@carlfriedrich
Copy link
Collaborator

@carlfriedrich

I think next could be the execute_silent commands, right?

I think we should do _forgit_extract_sha first because that's used in the execute-silent commands a few times.

What else have we left?

Apart from those two, I think only _forgit_quote_files is left. I might split the execute_silent commands into multiple PRs though (edit, yank commit and yank stash).

Great, let's do it like that!

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.
@sandr01d
Copy link
Collaborator Author

Changes have been merged with #326

@sandr01d sandr01d closed this Mar 24, 2024
@sandr01d sandr01d deleted the enter_cmd branch April 5, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants