Re: [PATCH] global: resolve Perl executable via PATH

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 18, 2023, 08:59 UTC
Message-ID: <643e5be864894_21c9542949d@chronos.notmuch>
In-Reply-To: <230406.86y1n5tnvk.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason wrote:

Show 60 quoted lines

> 
> 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@peff.net> 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@coredump.intra.peff.net> in the side-thread.

That is true, however, the fact that something isn't *necessary* doesn't mean it isn't good.

> Using "env" liket his is also incorrect.

No, it's not.

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

And you can keep doing that as Patrick's patch does not change that priority.

I applied both Patrick's patch and Jeff's patch, and then did:

  ln -s /bin/false ~/bin/perl

Guess what... Everything works fine (as it should). $PEARL_PATH should override the shebangs (and it does), so it does not matter what `/usr/bin/env perl` returns... unless somebody does run the offending scripts manually, in which case they are on their own.

So, I think Patrick's patch should still be applied, but in practice makes no difference, because Jeff's patch fixes the actual problems.

Or another way to put it: Patrick's patch is unnecessary, but in my opinion still desirable.

Cheers.

-- 
Felipe Contreras