On Wed, Apr 05 2023, Jeff King wrote: > On Wed, Apr 05, 2023 at 09:18:20PM -0500, Felipe Contreras wrote: > >> On Wed, Apr 5, 2023 at 2:09 PM Jeff King <peff@xxxxxxxx> wrote: >> > On Wed, Apr 05, 2023 at 07:32:22PM +0200, Patrick Steinhardt wrote: >> >> > IMHO we should aim for fixing those inconsistencies, and then letting >> > people set PERL_PATH as appropriate (even to something that will find it >> > via $PATH if they want to). >> >> We can aim to fix all those inconsistencies *eventually* while in the >> meantime make them runnable for most people *today*. >> >> It's not a dichotomy. > > It is if the proposed patches change the behavior in such a way as to > make things less consistent. > > There are three plausible perls to run (whether intentionally or > accidentally): > > 1. the one in PERL_PATH > > 2. /usr/bin/perl > > 3. the first one in $PATH > > What the code tries to do now is to consistently use (1). If there are > cases that accidentally use (2), which is what I took Patrick's patch to > mean, then that is a problem for people who set PERL_PATH to something > else, but not for people who leave it as /usr/bin/perl. If we "fix" > those cases by switching them to (3), then now things are less > consistent for such people than when we started. > > But I am not clear on what those cases are (if any), and we have not > seen Patrick's follow-up proposed patch. > > I did find one case that is accidentally doing (3), and posted a patch > elsewhere in the thread to convert it to (1). If you prefer behavior > (3), you might consider that a regression, but it seems meaningless > given the 99% of other cases that are using (1). If you want (3) to be > the behavior everywhere, then we'd need to completely change our stance > on how we invoke perl, or we need to teach PERL_PATH to handle this case > so that people building Git can choose their own preference (sadly I > don't think "make PERL_PATH='/usr/bin/env perl'" quite works because we > have to shell-quote it in some contexts before evaluating). I just want to chime in to say that I've read this whole thread-at-large, and I think what you're pointing out here is correct, and that we should keep hardcoding "#!/usr/bin/perl", and then just have "PERL_PATH" set. I.e. most of Patrick's original patch is unnecessary, as we either use "$PERL_PATH" in the Makefile already, or munge the shebang when we install. Then the only change we should need is the one you suggested in <20230406032647.GA2092142@xxxxxxxxxxxxxxxxxxxxxxx> in the side-thread. Using "env" liket his is also incorrect. I might have a "perl" in my "$PATH" which I expect to use for e.g. by .bashrc, but I don't want that perl to take priority over "$PERL_PATH" for git when I run some test script. I also wonder if something like this (untested) wouldn't be useful to provide an earlier warning of this, instead of failing when we fail to invoke the relevant scripts. diff --git a/Makefile b/Makefile index 60ab1a8b4f4..9abc2e52cfa 100644 --- a/Makefile +++ b/Makefile @@ -910,6 +910,15 @@ ifndef PYTHON_PATH PYTHON_PATH = /usr/bin/python endif +define check-path-exists +ifeq ($$(wildcard $$($(1))),) +$$(error $(1) set to '$(2)', which does not exist) +endif +endef + +$(eval $(call check-path-exists,SHELL_PATH,$(SHELL_PATH))) +$(eval $(call check-path-exists,PERL_PATH,$(PERL_PATH))) + export PERL_PATH export PYTHON_PATH That should not break anything in principle, as we already rely on these to build git itself, but note that that's not the case with "PYTHON_PATH". In the case of "PERL_PATH" I think that's limited to building the docs, so if we did something like this perhaps it should be in shared.mak, and that check should be in Documentation/Makefile. But perhaps it's all reduntant to just running into an error when we try to generate-cmdlist.sh or whatever...
