search

wfxr / forgit

:zzz: A utility tool powered by fzf for using git interactively.
MIT License
4.32k stars 136 forks source link

Refactor: Quote variables in preview of _forgit_diff, _forgit_reset_head & _forgit_cherry_pick_form_branch #372

Closed sandr01d closed 3 months ago

sandr01d commented 3 months ago

Check list

Description

The $_forgit_preview_context variable used in the deferred code block for generating the preview of _forgit_diff was not quoted. No known bugs were caused by this, just making sure we follow best practices.

Note

This is a follow-up to #326 and was originally discussed here.

Type of change

Test environment

sandr01d commented 3 months ago

Good idea to use single quotes, looks way nicer this way @carlfriedrich I'd prefer using the single quotes for the outer quotes and updated the accordingly. Do you approve?

carlfriedrich commented 3 months ago

@sandr01d I just did a quick echo $opts for testing, and it seems the outer single quotes do not work because we also use single qoutes within $escaped_commits. So the preview arg renders as follows:

--preview='/home/tim/packages/forgit/bin/git-forgit diff_view {} "3" 'HEAD^' '

It works anyway, but it's still not what we want, I guess.

sandr01d commented 3 months ago

@sandr01d I just did a quick echo $opts for testing, and it seems the outer single quotes do not work because we also use single qoutes within $escaped_commits. So the preview arg renders as follows:

--preview='/home/tim/packages/forgit/bin/git-forgit diff_view {} "3" 'HEAD^' '

It works anyway, but it's still not what we want, I guess.

@carlfriedrich You're right, good catch! I think in this case the right way to go is to use double quotes for outer and single quotes for inner quotation. I've updated this PR accordingly. I've printed out the assigned $opts variable and it does look correct to me now. Could you give it a second look?

carlfriedrich commented 3 months ago

Looks good to me now, thanks for the update!