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@xxxxxxxxx> 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 <gitster@xxxxxxxxx> wrote:
> Jonathan Nieder <jrnieder@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> Dridi Boukelmoune <dridi.boukelmoune@xxxxxxxxx> 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!
> 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
