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

[sandr01d]

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

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

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

Test environment

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

[carlfriedrich]

Is this really the correct thing to do here? As far as I can see we're assigning the opts variable, so the escaped quotes will be a quote in the variable's value, which would make this line:

--preview="$FORGIT diff_view {} "$_forgit_preview_context" $escaped_commits"

So actually this does not really quote the variable in the --preview argument. Or am I wrong?

[sandr01d]

I think it is. From my understanding, fzf evaluates the string that is passed to --preview, which essentially removes the outer quotes.

[carlfriedrich]

But evaluating the string would also remove the inner quotes, because the first one of them closes the very first quote, and the second one opens a new quote to be closed by the very last quote. Wouldn't it?

[sandr01d]

I see you point now and you're right. I think the correct way to escape this would be like this then:

--preview=\"$FORGIT diff_view {} \\\"$_forgit_preview_context\\\" $escaped_commits\"

Do you agree?
We would also have to update several other lines that make the same mistake, e.g. in _forgit_reset_head we do the same thing

    opts="
        $FORGIT_FZF_DEFAULT_OPTS
        -m -0
        --preview=\"$FORGIT reset_head_preview \"$rootdir\"/{}\"
        $FORGIT_RESET_HEAD_FZF_OPTS
    "

[carlfriedrich]

Glad you're seeing it as well. You're totally right, we should fix it everywhere. The double escape you suggested seems correct. It's hardly readable, though. Would single quotes work here instead? Or would that be bad practice? It shouldn't prevent the shell variables from being evaluated because that happens when assigning opts already, so the --preview argument would have constant values in it. We could choose if we'd either replace the outer or the inner double quotes with single quotes. WDYT?

[sandr01d]

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]

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

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

Looks good to me now, thanks for the update!