Refactor: Move git commands from deferred code into functions #343
Conversation
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.
|
@carlfriedrich I did not include the deferred git commands that we did not end up creating functions for in this PR. Let me know whether you would like to have them included here. A note regarding 2d81ed9: This was not originally included in #326. When I was revisiting this I came to the conclusion that a |
The array parsing was moved into the |
Could you please point me to a line were this is happening, I do not understand. The parsing is added to the |
|
@sandr01d Oh, I'm sorry! I viewed the patch on my tiny mobile phone screen, which obviously is not a good idea, and also I obviously did not read your comment correctly. 🙈 So yes, on my desktop monitor I see now that the calls are moved and not removed. Will have a more detailed look soon. |
No worries and thanks for the clarification. I was only questioning my sanity for a brief second 😆 |
|
Just browsed through all the changes and the patch looks clean, great work! Also good catch with the I wonder if we could replace the four |
sandr01d
force-pushed
the
git-functions
branch
from
2d81ed9
to
6fb371f
Compare
Done
I think that is a great idea, but I think we should probably do so in a follow up PR once #326 has been merged. I see this as more of a general refactor and not related to deferred code and #326 is already pretty big. WDYT? |
|
I understand that this can be seen as a more general refactoring. I think, though, that it would make this specific commit better, so IMO it would be the right thing to do here. |
That is totally reasonable and I agree with that. I am not against the change, but I don't really see a good way to implement it in this specific PR, let me elaborate a bit on this. # xargs is within this deferred code
cmd="echo {} |cut -d: -f1 |xargs -I% $FORGIT git_stash_show % |$_forgit_diff_pager"
opts="
$FORGIT_FZF_DEFAULT_OPTS
+s +m -0 --tiebreak=index --bind=\"enter:execute($cmd | $_forgit_enter_pager)\"
--bind=\"ctrl-y:execute-silent(echo {} | cut -d: -f1 | tr -d '[:space:]' | ${FORGIT_COPY_CMD:-pbcopy})\"
# we use this for the fzf preview here
--preview=\"$cmd\"
$FORGIT_STASH_FZF_OPTS
"We can not call _forgit_stash_show_preview() {
- echo "$1" | cut -d: -f1 | xargs -I% "$FORGIT" "git_stash_show" % | $_forgit_diff_pager
+ # we don't actually need a loop here because
+ # there is always just one stash previewed
+ local stash
+ stash=$(echo "$1" | cut -d: -f1)
+ _forgit_git_stash_show "$stash" | $_forgit_diff_pager
}If we want to get rid of
Appreciated, thanks. I think it would be great if we could take turns rebasing every now and then. 👍 |
|
Alternatively, to doing this after #326, we could keep this here as is for the moment and continue splitting out other chunks. Once the preview functions are introduced, we can rebase this PR on top of that and get rid of |
|
That sounds like a good idea! Then let's do the preview functions in the next chunk. I just took a quick look over them and think they would make a reasonable patch. WDYT? |
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.
sandr01d
force-pushed
the
git-functions
branch
from
6fb371f
to
5c9768c
Compare
|
I've now rebased this PR on #349 and replaced the usage of |
sandr01d
force-pushed
the
git-functions
branch
from
5c9768c
to
f69ad7f
Compare
| @@ -677,9 +709,15 @@ _forgit_branch_delete() { | |||
| " | |||
|
|
|||
| cmd="git branch --color=always | LC_ALL=C sort -k1.1,1.1 -rs" | |||
There was a problem hiding this comment.
What about this deferred git call here? Can we get rid of that as well?
There was a problem hiding this comment.
I've only included deferred git calls that we replaced with separate functions in #326, see my previous comment. We could replace all deferred git calls in this PR, which would make it a bit bigger, as there are still a few of them left that we did not create functions for. We could also do that in the next PR. I'm open for both.
There was a problem hiding this comment.
Ah sorry, I missed that part of the comment in the first place. I would prefer replacing all git calls in this PR, since that's what its title says. :-)
There was a problem hiding this comment.
0de4f2e removes all deferred git commands. I had to include converting $_forgit_emojify into a function and the replacement of the $graph variable with an array to get the commands working without deferred code and eval. This makes this PR quite big. Haven't squashed the commit yet, because I'm thinking creating a separate PR out of 0de4f2e to reduce the size of this one. WDYT?
There was a problem hiding this comment.
Ah, I see, that's quite a huge additional change indeed. I did not expect that, sorry. So yes, let's move that to a new PR. I would consider this PR finished then.
Do I get it right that none of the new changes were part of the original #326? I wonder what's the best way to continue then, since we have some changes in #326 left to include. Shall we do those first?
There was a problem hiding this comment.
Do I get it right that none of the new changes were part of the original #326?
No they were mostly part of #326, I just moved them over to this branch. The only thing in this PR that is not in #326 yet are the four hunks that remove xargs.
I would suggest we rebase #326 on this PR to make sure we don't digress too far and I create a new PR that converts $_forgit_emojify into a function and after that a PR that replaces the rest of the deferred git commands. Sounds good?
There was a problem hiding this comment.
Yep, that sounds good, thanks a lot!
There was a problem hiding this comment.
FYI: I started to rebase #326. I'm almost done and will finalize it tomorrow, so don't put any effort into it.
There was a problem hiding this comment.
Oh wow, thanks a lot for your effort!
sandr01d
force-pushed
the
git-functions
branch
from
f79da92
to
0de4f2e
Compare
sandr01d
force-pushed
the
git-functions
branch
3 times, most recently
from
9659072
to
84acaf2
Compare
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.
sandr01d
force-pushed
the
git-functions
branch
from
84acaf2
to
1f8c594
Compare
|
Changes have been merged with #326 |
Check list
Description
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 these new functions are used with xargs, which needs to execute them from a subshell. To make this possible, we now expose the concerned functions as forgit commands without advertising them in the help text.
Type of change
Test environment
Note
This PR belongs to #326 and resulted from discussions in #324