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

From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Sep 29, 2017, 11:58 UTC
Message-ID: <CAJYncsiou3WjNhvn4YuzjuyqGknSgWmXewFhgD5M1j+Wsmmefw@mail.gmail.com>
In-Reply-To: <xmqqh8vmxfoq.fsf@gitster.mtv.corp.google.com>

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 <jrnieder@gmail.com> wrote: snip

Show 6 quoted lines

> 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 <gitster@pobox.com> wrote:

Show 9 quoted lines

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> 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 <jrnieder@gmail.com> wrote:

Show 26 quoted lines

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> 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 <gitster@pobox.com> wrote:

Show 18 quoted lines

> Dridi Boukelmoune <dridi.boukelmoune@gmail.com> writes:
>
>> 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!

Show 6 quoted lines

> 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.

Show 10 quoted lines

> 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