Re: [PATCH] git-sh: Avoid sourcing scripts with git --exec-path

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 29, 2017, 03:48 UTC
Message-ID: <20170929034856.GB28303@aiede.mtv.corp.google.com>
In-Reply-To: <20170928223134.GA30744@varnish>

Hi,

Dridi Boukelmoune wrote:

Show 6 quoted lines

> For end users making use of a custom exec path many commands will simply
> fail. Adding git's exec path to the PATH also allows overriding git-sh-*
> scripts, not just adding commands. One can then patch a script without
> tainting their system installation of git for example.
>
> Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

This has been broken for a while, but better late than never to address it.

[...]

Show 14 quoted lines

> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -151,6 +151,28 @@ For shell scripts specifically (not exhaustive):
>     interface translatable. See "Marking strings for translation" in
>     po/README.
>  
> + - When sourcing a "git-sh-*" file using "git --exec-path" make sure
> +   to only use its base name.
> +
> +	(incorrect)
> +	. "$(git --exec-path)/git-sh-setup"
> +
> +	(correct)
> +	. git-sh-setup

I wonder if we can make this so intuitive that it doesn't need mentioning in CodingGuidelines. What if the test harness t/test-lib.sh were to set a GIT_EXEC_PATH with multiple directories in it? That way, authors of new commands would not have to be careful to look out for this issue proactively because the testsuite would catch it.

[...]

Show 8 quoted lines

> +++ b/contrib/convert-grafts-to-replace-refs.sh
> @@ -5,7 +5,8 @@
>  
>  GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
>  
> -. $(git --exec-path)/git-sh-setup
> +PATH="$(git --exec-path):$PATH"
> +. git-sh-setup

I wish there were a simple way to do this that doesn't involve polluting $PATH with the dashed commands. E.g. we can do something like

	OIFS=$IFS
	IFS=:
	set -f
	for d in $(git --exec-path)
	do
		if test -e "$d/git-sh-setup"
		then
			. "$d/git-sh-setup"
			break
		fi
	done
	set +f
	IFS=$OIFS

but that is not very simple.  Something like

	old_PATH=$PATH
	PATH=$(git --exec-path):$PATH
	. git-sh-setup
	PATH=$old_PATH

looks like it could work, but it would undo the work git-sh-setup does to set a sane $PATH on platforms like Solaris.

Show 9 quoted lines

> --- a/contrib/rerere-train.sh
> +++ b/contrib/rerere-train.sh
> @@ -7,7 +7,8 @@ USAGE="$me rev-list-args"
>  
>  SUBDIRECTORY_OK=Yes
>  OPTIONS_SPEC=
> -. "$(git --exec-path)/git-sh-setup"
> +PATH="$(git --exec-path):$PATH"
> +. git-sh-setup

This makes me similarly unhappy about PATH pollution, but it may be that there's nothing to be done about that.

[...]

Show 8 quoted lines

> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -32,7 +32,6 @@ squash        merge subtree changes as a single commit
>  "
>  eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
>  
> -PATH=$PATH:$(git --exec-path)
>  . git-sh-setup

This looks like a change that could be separated into a separate patch, both because it is to contrib/subtree (which is maintained separately) and because it is not necessary for the goal described in this patch's commit message. I do like this change, since it improves consistency with other commands.

Show 8 quoted lines

> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -44,7 +44,7 @@ git_broken_path_fix () {
>  # @@BROKEN_PATH_FIX@@
>  
>  # Source git-sh-i18n for gettext support.
> -. "$(git --exec-path)/git-sh-i18n"
> +. git-sh-i18n

Do git-mergetool--lib.txt, git-parse-remote.txt, git-sh-i18n.txt, and git-sh-setup.txt in Documentation/ need the same treatment?

Summary: I like the goal of this patch but I am nervous about the
side-effect it introduces of PATH pollution.  Is there a way around
that?  If there isn't, then this needs a few tweaks and then it should
be ready to go.

Thanks and hope that helps, Jonathan