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

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 [off-list ref] wrote:
snip

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.

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

snip

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

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

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.

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.

On Fri, Sep 29, 2017 at 5:58 AM, Junio C Hamano [off-list ref] wrote:

Jonathan Nieder [off-list ref] writes:

quoted

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

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.

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.

On Fri, Sep 29, 2017 at 6:21 AM, Jonathan Nieder [off-list ref] wrote:

Junio C Hamano wrote:

quoted

Jonathan Nieder [off-list ref] writes:

quoted

quoted

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

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.

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.

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

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.

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.

On Fri, Sep 29, 2017 at 7:00 AM, Junio C Hamano [off-list ref] wrote:

Dridi Boukelmoune [off-list ref] writes:

quoted

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.

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

Exactly!

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.

The git(1) manual says:

This can also be controlled by setting the GIT_EXEC_PATH environment
variable.

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

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

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.

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.

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