fish_git_prompt may run certain git commands which may invoke certain
external programs as specified .git/config. Prevent this by suppressing
certain git config options by default. Introduce a variable
$__fish_git_prompt_trusted_paths which allows these options to be
re-enabled for a directory hierarchy (possibly /).

Copy link

Member

****faho** commented Dec 27, 2021**

Introduce a variable
$__fish_git_prompt_trusted_paths which allows these options to be
re-enabled for a directory hierarchy (possibly /).

I do not think a configuration knob is warranted here. I don’t believe these git config options (core.fsmonitor, which is an optimization, and diff.external, which renders the diff) have an influence on what fish prints, and this isn’t the kind of decision we want to ask of users.

@@ -211,6 +212,15 @@ function fish_git_prompt --description “Prompt function for Git”

set -l space “$___fish_git_prompt_color$___fish_git_prompt_char_stateseparator$___fish_git_prompt_color_done”

Suppress certain config options in untrusted repos.

set -l extra_config -c core.fsmonitor= -c diff.external=

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today, overriding diff.external shouldn’t make a difference.

• Our “git diff” already uses --no-ext-diff, which has the same meaning (and takes precedence)
• “git diff-index” is a plumbing command that, by default, doesn’t use external diff drivers,
  only if you add --ext-diff and --patch (without --patch it doesn’t look at content changes)

As noted by @krobelus on Gitter, what about tab-completion?

I do not think a configuration knob is warranted here. […] core.fsmonitor, which is an optimization

OK. I wasn’t sure at first – this article mentions 12 vs. 1.5 seconds for git status – however unless bash.showInformativeStatus is set we are probably much faster.
So I think it’s okay to remove the configuration until someone complains.

what about tab-completion?

Right, let’s make that consistent:

diff --git a/share/completions/git.fish b/share/completions/git.fish index 6c1f80e7e…8a6090bd8 100644 — a/share/completions/git.fish +++ b/share/completions/git.fish @@ -17,7 +17,7 @@ function __fish_git end # Using ‘command git’ to avoid interactions for aliases from git to (e.g.) hub # Using eval to expand ~ and variables specified on the commandline. - eval command git $global_args \$saved_args 2>/dev/null

• eval command git $global_args -c core.fsmonitor= \$saved_args 2>/dev/null end

Print an optspec for argparse to handle git’s options that are independent of any subcommand.

Ok, you’ve convinced me to remove the knob. Our story can be “don’t use bash.showInformativeStatus on watchman-backed repos” and if someone really needs this they can hack up fish_git_prompt themselves.

I think it’s OK to leave tab completions as they are. If you are tab-completing git you probably have a reason, and tab-completing e.g. git add will run git status which really can benefit from watchman.

fish_git_prompt may run certain git commands which may invoke certain external programs as specified `.git/config`. Prevent this by suppressing certain git config options.

rm -Rf .git *

git init >/dev/null 2>&1

echo -n > ran.txt

git config core.fsmonitor ‘echo fsmonitor >> ran.txt; false’

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

(fish_indent would make some of the redirections consistent (>/dev/null vs > /dev/null) but it can’t see inside quotes)

Ok, you’ve convinced me to remove the knob. Our story can be “don’t use bash.showInformativeStatus on watchman-backed repos” and if someone really needs this they can hack up fish_git_prompt themselves.

Yeah. Going back and forth on this one. bash.showInformativeStatus and fsmonitor are beyond my horizon so I find it hard to empathize.

I think it’s OK to leave tab completions as they are. If you are tab-completing git you probably have a reason, and tab-completing e.g. git add will run git status which really can benefit from watchman.

OK