Discussion: Reduce the amount of deferred execution

[sandr01d]

We currently rely on deferred execution quite heavily. This leads to subtle bugs that are quite hard to catch (see e.g. #271 and #323). I also find that it makes the code hard to read in some places. Additionally deferred code has security implications, especially when using eval and/or when processing untrusted sources (e.g. other peoples git repositories). I think we should reduce the amount of deferred code. I'm aware that this would require quite some amount of work, but in my opinion it would be worth it.
My current idea would be to replace the deferred code with functions. I think we might also be able to reuse more code/make reusing code easier with this approach. We'd end up with two different kinds of functions:

1. Functions for the individual git commands
2. Helper functions that are used in functions of category 1.

I think it would make sense to move all functions of category 2 into a separate file.
Ideally this would also speed up the process of adding new features in the long run, as adding a new function of category 1 would mostly mean chaning up functions of category 2.

If we decide to do this I suggest we create a draft early, so anyone can comment on the general structure of things or even work on it together.

Would be interested in hearing your opinions on this.

[cjappl]

I'm a fan of removing the deferred execution. These bugs are the ones that really stump me for a long time ("Do I need 4 quotes, or 5? How many backslashes?")

I'm agnostic about splitting things into separate files. Unless it becomes extremely verbose, I think a project as simple as this benefits from there being one file to find everything you may need to know. With not a lot of major IDE support for things that exist in other languages (go to definition) I could see splitting the source becoming a pain in development.

Pinging @carlfriedrich @wfxr

[carlfriedrich]

@sandr01d Thanks for starting this topic. I actually agree with the issues you mention and had similar thoughts multiple times when searching for certain bugs. I was always scared to change more than needed, though, and funnily the reason is that the code covers so many corner cases in certain areas that I was afraid to lose any of them when doing too much refactoring. So that's a vicious circle, obviously. The code is hard to read and to maintain, but in order to not destroy any working behavior, we're adding even more hard-to-read and hard-to-maintain stuff. 🙈

So approaching this as a group effort is something I am all-in on! Do I get it right that you would be willing to create a first draft?

Concerning the additional file for helper functions: I actually don't see the need of it either since the forgit code is not that huge, but I would't bother having it split up as well. I think two files should be easy to handle even without a major IDE. 😊

Removing deferred executions should also make forgit faster, so that's a bonus IMO.

[sandr01d]

I don't have a strong opinion about splitting things into separate files either. That was just something that came to my mind when thinking about this. After reading your thoughts I think it would probably be best to only do one thing at a time and keep everything in one file for now. It should be trivial to split things up later in case we decide to do so.
Great to hear that we're on the same page regarding the deferred code.

Do I get it right that you would be willing to create a first draft?

Yes, absolutely. My plan is to create a draft that refactors one or two of the existing functions as a base for further discussion.

[carlfriedrich]

@sandr01d Great, looking forward to it!

[sandr01d]

Implementing this is more difficult than I previously anticipated. The reason for this is that fzf starts an external process for every preview, as described here. Meaning functions that are simply declared in the same file won't be available in the preview process. The only workarounds that have come to my mind so far are to export the functions we want to use in the preview or move them to a separate file and source them from the preview process. As both ideas would heavily decrease the readability and maintainability of the code this isn't really an option.
Maybe someone else here has an idea on how we could approach this?

[carlfriedrich]

I am not sure if I understand your problem correctly. Can you maybe share a piece of actual (non-working) code that we can comment on?

Concerning exporting functions: this is only possible with bash. Both zsh and fish do not support exporting functions, so this won't be a solution.

[sandr01d]

I am not sure if I understand your problem correctly. Can you maybe share a piece of actual (non-working) code that we can comment on?

Concerning exporting functions: this is only possible with bash. Both zsh and fish do not support exporting functions, so this won't be a solution.

I've made a bit of progress since my last post, so the issue is a bit different than I described previously. If you could take a look at it anyway I would highly appreciate it.

I've modified _forgit_blame to look like this:

_forgit_is_file_tracked() {
    git ls-files "$1" --error-unmatch &> /dev/null
}

_forgit_blame_preview() {
    # echo "$1" correctly print out the file that is currently selected in fzf
    # all other commands seem to receive "{}" as a value directly for some reason
    if _forgit_is_file_tracked "$1"; then
        git blame "$@" ${FORGIT_BLAME_GIT_OPTS:+"$FORGIT_BLAME_FZF_OPTS"} --date=short | $_forgit_diff_pager
    else
        echo "File not tracked"
    fi
}

# git blame viewer
_forgit_blame() {
    _forgit_inside_work_tree || return 1
    local git_blame opts flags preview file
    git_blame="git blame ${FORGIT_BLAME_GIT_OPTS:+"$FORGIT_BLAME_GIT_OPTS"}"
    _forgit_contains_non_flags "$@" && { $git_blame "$@"; return $?; }
    opts="
        $FORGIT_FZF_DEFAULT_OPTS
        $FORGIT_BLAME_FZF_OPTS
    "
    flags=$(git rev-parse --flags "$@")
    file=$(FZF_DEFAULT_OPTS="$opts" fzf --preview="echo \"$(_forgit_blame_preview {} ${flags:+"$flags"})\"")
    [[ -z "$file" ]] && return 1
    $git_blame "$file" ${flags:+"$flags"}
}

What I meant in my previous post is that the fzf command does not work if used like this

file=$(FZF_DEFAULT_OPTS="$opts" fzf --preview="_forgit_blame_preview {} ${flags:+"$flags"}")

This will print out

/usr/bin/bash: line 1: _forgit_blame_preview: command not found

because the subshell created by fzf is not aware of the _forgit_blame_preview function. Calling it like I do in the example above works though. My main issue at the moment is as described in the comment in the _forgit_blame_preview function. Something seems to go wrong with the substitution. When I print out the first argument passed to _forgit_blame_preview with echo "$1", everything works as expected. When passing it on to another command as I do with git ls-files (but also when using e.g. cat "$1") it seems to be interpreted as a string with the value "{}".

[carlfriedrich]

@sandr01d Thanks for your snippet, that helps a lot. I would suggest making the functions we need in the preview public in the form of forgit commands. They shouldn't appear in the help text, though, so I could think of the following:

public_commands=(
    "blame"
    ...
)

private_commands=(
    "blame_preview"
   ...
)

cmd="$1"
shift

if [[ ! " ${public_commands[*]} " =~ " ${cmd} " ]] && [[ ! " ${private_commands[*]} " =~ " ${cmd} " ]] ; then
    if [[ -z "$cmd" ]]; then
        printf "forgit: missing command\n\n"
    else
        printf "forgit: '%s' is not a valid forgit command.\n\n" "$cmd"
    fi
    printf "The following commands are supported:\n"
    printf "\t%s\n" "${public_commands[@]}"
    exit 1
fi

This way you should be able to call forgit in the preview:

file=$(FZF_DEFAULT_OPTS="$opts" fzf --preview="${FORGIT} blame_preview {} ${flags:+"$flags"}")

Haven't tried this, so there might be careless errors, but the basic concept should work. Actually navi (a fzf based cheat sheet tool) does it in a similar way.