git-sh: Avoid sourcing scripts with git --exec-path

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.

---

Hi,

I've been bothered by this problem ever since I upgraded my system to Fedora 26, and I grew tired of locally patching git-sh-setup after every git-core update. So I decided to look at the instructions on how to send patches to the Git project, and here is my first patch.

I hope you will find it appropriate, I didn't study the test suite to have it enforce that files shouldn't be sourced from the "system" installation. I couldn't reproduce it after a quick look, and I'm not familiar enough to tinker with it yet, so I reached my trial&error quota before I could figure things out.

Best Regards,
Dridi

 Documentation/CodingGuidelines            | 22 ++++++++++++++++++++++
 contrib/convert-grafts-to-replace-refs.sh |  3 ++-
 contrib/rerere-train.sh                   |  3 ++-
 contrib/subtree/git-subtree.sh            |  1 -
 git-sh-setup.sh                           |  2 +-
 5 files changed, 27 insertions(+), 4 deletions(-)

index c4cb5ff0d..fdc0d3318 100644
@@ -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
+
+   Otherwise for a user with a custom "GIT_EXEC_PATH=/foo" the shell
+   expects "/foo:/usr/libexec/git-core/git-sh-setup" or something
+   similar depending on the installation. The git command already
+   adds the git exec path to the regular path before executing a
+   command.
+
+   For scripts that aren't run via the git command, add the git exec
+   path to the path instead.
+
+	(correct)
+	PATH="$(git --exec-path):$PATH"
+	. git-sh-setup
+
  - We do not write our "test" command with "-a" and "-o" and use "&&"
    or "||" to concatenate multiple "test" commands instead, because
    the use of "-a/-o" is often error-prone.  E.g.

index 0cbc917b8..f7d2fab03 100755
@@ -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
 
 test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
 

index 52ad9e41f..07bad67e6 100755
@@ -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
 require_work_tree
 cd_to_toplevel
 

index dec085a23..1d61f75d0 100755
@@ -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
 
 require_work_tree

index 378928518..12e1220f8 100644
@@ -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
 
 die () {
 	die_with_status 1 "$@"

2.13.5

Hi,

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

[...]

	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

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

I am not sure if this is broken in the first place. We do want to make sure that the scripted porcelains will source the shell helper library from matching Git release. The proposed patch goes directly against that and I do not see how it could be an improvement.

To me, all the instances of the unhappiness in your review seem to be caused by the unjustified declaration that it is bad to source from the directory "gti --exec-path" reports. If a user wants to tweak things Git does, why should the user futz with his own copy of sh-setup and force scripted Porcelains to read from it, which would only affect the scripted Porcelains and have no chance of affecting compiled commands? Is building from the source so bad for an open source tool?

It used to be that git allowed setting a colon-separated list of paths in GIT_EXEC_PATH. (Very long ago, I relied on that feature.) If we can restore that functionality without too much cost, then I think it's worthwhile.

But the cost in this patch for that is pretty high. And

	$ git log -S'$(git --exec-path)/'

tells me that colon-separated paths in GIT_EXEC_PATH did not work for some use cases as far back as 2006. Since 2008 the documentation has encouraged using "git --exec-path" in a way that does not work with colon-separated paths, too. I hadn't realized it was so long. Given that it hasn't been supported for more than ten years, I was wrong to read this as a bug report --- instead, it's a feature request.

In that context, this cost is likely not worth paying.

I wonder if there's another way to achieve this patch's goal. E.g. what if there were a way to specify some paths git could search for custom commands, separate from "git --exec-path"?

Putting the custom commands on the $PATH seems nicer unless you're trying to override the implementation of an existing git command. And we already discourage overriding the implementation of an existing git command (as it's open source, you can patch them instead), so...

Another possible motivation (the one that led me to use this mechnism long ago) is avoiding providing the dashed form git-$cmd in $PATH. I think time has shown that having git-$cmd in $PATH is not too painful.

Thanks and hope that helps,
Jonathan

I think the first sentence is where you went wrong. It seems that you think this ought to work:

rm -fr $HOME/random-stuff
mkdir $HOME/random-stuff
echo "echo happy" >$HOME/random-stuff/git-happy chmod +x $HOME/random-stuff/git-happy GIT_EXEC_PATH=$HOME/random-stuff
export GIT_EXEC_PATH
# then...
git happy

But that is not the right/officially sanctioned/kosher way to add custom git commands (adding your directory that has git-happy in it to $PATH is). GIT_EXEC_PATH is for the git-cmd binaries and scripts we ship; it always is used to find non built-in commands, and even for built-in commands, the command found via alias look-up is invoked that way.

By insisting on overriding GIT_EXEC_PATH and not populating with the stuff we ship, you'd need a workaround like your patch just to make the scripts "work" again. I have a feeling that even with your patch you wouldn't be able to make non built-in commands, unless you copy them (or write a thin wrapper that exec's the real thing).

So, instead of the two GIT_EXEC_PATH steps in the above example, you can do

PATH=$HOME/random-stuff:$PATH

and you'll see "git happy" to work, I would think, without breaking other things.

Hi all,

Thanks for the fast feedback, I'll answer everyone in a single email if you don't mind.

On Fri, Sep 29, 2017 at 5:48 AM, Jonathan Nieder <[email protected]> wrote:
snip

Now that you pointed out that I missed the relevant documentations, I don't think this belongs in the guidelines at all.

snip

That is embarrassing, I thought I had done my research properly...

The PATH is already "polluted" when git-* commands are run via git, and in the context of a script using git-sh-setup I wouldn't consider that completely irrelevant.

But the problem is that just by having a GIT_EXEC_PATH you will source an incorrect file name. If there was something like --exec-dir that wouldn't take the PATH into account. Before I tried to contribute a fix, my local patching of git-sh-setup after git-core upgrades was actually this:

    -. "$(git --exec-path)/git-sh-i18n"
    +. "$(GIT_EXEC_PATH= git --exec-path)/git-sh-i18n"

That's not pretty, but it gives the guarantee to source from matching Git release. Considering the PATH semantics, this is how I would fix it after reading your feedback.

Well, from my perspective it's a bug report, upgrading git caused a regression in my setup. I didn't know I was doing it wrong ;)

snip

In my case, yes, I'm maintaining commands but don't really want to pollute my general-purpose PATH. But I can live with that and use PATH instead.

Exactly!

The git(1) manual says:

This is why I set up my system like this years ago. The manual could say when the envvar usage is appropriate.

I forgot about that, and I actually had to look at my bashrc to realize that GIT_EXEC_PATH completely overrides the exec path. I did this, and forgot about it once I was done:

    export GIT_EXEC_PATH=<my-path>:"$(GIT_EXEC_PATH= git --exec-path)"

So for so while I managed to convince myself that git --exec-path always includes the local installation path. Today I re-learn.

That creates a gap between git-core commands being in libexec outside of the PATH and my git-* commands having to be in the PATH even though they aren't meant to be executed directly.

Should I attempt a new patch? I'd make sure to source by neutering GIT_EXEC_PATH and update the relevant documentation this time.

Thanks for the review, it has been very instructive, even for a simple shell problem!

Dridi