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 <jrnie...@gmail.com> 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 <gits...@pobox.com> wrote:
> Jonathan Nieder <jrnie...@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 <jrnie...@gmail.com> wrote:
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnie...@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 <gits...@pobox.com> wrote:
> Dridi Boukelmoune <dridi.boukelmo...@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
> ech
