{"thread":{"id":"59549","subject":"[PATCH] global: resolve Perl executable via PATH","startedAt":"2023-04-05T10:10:23Z","lastAt":"2023-04-18T09:05:56Z","messageCount":27,"participants":["Patrick Steinhardt","Felipe Contreras","Todd Zullinger","Jeff King","Kristoffer Haugsbakk","Junio C Hamano","Eric Wong","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474834","messageId":"d9cfad7caf9ff5bf88eb06cf7bb3be5e70e6d96f.1680689378.git.ps@pks.im","threadId":"59549","inReplyTo":null,"subject":"[PATCH] global: resolve Perl executable via PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-05T10:10:10Z","receivedAt":"2023-04-05T10:10:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The majority of Perl scripts we carry in Git have a `#!/usr/bin/perl`\nshebang. This is not a portable location for the Perl interpreter and\nmay thus break on some systems that have the interpreter installed in a\ndifferent location. One such example is NixOS, where the only executable\ninstalled in `/usr/bin` is env(1).\n\nConvert the shebangs to resolve the location of the Perl interpreter via\nenv(1) to make these scripts more portable. While the location of env(1)\nis not guaranteed by any standard either, in practice all distributions\nincluding NixOS have it available at `/usr/bin/env`. We're also already\nusing this idiom in a small set of other scripts, and until now nobody\ncomplained about them.\n\nThis makes the test suite pass on NixOS.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/build-docdep.perl | 2 +-\n Documentation/cat-texi.perl | 2 +-\n Documentation/cmd-list.perl | 3 ++-\n Documentation/fix-texi.perl | 4 +++-\n Documentation/lint-fsck-msgids.perl | 2 +-\n Documentation/lint-gitlink.perl | 2 +-\n Documentation/lint-man-end-blurb.perl | 2 +-\n Documentation/lint-man-section-order.perl | 2 +-\n compat/vcbuild/scripts/clink.pl | 5 ++++-\n compat/vcbuild/scripts/lib.pl | 5 ++++-\n contrib/buildsystems/CMakeLists.txt | 2 +-\n contrib/buildsystems/engine.pl | 5 ++++-\n contrib/buildsystems/generate | 5 ++++-\n contrib/buildsystems/parse.pl | 5 ++++-\n contrib/contacts/git-contacts | 2 +-\n contrib/credential/netrc/git-credential-netrc.perl | 2 +-\n contrib/credential/netrc/test.pl | 2 +-\n contrib/diff-highlight/Makefile | 2 +-\n contrib/fast-import/git-import.perl | 2 +-\n contrib/fast-import/import-directories.perl | 2 +-\n contrib/fast-import/import-tars.perl | 2 +-\n contrib/hooks/setgitperms.perl | 2 +-\n contrib/hooks/update-paranoid | 2 +-\n contrib/long-running-filter/example.pl | 2 +-\n contrib/mw-to-git/git-mw.perl | 2 +-\n contrib/mw-to-git/git-remote-mediawiki.perl | 2 +-\n contrib/mw-to-git/t/test-gitmw.pl | 6 +++++-\n contrib/stats/mailmap.pl | 2 +-\n contrib/stats/packinfo.pl | 2 +-\n contrib/subtree/t/Makefile | 2 +-\n git-archimport.perl | 2 +-\n git-cvsexportcommit.perl | 2 +-\n git-cvsimport.perl | 2 +-\n git-cvsserver.perl | 2 +-\n git-send-email.perl | 2 +-\n git-svn.perl | 2 +-\n gitweb/gitweb.perl | 2 +-\n t/Git-SVN/Utils/can_compress.t | 2 +-\n t/Git-SVN/Utils/fatal.t | 2 +-\n t/check-non-portable-shell.pl | 2 +-\n t/lib-gitweb.sh | 2 +-\n t/perf/aggregate.perl | 2 +-\n t/perf/min_time.perl | 2 +-\n t/t0019/parse_json.perl | 2 +-\n t/t0202/test.pl | 2 +-\n t/t0210/scrub_normal.perl | 2 +-\n t/t0211/scrub_perf.perl | 2 +-\n t/t0212/parse_events.perl | 2 +-\n t/t4034/perl/post | 2 +-\n t/t4034/perl/pre | 2 +-\n t/t7519/fsmonitor-all-v2 | 2 +-\n t/t7519/fsmonitor-watchman | 2 +-\n t/t7519/fsmonitor-watchman-v2 | 2 +-\n t/t9700/test.pl | 2 +-\n t/test-terminal.perl | 2 +-\n templates/hooks--fsmonitor-watchman.sample | 2 +-\n 56 files changed, 78 insertions(+), 56 deletions(-)\n\ndiff --git a/Documentation/build-docdep.perl b/Documentation/build-docdep.perl\nindex 1b3ac8fdd9..fe6a1e724d 100755\n--- a/Documentation/build-docdep.perl\n+++ b/Documentation/build-docdep.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n my %include = ();\n my %included = ();\ndiff --git a/Documentation/cat-texi.perl b/Documentation/cat-texi.perl\nindex 14d2f83415..5b44d64f7d 100755\n--- a/Documentation/cat-texi.perl\n+++ b/Documentation/cat-texi.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl -w\n+#!/usr/bin/env perl\n \n use strict;\n use warnings;\ndiff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl\nindex 755a110bc4..3fe43b8968 100755\n--- a/Documentation/cmd-list.perl\n+++ b/Documentation/cmd-list.perl\n@@ -1,5 +1,6 @@\n-#!/usr/bin/perl -w\n+#!/usr/bin/env perl\n \n+use warnings;\n use File::Compare qw(compare);\n \n sub format_one {\ndiff --git a/Documentation/fix-texi.perl b/Documentation/fix-texi.perl\nindex ff7d78f620..61287a0a9e 100755\n--- a/Documentation/fix-texi.perl\n+++ b/Documentation/fix-texi.perl\n@@ -1,4 +1,6 @@\n-#!/usr/bin/perl -w\n+#!/usr/bin/env perl\n+\n+use warnings;\n \n while (<>) {\n \tif (/^\\@setfilename/) {\ndiff --git a/Documentation/lint-fsck-msgids.perl b/Documentation/lint-fsck-msgids.perl\nindex 1233ffe815..fd2209e4f8 100755\n--- a/Documentation/lint-fsck-msgids.perl\n+++ b/Documentation/lint-fsck-msgids.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n my ($fsck_h, $fsck_msgids_txt, $okfile) = @ARGV;\n \ndiff --git a/Documentation/lint-gitlink.perl b/Documentation/lint-gitlink.perl\nindex 1c61dd9512..b27418ee16 100755\n--- a/Documentation/lint-gitlink.perl\n+++ b/Documentation/lint-gitlink.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use strict;\n use warnings;\ndiff --git a/Documentation/lint-man-end-blurb.perl b/Documentation/lint-man-end-blurb.perl\nindex 6bdb13ad9f..d1ee7ba130 100755\n--- a/Documentation/lint-man-end-blurb.perl\n+++ b/Documentation/lint-man-end-blurb.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use strict;\n use warnings;\ndiff --git a/Documentation/lint-man-section-order.perl b/Documentation/lint-man-section-order.perl\nindex 02408a0062..27921ef66f 100755\n--- a/Documentation/lint-man-section-order.perl\n+++ b/Documentation/lint-man-section-order.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use strict;\n use warnings;\ndiff --git a/compat/vcbuild/scripts/clink.pl b/compat/vcbuild/scripts/clink.pl\nindex 3bd824154b..74be842ebd 100755\n--- a/compat/vcbuild/scripts/clink.pl\n+++ b/compat/vcbuild/scripts/clink.pl\n@@ -1,4 +1,7 @@\n-#!/usr/bin/perl -w\n+#!/usr/bin/env perl\n+\n+use warnings;\n+\n ######################################################################\n # Compiles or links files\n #\ndiff --git a/compat/vcbuild/scripts/lib.pl b/compat/vcbuild/scripts/lib.pl\nindex d8054e469f..6f2c2cb89f 100755\n--- a/compat/vcbuild/scripts/lib.pl\n+++ b/compat/vcbuild/scripts/lib.pl\n@@ -1,4 +1,7 @@\n-#!/usr/bin/perl -w\n+#!/usr/bin/env perl\n+\n+use warnings;\n+\n ######################################################################\n # Libifies files on Windows\n #\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 2f6e0197ff..43d64909cb 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -848,7 +848,7 @@ string(REPLACE \"@@INSTLIBDIR@@\" \"${INSTLIBDIR}\" perl_header \"${perl_header}\")\n \n foreach(script ${git_perl_scripts})\n \tfile(STRINGS ${CMAKE_SOURCE_DIR}/${script}.perl content NEWLINE_CONSUME)\n-\tstring(REPLACE \"#!/usr/bin/perl\" \"#!/usr/bin/perl\\n${perl_header}\\n\" content \"${content}\")\n+\tstring(REPLACE \"#!/usr/bin/env perl\" \"#!/usr/bin/env perl\\n${perl_header}\\n\" content \"${content}\")\n \tstring(REPLACE \"@@GIT_VERSION@@\" \"${PROJECT_VERSION}\" content \"${content}\")\n \tfile(WRITE ${CMAKE_BINARY_DIR}/${script} ${content})\n endforeach()\ndiff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl\nindex ed6c45988a..ea32de593c 100755\n--- a/contrib/buildsystems/engine.pl\n+++ b/contrib/buildsystems/engine.pl\n@@ -1,4 +1,7 @@\n-#!/usr/bin/perl -w\n+#!/usr/bin/env perl\n+\n+use warnings;\n+\n ######################################################################\n # Do not call this script directly!\n #\ndiff --git a/contrib/buildsystems/generate b/contrib/buildsystems/generate\nindex bc10f25ff2..41c1967496 100755\n--- a/contrib/buildsystems/generate\n+++ b/contrib/buildsystems/generate\n@@ -1,4 +1,7 @@\n-#!/usr/bin/perl -w\n+#!/usr/bin/env perl\n+\n+use warnings;\n+\n ######################################################################\n # Generate buildsystem files\n #\ndiff --git a/contrib/buildsystems/parse.pl b/contrib/buildsystems/parse.pl\nindex c9656ece99..03050ab28d 100755\n--- a/contrib/buildsystems/parse.pl\n+++ b/contrib/buildsystems/parse.pl\n@@ -1,4 +1,7 @@\n-#!/usr/bin/perl -w\n+#!/usr/bin/env perl\n+\n+use warnings;\n+\n ######################################################################\n # Do not call this script directly!\n #\ndiff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts\nindex 85ad732fc0..a9ce8edb85 100755\n--- a/contrib/contacts/git-contacts\n+++ b/contrib/contacts/git-contacts\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n # List people who might be interested in a patch. Useful as the argument to\n # git-send-email --cc-cmd option, and in other situations.\ndiff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl\nindex 9fb998ae09..514f68d00b 100755\n--- a/contrib/credential/netrc/git-credential-netrc.perl\n+++ b/contrib/credential/netrc/git-credential-netrc.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use strict;\n use warnings;\ndiff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl\nindex c0fb3718b2..4387379d52 100755\n--- a/contrib/credential/netrc/test.pl\n+++ b/contrib/credential/netrc/test.pl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use warnings;\n use strict;\ndiff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile\nindex f2be7cc924..9da2a0b442 100644\n--- a/contrib/diff-highlight/Makefile\n+++ b/contrib/diff-highlight/Makefile\n@@ -1,6 +1,6 @@\n all: diff-highlight\n \n-PERL_PATH = /usr/bin/perl\n+PERL_PATH = /usr/bin/env perl\n -include ../../config.mak\n \n PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))\ndiff --git a/contrib/fast-import/git-import.perl b/contrib/fast-import/git-import.perl\nindex 0891b9e366..440790523d 100755\n--- a/contrib/fast-import/git-import.perl\n+++ b/contrib/fast-import/git-import.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n #\n # Performs an initial import of a directory. This is the equivalent\n # of doing 'git init; git add .; git commit'. It's a little slower,\ndiff --git a/contrib/fast-import/import-directories.perl b/contrib/fast-import/import-directories.perl\nindex a16f79cfdc..5ea90dada9 100755\n--- a/contrib/fast-import/import-directories.perl\n+++ b/contrib/fast-import/import-directories.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n #\n # Copyright 2008-2009 Peter Krefting \n #\ndiff --git a/contrib/fast-import/import-tars.perl b/contrib/fast-import/import-tars.perl\nindex d50ce26d5d..d28c7b1a4a 100755\n--- a/contrib/fast-import/import-tars.perl\n+++ b/contrib/fast-import/import-tars.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n ## tar archive frontend for git-fast-import\n ##\ndiff --git a/contrib/hooks/setgitperms.perl b/contrib/hooks/setgitperms.perl\nindex 2770a1b1d2..18444e0157 100755\n--- a/contrib/hooks/setgitperms.perl\n+++ b/contrib/hooks/setgitperms.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n #\n # Copyright (c) 2006 Josh England\n #\ndiff --git a/contrib/hooks/update-paranoid b/contrib/hooks/update-paranoid\nindex 0092d67b8a..a85d3badd1 100755\n--- a/contrib/hooks/update-paranoid\n+++ b/contrib/hooks/update-paranoid\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use strict;\n use File::Spec;\ndiff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl\nindex a677569ddd..0ca49e370b 100755\n--- a/contrib/long-running-filter/example.pl\n+++ b/contrib/long-running-filter/example.pl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n #\n # Example implementation for the Git filter protocol version 2\n # See Documentation/gitattributes.txt, section \"Filter Protocol\"\ndiff --git a/contrib/mw-to-git/git-mw.perl b/contrib/mw-to-git/git-mw.perl\nindex eb52a53d32..cb0cd63ea7 100755\n--- a/contrib/mw-to-git/git-mw.perl\n+++ b/contrib/mw-to-git/git-mw.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n # Copyright (C) 2013\n # Benoit Person \ndiff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl\nindex a5624413dc..9ae596f3b7 100755\n--- a/contrib/mw-to-git/git-remote-mediawiki.perl\n+++ b/contrib/mw-to-git/git-remote-mediawiki.perl\n@@ -1,4 +1,4 @@\n-#! /usr/bin/perl\n+#!/usr/bin/env perl\n \n # Copyright (C) 2011\n # Jérémie Nikaes \ndiff --git a/contrib/mw-to-git/t/test-gitmw.pl b/contrib/mw-to-git/t/test-gitmw.pl\nindex c5d687f078..9005837713 100755\n--- a/contrib/mw-to-git/t/test-gitmw.pl\n+++ b/contrib/mw-to-git/t/test-gitmw.pl\n@@ -1,4 +1,8 @@\n-#!/usr/bin/perl -w -s\n+#!/usr/bin/env perl\n+\n+use strict;\n+use warnings;\n+\n # Copyright (C) 2012\n # Charles Roussel \n # Simon Cathebras \ndiff --git a/contrib/stats/mailmap.pl b/contrib/stats/mailmap.pl\nindex 9513f5e35b..469af8240c 100755\n--- a/contrib/stats/mailmap.pl\n+++ b/contrib/stats/mailmap.pl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use warnings 'all';\n use strict;\ndiff --git a/contrib/stats/packinfo.pl b/contrib/stats/packinfo.pl\nindex be188c0f11..51823ac942 100755\n--- a/contrib/stats/packinfo.pl\n+++ b/contrib/stats/packinfo.pl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n #\n # This tool will print vaguely pretty information about a pack. It\n # expects the output of \"git verify-pack -v\" as input on stdin.\ndiff --git a/contrib/subtree/t/Makefile b/contrib/subtree/t/Makefile\nindex 093399c788..b22cd62236 100644\n--- a/contrib/subtree/t/Makefile\n+++ b/contrib/subtree/t/Makefile\n@@ -8,7 +8,7 @@\n \n #GIT_TEST_OPTS=--verbose --debug\n SHELL_PATH ?= $(SHELL)\n-PERL_PATH ?= /usr/bin/perl\n+PERL_PATH ?= /usr/bin/env perl\n TAR ?= $(TAR)\n RM ?= rm -f\n PROVE ?= prove\ndiff --git a/git-archimport.perl b/git-archimport.perl\nindex b7c173c345..3f46e0c1dc 100755\n--- a/git-archimport.perl\n+++ b/git-archimport.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n #\n # This tool is copyright (c) 2005, Martin Langhoff.\n # It is released under the Gnu Public License, version 2.\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 289d4bc684..c674c22e4f 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use 5.008;\n use strict;\ndiff --git a/git-cvsimport.perl b/git-cvsimport.perl\nindex 7bf3c12d67..56011991d6 100755\n--- a/git-cvsimport.perl\n+++ b/git-cvsimport.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n # This tool is copyright (c) 2005, Matthias Urlichs.\n # It is released under the Gnu Public License, version 2.\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex 7b757360e2..9388efddf2 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n ####\n #### This application is a CVS emulation layer for git.\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 07f2a0cbea..d532c08439 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n #\n # Copyright 2002,2005 Greg Kroah-Hartman \n # Copyright 2005 Ryan Anderson \ndiff --git a/git-svn.perl b/git-svn.perl\nindex be987e316f..1ba835613b 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n # Copyright (C) 2006, Eric Wong \n # License: GPL v2 or later\n use 5.008;\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e66eb3d9ba..943099a024 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n # gitweb - simple web interface to track changes in git repositories\n #\ndiff --git a/t/Git-SVN/Utils/can_compress.t b/t/Git-SVN/Utils/can_compress.t\nindex d7b49b8d54..0cdedb25bf 100755\n--- a/t/Git-SVN/Utils/can_compress.t\n+++ b/t/Git-SVN/Utils/can_compress.t\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use strict;\n use warnings;\ndiff --git a/t/Git-SVN/Utils/fatal.t b/t/Git-SVN/Utils/fatal.t\nindex 49e1438295..75aaf1633d 100755\n--- a/t/Git-SVN/Utils/fatal.t\n+++ b/t/Git-SVN/Utils/fatal.t\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n use strict;\n use warnings;\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex dd8107cd7d..6fbec0162e 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -1,4 +1,4 @@\n-#!/usr/bin/perl\n+#!/usr/bin/env perl\n \n # Test t0000..t9999.sh for non portable shell scripts\n # This script can be called with one or more filenames as parameters\ndiff --git a/t/lib-gitweb.sh b/t/lib-gitweb.sh\nindex 1f32ca66ea..882c2e96f1 100644\n--- a/t/lib-gitweb.sh\n+++ b/t/lib-gitweb.sh\n@@ -7,7 +7,7 @@\n gitweb_init () {\n \tsafe_pwd=\"$(perl -MPOSIX=getcwd -e 'print quotemeta(getcwd)')\"\n \tcat >gitweb_config.perl < wrote:\n>\n> The majority of Perl scripts we carry in Git have a `#!/usr/bin/perl`\n> shebang. This is not a portable location for the Perl interpreter and\n> may thus break on some systems that have the interpreter installed in a\n> different location. One such example is NixOS, where the only executable\n> installed in `/usr/bin` is env(1).\n>\n> Convert the shebangs to resolve the location of the Perl interpreter via\n> env(1) to make these scripts more portable. While the location of env(1)\n> is not guaranteed by any standard either, in practice all distributions\n> including NixOS have it available at `/usr/bin/env`. We're also already\n> using this idiom in a small set of other scripts, and until now nobody\n> complained about them.\n\nThis is standard practice in Ruby, and it does seem to work everywhere.\n\nHowever, I wonder if /bin/env does also work. I can't imagine a system\nsystem providing /usr/bin/env but not /bin/env.\n\n-- \nFelipe Contreras\n"},{"id":"474846","messageId":"ZC2I7CfVzY6Tl7Pk@pobox.com","threadId":"59549","inReplyTo":"d9cfad7caf9ff5bf88eb06cf7bb3be5e70e6d96f.1680689378.git.ps@pks.im","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2023-04-05T14:42:52Z","receivedAt":"2023-04-05T14:43:16Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Patrick Steinhardt wrote:\n> The majority of Perl scripts we carry in Git have a `#!/usr/bin/perl`\n> shebang. This is not a portable location for the Perl interpreter and\n> may thus break on some systems that have the interpreter installed in a\n> different location. One such example is NixOS, where the only executable\n> installed in `/usr/bin` is env(1).\n\nIs there a reason to not set PERL_PATH, which is the\ndocumented method to handle this? From the Makefike:\n\n# Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n\n-- \nTodd\n"},{"id":"474847","messageId":"ZC2KICsJN5pmsqWX@ncase","threadId":"59549","inReplyTo":"CAMP44s0eLNOWFr7fc6M5Fompw1Y13vAxk8=fAWVZ8-22Y-xihg@mail.gmail.com","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-05T14:48:00Z","receivedAt":"2023-04-05T14:48:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 05, 2023 at 08:35:50AM -0500, Felipe Contreras wrote:\n> On Wed, Apr 5, 2023 at 5:53 AM Patrick Steinhardt wrote:\n> >\n> > The majority of Perl scripts we carry in Git have a `#!/usr/bin/perl`\n> > shebang. This is not a portable location for the Perl interpreter and\n> > may thus break on some systems that have the interpreter installed in a\n> > different location. One such example is NixOS, where the only executable\n> > installed in `/usr/bin` is env(1).\n> >\n> > Convert the shebangs to resolve the location of the Perl interpreter via\n> > env(1) to make these scripts more portable. While the location of env(1)\n> > is not guaranteed by any standard either, in practice all distributions\n> > including NixOS have it available at `/usr/bin/env`. We're also already\n> > using this idiom in a small set of other scripts, and until now nobody\n> > complained about them.\n> \n> This is standard practice in Ruby, and it does seem to work everywhere.\n> \n> However, I wonder if /bin/env does also work. I can't imagine a system\n> system providing /usr/bin/env but not /bin/env.\n\nNixOS does indeed only have /usr/bin/env and does not have /bin/env, so\nit wouldn't.\n\nPatrick\n"},{"id":"474848","messageId":"ZC2LOAwycdaUawxM@ncase","threadId":"59549","inReplyTo":"ZC2I7CfVzY6Tl7Pk@pobox.com","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-05T14:52:40Z","receivedAt":"2023-04-05T14:52:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 05, 2023 at 10:42:52AM -0400, Todd Zullinger wrote:\n> Patrick Steinhardt wrote:\n> > The majority of Perl scripts we carry in Git have a `#!/usr/bin/perl`\n> > shebang. This is not a portable location for the Perl interpreter and\n> > may thus break on some systems that have the interpreter installed in a\n> > different location. One such example is NixOS, where the only executable\n> > installed in `/usr/bin` is env(1).\n> \n> Is there a reason to not set PERL_PATH, which is the\n> documented method to handle this? From the Makefike:\n> \n> # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n\nSetting PERL_PATH helps with a subset of invocations where the Makefile\neither executes Perl directly or where it writes the shebang itself. But\nthe majority of scripts I'm touching have `#!/usr/bin/perl` as shebang,\nand that path is not adjusted by setting PERL_PATH.\n\nI'd be happy to amend the patch series to only fix up shebangs which\nwould not be helped by setting PERL_PATH. But if we can make it work\nwithout having to set PERL_PATH at all I don't quite see the point.\n\nPatrick\n"},{"id":"474858","messageId":"ZC2ZyTTZFbd_gNtw@pobox.com","threadId":"59549","inReplyTo":"ZC2LOAwycdaUawxM@ncase","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2023-04-05T15:54:49Z","receivedAt":"2023-04-05T15:55:02Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Patrick Steinhardt wrote:\n> On Wed, Apr 05, 2023 at 10:42:52AM -0400, Todd Zullinger wrote:\n>> Is there a reason to not set PERL_PATH, which is the\n>> documented method to handle this? From the Makefike:\n>> \n>> # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n> \n> Setting PERL_PATH helps with a subset of invocations where the Makefile\n> either executes Perl directly or where it writes the shebang itself. But\n> the majority of scripts I'm touching have `#!/usr/bin/perl` as shebang,\n> and that path is not adjusted by setting PERL_PATH.\n\nAhh. I wonder if that's intentional? I haven't dug into\nthe history, so I'm not sure. It seems like an oversight,\nas an initial reaction.\n\n> I'd be happy to amend the patch series to only fix up shebangs which\n> would not be helped by setting PERL_PATH. But if we can make it work\n> without having to set PERL_PATH at all I don't quite see the point.\n\nIt's certainly debatable whether using /path/to/env perl is\nbetter than hard-coding it at build time (forgetting about\nthe usage of RUNTIME_PREFIX). [Debatable in a friendly\nsense, of course.]\n\nAs a distribution packager, I prefer to set the path at\nbuild time to help ensure that an end user can't easily\nbreak things by installing a different perl in PATH.\n\nThe Fedora build system will munge /path/to/env perl\nshebangs to /usr/bin/perl and it won't effect us much.\n\nThat may not be true for other distributions and they may\ncare more if they want to keep using a hard-coded path to\nperl. I don't know how it may affects the Windows folks\neither, who are further askew from our other, more UNIX-like\ntargets.\n\nI don't know what the right choice is for upstream Git, it\ncan easily be argued in either direction. :)\n\nCheers,\n\n-- \nTodd\n"},{"id":"474861","messageId":"20230405165414.GA497301@coredump.intra.peff.net","threadId":"59549","inReplyTo":"ZC2LOAwycdaUawxM@ncase","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-05T16:54:14Z","receivedAt":"2023-04-05T16:55:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 05, 2023 at 04:52:40PM +0200, Patrick Steinhardt wrote:\n\n> > Is there a reason to not set PERL_PATH, which is the\n> > documented method to handle this? From the Makefike:\n> > \n> > # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n> \n> Setting PERL_PATH helps with a subset of invocations where the Makefile\n> either executes Perl directly or where it writes the shebang itself. But\n> the majority of scripts I'm touching have `#!/usr/bin/perl` as shebang,\n> and that path is not adjusted by setting PERL_PATH.\n\nWhich scripts? If I do:\n\n mkdir /tmp/foo\n ln -s /usr/bin/perl /tmp/foo/my-perl\n make PERL_PATH=/tmp/foo/my-perl prefix=/tmp/foo install\n\n head -n 1 /tmp/foo/bin/git-cvsserver\n\nThen I see:\n\n #!/tmp/foo/my-perl\n\nAnd that is due to this segment in the Makefile:\n\n $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE\n $(QUIET_GEN) \\\n sed -e '1{' \\\n -e ' s|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n -e ' r GIT-PERL-HEADER' \\\n -e ' G' \\\n -e '}' \\\n -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n $< >$@+ && \\\n chmod +x $@+ && \\\n mv $@+ $@\n\nAnd that behavior goes all the way back to bc6146d2abc ('build' scripts\nbefore installing., 2005-09-08). If there are some perl scripts we are\n\"building\" outside of this rule, then that is probably a bug.\n\nThe only thing I found via:\n\n find /tmp/foo -type | xargs grep /usr/bin/perl\n\nwas a sample hook (which is probably a bug; we do munge the hook scripts\nto replace @PERL_PATH@, etc, but I think the Makefile never learned that\nthe template hook scripts might be something other than shell scripts).\n\n-Peff\n"},{"id":"474862","messageId":"CAMP44s1V3nGnqCjoCi2xuH59APVPiAoQ82LQx3KNnsdDNfcoPg@mail.gmail.com","threadId":"59549","inReplyTo":"ZC2ZyTTZFbd_gNtw@pobox.com","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-05T17:09:38Z","receivedAt":"2023-04-05T17:09:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Apr 5, 2023 at 11:31 AM Todd Zullinger wrote:\n>\n> Patrick Steinhardt wrote:\n> > On Wed, Apr 05, 2023 at 10:42:52AM -0400, Todd Zullinger wrote:\n> >> Is there a reason to not set PERL_PATH, which is the\n> >> documented method to handle this? From the Makefike:\n> >>\n> >> # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n> >\n> > Setting PERL_PATH helps with a subset of invocations where the Makefile\n> > either executes Perl directly or where it writes the shebang itself. But\n> > the majority of scripts I'm touching have `#!/usr/bin/perl` as shebang,\n> > and that path is not adjusted by setting PERL_PATH.\n>\n> Ahh. I wonder if that's intentional? I haven't dug into\n> the history, so I'm not sure. It seems like an oversight,\n> as an initial reaction.\n\nGenerally the people interested in setting PERL_PATH are packagers,\nand it's so the installed scripts have the desired hardcoded path.\n\nSo a script that is never installed isn't considered.\n\n> > I'd be happy to amend the patch series to only fix up shebangs which\n> > would not be helped by setting PERL_PATH. But if we can make it work\n> > without having to set PERL_PATH at all I don't quite see the point.\n>\n> It's certainly debatable whether using /path/to/env perl is\n> better than hard-coding it at build time (forgetting about\n> the usage of RUNTIME_PREFIX). [Debatable in a friendly\n> sense, of course.]\n\nIt's better because it allows the user to choose another version\ndynamically, for example:\n\n PATH=/opt/perl7/bin:$PATH script\n\n> As a distribution packager, I prefer to set the path at\n> build time to help ensure that an end user can't easily\n> break things by installing a different perl in PATH.\n\nAnd as a user I would rather packagers don't treat me like a child.\n\nI decide how to use *my* system.\n\nAnd BTW, newer generations of developers use all kinds of version\nmanagers like RVM and nvm, and perl has one as well: perlbrew [1]. Not\nto mention docker, for which the Perl official image installs into\n/usr/local/bin/perl.\n\n> The Fedora build system will munge /path/to/env perl\n> shebangs to /usr/bin/perl and it won't effect us much.\n\nSo you can override '#!/usr/bin/env perl' instead of overriding\n`#!/usr/bin/perl`, which the Git Makefile does by default anyway.\n\nWhat's the difference to you?\n\n> That may not be true for other distributions and they may\n> care more if they want to keep using a hard-coded path to\n> perl.\n\nArch Linux does whatever upstream does by default.\n\n> I don't know how it may affects the Windows folks either, who are\n> further askew from our other, more UNIX-like targets.\n\nWindows doesn't use shebangs, you have to do `perl script`.\n\n> I don't know what the right choice is for upstream Git, it\n> can easily be argued in either direction. :)\n\nI don't think it matters to packagers how people developing Git run\nthe internal scripts.\n\nCheers.\n\n[1] https://metacpan.org/pod/perlbrew\n\n-- \nFelipe Contreras\n"},{"id":"474866","messageId":"ZC2wppC62E7wOcqM@xps","threadId":"59549","inReplyTo":"20230405165414.GA497301@coredump.intra.peff.net","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-05T17:32:22Z","receivedAt":"2023-04-05T17:32:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 05, 2023 at 12:54:14PM -0400, Jeff King wrote:\n> On Wed, Apr 05, 2023 at 04:52:40PM +0200, Patrick Steinhardt wrote:\n> \n> > > Is there a reason to not set PERL_PATH, which is the\n> > > documented method to handle this? From the Makefike:\n> > > \n> > > # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n> > \n> > Setting PERL_PATH helps with a subset of invocations where the Makefile\n> > either executes Perl directly or where it writes the shebang itself. But\n> > the majority of scripts I'm touching have `#!/usr/bin/perl` as shebang,\n> > and that path is not adjusted by setting PERL_PATH.\n> \n> Which scripts? If I do:\n> \n> mkdir /tmp/foo\n> ln -s /usr/bin/perl /tmp/foo/my-perl\n> make PERL_PATH=/tmp/foo/my-perl prefix=/tmp/foo install\n> \n> head -n 1 /tmp/foo/bin/git-cvsserver\n> \n> Then I see:\n> \n> #!/tmp/foo/my-perl\n> \n> And that is due to this segment in the Makefile:\n> \n> $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE\n> $(QUIET_GEN) \\\n> sed -e '1{' \\\n> -e ' s|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n> -e ' r GIT-PERL-HEADER' \\\n> -e ' G' \\\n> -e '}' \\\n> -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n> $< >$@+ && \\\n> chmod +x $@+ && \\\n> mv $@+ $@\n> \n> And that behavior goes all the way back to bc6146d2abc ('build' scripts\n> before installing., 2005-09-08). If there are some perl scripts we are\n> \"building\" outside of this rule, then that is probably a bug.\n> \n> The only thing I found via:\n> \n> find /tmp/foo -type | xargs grep /usr/bin/perl\n> \n> was a sample hook (which is probably a bug; we do munge the hook scripts\n> to replace @PERL_PATH@, etc, but I think the Makefile never learned that\n> the template hook scripts might be something other than shell scripts).\n\nYeah, agreed, the scripts we install are fine from all I can tell. I\nshould've clarified, but what I care about is our build infra as well as\nour test scripts. That's neither clear from the commit description nor\nfrom the changes that I'm doing.\n\nI'd be happy to keep the current state of installed scripts as-is and\nresend another iteration of this patch that only addresses shebangs used\nin internal scripts.\n\nPatrick\n"},{"id":"474867","messageId":"ZC2xcDwuhiEn2giX@xps","threadId":"59549","inReplyTo":"ZC2ZyTTZFbd_gNtw@pobox.com","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-05T17:35:44Z","receivedAt":"2023-04-05T17:35:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 05, 2023 at 11:54:49AM -0400, Todd Zullinger wrote:\n> Patrick Steinhardt wrote:\n> > On Wed, Apr 05, 2023 at 10:42:52AM -0400, Todd Zullinger wrote:\n> >> Is there a reason to not set PERL_PATH, which is the\n> >> documented method to handle this? From the Makefike:\n> >> \n> >> # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n> > \n> > Setting PERL_PATH helps with a subset of invocations where the Makefile\n> > either executes Perl directly or where it writes the shebang itself. But\n> > the majority of scripts I'm touching have `#!/usr/bin/perl` as shebang,\n> > and that path is not adjusted by setting PERL_PATH.\n> \n> Ahh. I wonder if that's intentional? I haven't dug into\n> the history, so I'm not sure. It seems like an oversight,\n> as an initial reaction.\n> \n> > I'd be happy to amend the patch series to only fix up shebangs which\n> > would not be helped by setting PERL_PATH. But if we can make it work\n> > without having to set PERL_PATH at all I don't quite see the point.\n> \n> It's certainly debatable whether using /path/to/env perl is\n> better than hard-coding it at build time (forgetting about\n> the usage of RUNTIME_PREFIX). [Debatable in a friendly\n> sense, of course.]\n> \n> As a distribution packager, I prefer to set the path at\n> build time to help ensure that an end user can't easily\n> break things by installing a different perl in PATH.\n> \n> The Fedora build system will munge /path/to/env perl\n> shebangs to /usr/bin/perl and it won't effect us much.\n> \n> That may not be true for other distributions and they may\n> care more if they want to keep using a hard-coded path to\n> perl. I don't know how it may affects the Windows folks\n> either, who are further askew from our other, more UNIX-like\n> targets.\n> \n> I don't know what the right choice is for upstream Git, it\n> can easily be argued in either direction. :)\n\nI agree, there is no clearly-superior choice -- both have their merits.\nI'll probably send a v2 that only munges internal scripts that are used\nas part of our build and testing infrastructure. That's the area I care\nmost about in this context anyway.\n\nPatrick\n"},{"id":"474871","messageId":"20230405181505.GA517608@coredump.intra.peff.net","threadId":"59549","inReplyTo":"ZC2wppC62E7wOcqM@xps","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-05T18:15:05Z","receivedAt":"2023-04-05T18:15:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 05, 2023 at 07:32:22PM +0200, Patrick Steinhardt wrote:\n\n> Yeah, agreed, the scripts we install are fine from all I can tell. I\n> should've clarified, but what I care about is our build infra as well as\n> our test scripts. That's neither clear from the commit description nor\n> from the changes that I'm doing.\n\nAh, OK, that makes more sense.\n\n> I'd be happy to keep the current state of installed scripts as-is and\n> resend another iteration of this patch that only addresses shebangs used\n> in internal scripts.\n\nWe generally try to use $PERL_PATH even for building and testing by\ninvoking \"$PERL_PATH script.pl\", and declaring a perl() wrapper within\nthe test scripts. But I would not be surprised if there are cases where\nwe fail to (and nobody noticed because it usually just works to find one\nat /usr/bin/perl).\n\nIMHO we should aim for fixing those inconsistencies, and then letting\npeople set PERL_PATH as appropriate (even to something that will find it\nvia $PATH if they want to).\n\n-Peff\n"},{"id":"474872","messageId":"733d4a0a-d544-4e25-80af-45d9551ab034@app.fastmail.com","threadId":"59549","inReplyTo":"d9cfad7caf9ff5bf88eb06cf7bb3be5e70e6d96f.1680689378.git.ps@pks.im","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-04-05T18:28:36Z","receivedAt":"2023-04-05T18:29:22Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Apr 5, 2023, at 12:10, Patrick Steinhardt wrote:\n> The majority of Perl scripts we carry in Git have a `#!/usr/bin/perl`\n> shebang. This is not a portable location for the Perl interpreter and\n> may thus break on some systems that have the interpreter installed in a\n> different location. One such example is NixOS, where the only executable\n> installed in `/usr/bin` is env(1).\n>\n> Convert the shebangs to resolve the location of the Perl interpreter via\n> env(1) to make these scripts more portable. While the location of env(1)\n> is not guaranteed by any standard either, in practice all distributions\n> including NixOS have it available at `/usr/bin/env`. We're also already\n> using this idiom in a small set of other scripts, and until now nobody\n> complained about them.\n\nThis is great. I love to see efforts towards portability, and\nparticularly when it benefits NixOS (as well).\n\n-- \nKristoffer Haugsbakk\n"},{"id":"474874","messageId":"xmqqv8iaw4n5.fsf@gitster.g","threadId":"59549","inReplyTo":"ZC2xcDwuhiEn2giX@xps","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-05T18:44:14Z","receivedAt":"2023-04-05T18:44:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> I don't know what the right choice is for upstream Git, it\n>> can easily be argued in either direction. :)\n>\n> I agree, there is no clearly-superior choice -- both have their merits.\n> I'll probably send a v2 that only munges internal scripts that are used\n> as part of our build and testing infrastructure. That's the area I care\n> most about in this context anyway.\n\nMy preference is \n\n (1) not to touch scripts that are processed by Makefile to use\n $PERL_PATH,\n\n (2) fix callers of \"./foo.pl\" to invoke \"$PERL_PATH ./foo.pl\" where\n the perl () { command \"$PERL_PATH\" \"$@\" } wrapper is not\n avialable, and\n\n (3) fix them to use \"perl foo.pl\" where the wrapper is visible.\n\nThat way, we can wean ourselves away from the assumption that perl\ninterpreter should exist at /usr/bin/perl without introducing a new\nassumption that everybody's env should exist at /usr/bin/env. There\nmay already be scripts that assume \"#!/usr/bin/env foo\" is\nacceptable, but fixing them would be outside the scope of this\ndiscussion, I would say.\n\nThanks all for a good discussion.\n\n\n\n\n\n\n"},{"id":"474890","messageId":"20230405213020.M231170@dcvr","threadId":"59549","inReplyTo":"d9cfad7caf9ff5bf88eb06cf7bb3be5e70e6d96f.1680689378.git.ps@pks.im","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-04-05T21:30:20Z","receivedAt":"2023-04-05T21:30:23Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Patrick Steinhardt wrote:\n> diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl\n> index 755a110bc4..3fe43b8968 100755\n> --- a/Documentation/cmd-list.perl\n> +++ b/Documentation/cmd-list.perl\n> @@ -1,5 +1,6 @@\n> -#!/usr/bin/perl -w\n> +#!/usr/bin/env perl\n> \n> +use warnings;\n\nFwiw, adding `use warnings' only affects the current scope\n(package main), whereas `-w' affects the entire Perl process.\n\nI prefer `-w' since adding `use warnings' everywhere is\nannoyingly verbose and I only use 3rd-party code that's\nwarning-clean.\n\nIn *.t test scripts and stuff I intend to be overwritten in\ninstall scripts; I've been using `#!perl -w' as the shebang\nas a clear signal that it should be overwritten on install\nor or run via `$(PERL) FOO' in a Makefile.\n\nFor personal scripts in ~/bin, I've been going shebang-less\nand having the following as the first two lines:\n\n\teval 'exec perl -w -S $0 ${1+\"$@\"}'\n\tif 0; # running under some shell\n\n(Only tested GNU/Linux and FreeBSD, though). This (and\n`env perl') will fail if distros someday decide to start\nusing `perl5' as the executable name.\n\n\nThat said, I don't know if anything I've said above is\nappropriate for the git project aside from noting the\ndifference between `-w' and `use warnings'.\n"},{"id":"474898","messageId":"CAMP44s1WDh7wbYW1+xdW5Rds7dnDtz7-aGy17pSynxVmAYo4dw@mail.gmail.com","threadId":"59549","inReplyTo":"20230405213020.M231170@dcvr","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-06T02:16:18Z","receivedAt":"2023-04-06T02:16:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Apr 5, 2023 at 4:50 PM Eric Wong wrote:\n\n> (Only tested GNU/Linux and FreeBSD, though). This (and\n> `env perl') will fail if distros someday decide to start\n> using `perl5' as the executable name.\n\nWhen that happens you can just do `ln -s perl ~/bin/perl5`.\n\nThis is similar to what I did with the horrendous python3 transition.\n\n-- \nFelipe Contreras\n"},{"id":"474899","messageId":"CAMP44s2_b0=Bm-NmDQ7ZVBen27ZtK9DpaF0gs965k1wXzzhARQ@mail.gmail.com","threadId":"59549","inReplyTo":"20230405181505.GA517608@coredump.intra.peff.net","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-06T02:18:20Z","receivedAt":"2023-04-06T02:18:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Apr 5, 2023 at 2:09 PM Jeff King wrote:\n> On Wed, Apr 05, 2023 at 07:32:22PM +0200, Patrick Steinhardt wrote:\n\n> IMHO we should aim for fixing those inconsistencies, and then letting\n> people set PERL_PATH as appropriate (even to something that will find it\n> via $PATH if they want to).\n\nWe can aim to fix all those inconsistencies *eventually* while in the\nmeantime make them runnable for most people *today*.\n\nIt's not a dichotomy.\n\n-- \nFelipe Contreras\n"},{"id":"474900","messageId":"CAMP44s2VuvP_mhLSXXyC1__GGYhfQM4Td-bghOE3AmqfQW0Czw@mail.gmail.com","threadId":"59549","inReplyTo":"xmqqv8iaw4n5.fsf@gitster.g","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-06T02:27:48Z","receivedAt":"2023-04-06T02:28:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Apr 5, 2023 at 2:29 PM Junio C Hamano wrote:\n>\n> Patrick Steinhardt writes:\n>\n> >> I don't know what the right choice is for upstream Git, it\n> >> can easily be argued in either direction. :)\n> >\n> > I agree, there is no clearly-superior choice -- both have their merits.\n> > I'll probably send a v2 that only munges internal scripts that are used\n> > as part of our build and testing infrastructure. That's the area I care\n> > most about in this context anyway.\n>\n> My preference is\n>\n> (1) not to touch scripts that are processed by Makefile to use\n> $PERL_PATH,\n>\n> (2) fix callers of \"./foo.pl\" to invoke \"$PERL_PATH ./foo.pl\" where\n> the perl () { command \"$PERL_PATH\" \"$@\" } wrapper is not\n> avialable, and\n>\n> (3) fix them to use \"perl foo.pl\" where the wrapper is visible.\n\nThat is orthogonal to the patch.\n\nAll those steps can be done *eventually* while the proposed patch is\napplied *today*.\n\n> That way, we can wean ourselves away from the assumption that perl\n> interpreter should exist at /usr/bin/perl without introducing a new\n> assumption that everybody's env should exist at /usr/bin/env.\n\nThe patch doesn't introduce such an assumption.\n\nChanging the shebang only affects scripts that are 1) not processed by\nthe Makefile, and 2) not called as \"${PERL_PATH-perl} foo.pl\".\n\nIf your system does not have /usr/bin/env and everything you cared\nabout worked yesterday, it would still work with the patch applied\ntoday.\n\nHaving a `#!/usr/bin/env perl` shebang is simply a good practice to\nwrite in all scripts.\n\nBut that is just the *default*, nobody is being forced to actually use\nthat shebang because 1) the Makefile is still going to override that\nand replace it with $PERL_PATH in generated scripts, and 2) the\nscripts that do \"$PERL_PATH ./foo.pl\" are essentially overriding it\nto, and so does 3).\n\nI believe this is a red herring (which might be desirable to fix some day).\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"474902","messageId":"20230406032647.GA2092142@coredump.intra.peff.net","threadId":"59549","inReplyTo":"d9cfad7caf9ff5bf88eb06cf7bb3be5e70e6d96f.1680689378.git.ps@pks.im","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-06T03:26:47Z","receivedAt":"2023-04-06T03:26:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 05, 2023 at 12:10:10PM +0200, Patrick Steinhardt wrote:\n\n> The majority of Perl scripts we carry in Git have a `#!/usr/bin/perl`\n> shebang. This is not a portable location for the Perl interpreter and\n> may thus break on some systems that have the interpreter installed in a\n> different location. One such example is NixOS, where the only executable\n> installed in `/usr/bin` is env(1).\n> \n> Convert the shebangs to resolve the location of the Perl interpreter via\n> env(1) to make these scripts more portable. While the location of env(1)\n> is not guaranteed by any standard either, in practice all distributions\n> including NixOS have it available at `/usr/bin/env`. We're also already\n> using this idiom in a small set of other scripts, and until now nobody\n> complained about them.\n> \n> This makes the test suite pass on NixOS.\n\nCan you tell us more about which tests failed?\n\nSkimming over the list of files here, the first few examples:\n\n> Documentation/build-docdep.perl | 2 +-\n> Documentation/cat-texi.perl | 2 +-\n> Documentation/cmd-list.perl | 3 ++-\n> Documentation/fix-texi.perl | 4 +++-\n> Documentation/lint-fsck-msgids.perl | 2 +-\n\nwill not be affected by your patch, because we never use their shebang\nlines at all (we say \"$PERL_PATH cmd-list.perl\" in the Makefile).\n\nI did try removing /usr/bin/perl completely (and thus likewise had no\nperl in my path), setting PERL_PATH, and got a few broken tests, which\ncould be fixed as below.\n\nDoes this fix the cases you saw, or are there others?\n\n-- >8 --\nSubject: [PATCH] t/lib-httpd: pass PERL_PATH to CGI scripts\n\nAs discussed in t/README, tests should aim to use PERL_PATH rather than\nstraight \"perl\". We usually do this automatically with a \"perl\" function\nin test-lib.sh, but a few cases need to be handled specially.\n\nOne such case is the apply-one-time-perl.sh CGI, which invokes plain\n\"perl\". It should be using $PERL_PATH, but to make that work, we must\nalso instruct Apache to pass through the variable.\n\nPrior to this patch, doing:\n\n mv /usr/bin/perl /usr/bin/my-perl\n make PERL_PATH=/usr/bin/my-perl test\n\nwould fail t5702, t5703, and t5616. After this it passes. This is a\npretty extreme case, as even if you install perl elsewhere, you'd likely\nstill have it in your $PATH. A more realistic case is that you don't\nwant to use the perl in your $PATH (because it's older, broken, etc) and\nexpect PERL_PATH to consistently override that (since that's what it's\ndocumented to do). Removing it completely is just a convenient way of\ncompletely breaking it for testing purposes.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd/apache.conf | 2 ++\n t/lib-httpd/apply-one-time-perl.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex f43a25c1f10..9e6892970de 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -101,6 +101,8 @@ PassEnv LC_ALL\n Alias /dumb/ www/\n Alias /auth/dumb/ www/auth/dumb/\n \n+SetEnv PERL_PATH ${PERL_PATH}\n+\n \n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\ndiff --git a/t/lib-httpd/apply-one-time-perl.sh b/t/lib-httpd/apply-one-time-perl.sh\nindex 09a0abdff7c..d7f9fed6aee 100644\n--- a/t/lib-httpd/apply-one-time-perl.sh\n+++ b/t/lib-httpd/apply-one-time-perl.sh\n@@ -13,7 +13,7 @@ then\n \texport LC_ALL\n \n \t\"$GIT_EXEC_PATH/git-http-backend\" >out\n-\tperl -pe \"$(cat one-time-perl)\" out >out_modified\n+\t\"$PERL_PATH\" -pe \"$(cat one-time-perl)\" out >out_modified\n \n \tif cmp -s out out_modified\n \tthen\n-- \n2.40.0.824.g7b678b1f643\n\n"},{"id":"474904","messageId":"20230406033507.GA2092122@coredump.intra.peff.net","threadId":"59549","inReplyTo":"CAMP44s2_b0=Bm-NmDQ7ZVBen27ZtK9DpaF0gs965k1wXzzhARQ@mail.gmail.com","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-06T03:35:07Z","receivedAt":"2023-04-06T03:35:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 05, 2023 at 09:18:20PM -0500, Felipe Contreras wrote:\n\n> On Wed, Apr 5, 2023 at 2:09 PM Jeff King wrote:\n> > On Wed, Apr 05, 2023 at 07:32:22PM +0200, Patrick Steinhardt wrote:\n> \n> > IMHO we should aim for fixing those inconsistencies, and then letting\n> > people set PERL_PATH as appropriate (even to something that will find it\n> > via $PATH if they want to).\n> \n> We can aim to fix all those inconsistencies *eventually* while in the\n> meantime make them runnable for most people *today*.\n> \n> It's not a dichotomy.\n\nIt is if the proposed patches change the behavior in such a way as to\nmake things less consistent.\n\nThere are three plausible perls to run (whether intentionally or\naccidentally):\n\n 1. the one in PERL_PATH\n\n 2. /usr/bin/perl\n\n 3. the first one in $PATH\n\nWhat the code tries to do now is to consistently use (1). If there are\ncases that accidentally use (2), which is what I took Patrick's patch to\nmean, then that is a problem for people who set PERL_PATH to something\nelse, but not for people who leave it as /usr/bin/perl. If we \"fix\"\nthose cases by switching them to (3), then now things are less\nconsistent for such people than when we started.\n\nBut I am not clear on what those cases are (if any), and we have not\nseen Patrick's follow-up proposed patch.\n\nI did find one case that is accidentally doing (3), and posted a patch\nelsewhere in the thread to convert it to (1). If you prefer behavior\n(3), you might consider that a regression, but it seems meaningless\ngiven the 99% of other cases that are using (1). If you want (3) to be\nthe behavior everywhere, then we'd need to completely change our stance\non how we invoke perl, or we need to teach PERL_PATH to handle this case\nso that people building Git can choose their own preference (sadly I\ndon't think \"make PERL_PATH='/usr/bin/env perl'\" quite works because we\nhave to shell-quote it in some contexts before evaluating).\n\n-Peff\n"},{"id":"474912","messageId":"ZC59sbedolRAWF9k@ncase","threadId":"59549","inReplyTo":"20230405181505.GA517608@coredump.intra.peff.net","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-06T08:07:13Z","receivedAt":"2023-04-06T08:07:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 05, 2023 at 02:15:05PM -0400, Jeff King wrote:\n> On Wed, Apr 05, 2023 at 07:32:22PM +0200, Patrick Steinhardt wrote:\n> \n> > Yeah, agreed, the scripts we install are fine from all I can tell. I\n> > should've clarified, but what I care about is our build infra as well as\n> > our test scripts. That's neither clear from the commit description nor\n> > from the changes that I'm doing.\n> \n> Ah, OK, that makes more sense.\n> \n> > I'd be happy to keep the current state of installed scripts as-is and\n> > resend another iteration of this patch that only addresses shebangs used\n> > in internal scripts.\n> \n> We generally try to use $PERL_PATH even for building and testing by\n> invoking \"$PERL_PATH script.pl\", and declaring a perl() wrapper within\n> the test scripts. But I would not be surprised if there are cases where\n> we fail to (and nobody noticed because it usually just works to find one\n> at /usr/bin/perl).\n> \n> IMHO we should aim for fixing those inconsistencies, and then letting\n> people set PERL_PATH as appropriate (even to something that will find it\n> via $PATH if they want to).\n> \n> -Peff\n\nMakes sense to me, I'll send a v2 that goes into this direction. Thanks\nall for your input!\n\nPatrick\n"},{"id":"474913","messageId":"230406.86355dv363.gmgdl@evledraar.gmail.com","threadId":"59549","inReplyTo":"20230405213020.M231170@dcvr","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T08:05:46Z","receivedAt":"2023-04-06T08:13:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 05 2023, Eric Wong wrote:\n\n> Patrick Steinhardt wrote:\n>> diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl\n>> index 755a110bc4..3fe43b8968 100755\n>> --- a/Documentation/cmd-list.perl\n>> +++ b/Documentation/cmd-list.perl\n>> @@ -1,5 +1,6 @@\n>> -#!/usr/bin/perl -w\n>> +#!/usr/bin/env perl\n>> \n>> +use warnings;\n>\n> Fwiw, adding `use warnings' only affects the current scope\n> (package main), whereas `-w' affects the entire Perl process.\n>\n> I prefer `-w' since adding `use warnings' everywhere is\n> annoyingly verbose and I only use 3rd-party code that's\n> warning-clean.\n\nI much prefer \"use warnings\", and it's what we should be using. Note\nthat \"-w\" isn't just \"global warnings\", but for anything non-trivial\nyou'll likely be tripped up by \"-w\" being dynamically scoped, and \"use\nwarnings\" being lexically scoped. See \"What's wrong with -w and $^W\" in\n\"perldoc warnings\".\n\nIn any case, I think it's fine to change these to \"use warnings\", and\nthe behavior won't change, as the scripts are trivial, and the either\ndon't use any modules, or use perl core modules that are (presumably)\nwarning-free.\n\nBut I think that change really should be split up from any proposed\nshebang change, as it's just being made here because by usin g\"env\" in\nparticular we can't pass the \"-w\" argument...\n"},{"id":"474915","messageId":"ZC6AdylF4TI41vnX@ncase","threadId":"59549","inReplyTo":"20230406032647.GA2092142@coredump.intra.peff.net","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-06T08:19:03Z","receivedAt":"2023-04-06T08:19:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 05, 2023 at 11:26:47PM -0400, Jeff King wrote:\n> On Wed, Apr 05, 2023 at 12:10:10PM +0200, Patrick Steinhardt wrote:\n> \n> > The majority of Perl scripts we carry in Git have a `#!/usr/bin/perl`\n> > shebang. This is not a portable location for the Perl interpreter and\n> > may thus break on some systems that have the interpreter installed in a\n> > different location. One such example is NixOS, where the only executable\n> > installed in `/usr/bin` is env(1).\n> > \n> > Convert the shebangs to resolve the location of the Perl interpreter via\n> > env(1) to make these scripts more portable. While the location of env(1)\n> > is not guaranteed by any standard either, in practice all distributions\n> > including NixOS have it available at `/usr/bin/env`. We're also already\n> > using this idiom in a small set of other scripts, and until now nobody\n> > complained about them.\n> > \n> > This makes the test suite pass on NixOS.\n> \n> Can you tell us more about which tests failed?\n> \n> Skimming over the list of files here, the first few examples:\n> \n> > Documentation/build-docdep.perl | 2 +-\n> > Documentation/cat-texi.perl | 2 +-\n> > Documentation/cmd-list.perl | 3 ++-\n> > Documentation/fix-texi.perl | 4 +++-\n> > Documentation/lint-fsck-msgids.perl | 2 +-\n> \n> will not be affected by your patch, because we never use their shebang\n> lines at all (we say \"$PERL_PATH cmd-list.perl\" in the Makefile).\n> \n> I did try removing /usr/bin/perl completely (and thus likewise had no\n> perl in my path), setting PERL_PATH, and got a few broken tests, which\n> could be fixed as below.\n> \n> Does this fix the cases you saw, or are there others?\n\nYou know, let's just go with your patch. With PERL_PATH set it fixes all\nthe issues I have observed. At some point in time I saw more issues than\nthe one you fix here, but that's because my `config.mak` got lost\nwithout me noticing. Oops, embarassing.\n\nThanks!\n\nPatrick\n\n> -- >8 --\n> Subject: [PATCH] t/lib-httpd: pass PERL_PATH to CGI scripts\n> \n> As discussed in t/README, tests should aim to use PERL_PATH rather than\n> straight \"perl\". We usually do this automatically with a \"perl\" function\n> in test-lib.sh, but a few cases need to be handled specially.\n> \n> One such case is the apply-one-time-perl.sh CGI, which invokes plain\n> \"perl\". It should be using $PERL_PATH, but to make that work, we must\n> also instruct Apache to pass through the variable.\n> \n> Prior to this patch, doing:\n> \n> mv /usr/bin/perl /usr/bin/my-perl\n> make PERL_PATH=/usr/bin/my-perl test\n> \n> would fail t5702, t5703, and t5616. After this it passes. This is a\n> pretty extreme case, as even if you install perl elsewhere, you'd likely\n> still have it in your $PATH. A more realistic case is that you don't\n> want to use the perl in your $PATH (because it's older, broken, etc) and\n> expect PERL_PATH to consistently override that (since that's what it's\n> documented to do). Removing it completely is just a convenient way of\n> completely breaking it for testing purposes.\n> \n> Signed-off-by: Jeff King \n> ---\n> t/lib-httpd/apache.conf | 2 ++\n> t/lib-httpd/apply-one-time-perl.sh | 2 +-\n> 2 files changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\n> index f43a25c1f10..9e6892970de 100644\n> --- a/t/lib-httpd/apache.conf\n> +++ b/t/lib-httpd/apache.conf\n> @@ -101,6 +101,8 @@ PassEnv LC_ALL\n> Alias /dumb/ www/\n> Alias /auth/dumb/ www/auth/dumb/\n> \n> +SetEnv PERL_PATH ${PERL_PATH}\n> +\n> \n> \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n> \tSetEnv GIT_HTTP_EXPORT_ALL\n> diff --git a/t/lib-httpd/apply-one-time-perl.sh b/t/lib-httpd/apply-one-time-perl.sh\n> index 09a0abdff7c..d7f9fed6aee 100644\n> --- a/t/lib-httpd/apply-one-time-perl.sh\n> +++ b/t/lib-httpd/apply-one-time-perl.sh\n> @@ -13,7 +13,7 @@ then\n> \texport LC_ALL\n> \n> \t\"$GIT_EXEC_PATH/git-http-backend\" >out\n> -\tperl -pe \"$(cat one-time-perl)\" out >out_modified\n> +\t\"$PERL_PATH\" -pe \"$(cat one-time-perl)\" out >out_modified\n> \n> \tif cmp -s out out_modified\n> \tthen\n> -- \n> 2.40.0.824.g7b678b1f643\n> \n"},{"id":"474916","messageId":"230406.86y1n5tnvk.gmgdl@evledraar.gmail.com","threadId":"59549","inReplyTo":"20230406033507.GA2092122@coredump.intra.peff.net","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T08:03:53Z","receivedAt":"2023-04-06T08:29:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 05 2023, Jeff King wrote:\n\n> On Wed, Apr 05, 2023 at 09:18:20PM -0500, Felipe Contreras wrote:\n>\n>> On Wed, Apr 5, 2023 at 2:09 PM Jeff King wrote:\n>> > On Wed, Apr 05, 2023 at 07:32:22PM +0200, Patrick Steinhardt wrote:\n>> \n>> > IMHO we should aim for fixing those inconsistencies, and then letting\n>> > people set PERL_PATH as appropriate (even to something that will find it\n>> > via $PATH if they want to).\n>> \n>> We can aim to fix all those inconsistencies *eventually* while in the\n>> meantime make them runnable for most people *today*.\n>> \n>> It's not a dichotomy.\n>\n> It is if the proposed patches change the behavior in such a way as to\n> make things less consistent.\n>\n> There are three plausible perls to run (whether intentionally or\n> accidentally):\n>\n> 1. the one in PERL_PATH\n>\n> 2. /usr/bin/perl\n>\n> 3. the first one in $PATH\n>\n> What the code tries to do now is to consistently use (1). If there are\n> cases that accidentally use (2), which is what I took Patrick's patch to\n> mean, then that is a problem for people who set PERL_PATH to something\n> else, but not for people who leave it as /usr/bin/perl. If we \"fix\"\n> those cases by switching them to (3), then now things are less\n> consistent for such people than when we started.\n>\n> But I am not clear on what those cases are (if any), and we have not\n> seen Patrick's follow-up proposed patch.\n>\n> I did find one case that is accidentally doing (3), and posted a patch\n> elsewhere in the thread to convert it to (1). If you prefer behavior\n> (3), you might consider that a regression, but it seems meaningless\n> given the 99% of other cases that are using (1). If you want (3) to be\n> the behavior everywhere, then we'd need to completely change our stance\n> on how we invoke perl, or we need to teach PERL_PATH to handle this case\n> so that people building Git can choose their own preference (sadly I\n> don't think \"make PERL_PATH='/usr/bin/env perl'\" quite works because we\n> have to shell-quote it in some contexts before evaluating).\n\nI just want to chime in to say that I've read this whole\nthread-at-large, and I think what you're pointing out here is correct,\nand that we should keep hardcoding \"#!/usr/bin/perl\", and then just have\n\"PERL_PATH\" set.\n\nI.e. most of Patrick's original patch is unnecessary, as we either use\n\"$PERL_PATH\" in the Makefile already, or munge the shebang when we\ninstall.\n\nThen the only change we should need is the one you suggested in\n<20230406032647.GA2092142@coredump.intra.peff.net> in the side-thread.\n\nUsing \"env\" liket his is also incorrect. I might have a \"perl\" in my\n\"$PATH\" which I expect to use for e.g. by .bashrc, but I don't want that\nperl to take priority over \"$PERL_PATH\" for git when I run some test\nscript.\n\nI also wonder if something like this (untested) wouldn't be useful to\nprovide an earlier warning of this, instead of failing when we fail to\ninvoke the relevant scripts.\n\n\tdiff --git a/Makefile b/Makefile\n\tindex 60ab1a8b4f4..9abc2e52cfa 100644\n\t--- a/Makefile\n\t+++ b/Makefile\n\t@@ -910,6 +910,15 @@ ifndef PYTHON_PATH\n\t \tPYTHON_PATH = /usr/bin/python\n\t endif\n\t \n\t+define check-path-exists\n\t+ifeq ($$(wildcard $$($(1))),)\n\t+$$(error $(1) set to '$(2)', which does not exist)\n\t+endif\n\t+endef\n\t+\n\t+$(eval $(call check-path-exists,SHELL_PATH,$(SHELL_PATH)))\n\t+$(eval $(call check-path-exists,PERL_PATH,$(PERL_PATH)))\n\t+\n\t export PERL_PATH\n\t export PYTHON_PATH\n\nThat should not break anything in principle, as we already rely on these\nto build git itself, but note that that's not the case with\n\"PYTHON_PATH\".\n\nIn the case of \"PERL_PATH\" I think that's limited to building the docs,\nso if we did something like this perhaps it should be in shared.mak, and\nthat check should be in Documentation/Makefile.\n\nBut perhaps it's all reduntant to just running into an error when we try\nto generate-cmdlist.sh or whatever...\n"},{"id":"474925","messageId":"20230406093602.GD2215039@coredump.intra.peff.net","threadId":"59549","inReplyTo":"ZC6AdylF4TI41vnX@ncase","subject":"[PATCH] t/lib-httpd: pass PERL_PATH to CGI scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-06T09:36:02Z","receivedAt":"2023-04-06T09:36:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 06, 2023 at 10:19:03AM +0200, Patrick Steinhardt wrote:\n\n> > Does this fix the cases you saw, or are there others?\n> \n> You know, let's just go with your patch. With PERL_PATH set it fixes all\n> the issues I have observed. At some point in time I saw more issues than\n> the one you fix here, but that's because my `config.mak` got lost\n> without me noticing. Oops, embarassing.\n\nOK, that is good if there is nothing left to fix. :) Let us know if any\nof the others pop up again.\n\nI also built the docs, which seems to use PERL_PATH as appropriate,\nthough one thing that did trip me up is that:\n\n make PERL_PATH=/my/actual/perl\n cd Documentation\n make\n\ndoes not pick up the earlier PERL_PATH. Which is not surprising if you\nthink about it. It's just that we shove some knobs into\nGIT-BUILD-OPTIONS and then pick them out later (but only in shell\nscripts, not in the Makefile), which created a false expectation.\n\n> > Subject: [PATCH] t/lib-httpd: pass PERL_PATH to CGI scripts\n\nReposting verbatim with a Cc and a catchier subject to get the\nmaintainer's attention.\n\n-- >8 --\nSubject: t/lib-httpd: pass PERL_PATH to CGI scripts\n\nAs discussed in t/README, tests should aim to use PERL_PATH rather than\nstraight \"perl\". We usually do this automatically with a \"perl\" function\nin test-lib.sh, but a few cases need to be handled specially.\n\nOne such case is the apply-one-time-perl.sh CGI, which invokes plain\n\"perl\". It should be using $PERL_PATH, but to make that work, we must\nalso instruct Apache to pass through the variable.\n\nPrior to this patch, doing:\n\n mv /usr/bin/perl /usr/bin/my-perl\n make PERL_PATH=/usr/bin/my-perl test\n\nwould fail t5702, t5703, and t5616. After this it passes. This is a\npretty extreme case, as even if you install perl elsewhere, you'd likely\nstill have it in your $PATH. A more realistic case is that you don't\nwant to use the perl in your $PATH (because it's older, broken, etc) and\nexpect PERL_PATH to consistently override that (since that's what it's\ndocumented to do). Removing it completely is just a convenient way of\ncompletely breaking it for testing purposes.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd/apache.conf | 2 ++\n t/lib-httpd/apply-one-time-perl.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex f43a25c1f10..9e6892970de 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -101,6 +101,8 @@ PassEnv LC_ALL\n Alias /dumb/ www/\n Alias /auth/dumb/ www/auth/dumb/\n \n+SetEnv PERL_PATH ${PERL_PATH}\n+\n \n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\ndiff --git a/t/lib-httpd/apply-one-time-perl.sh b/t/lib-httpd/apply-one-time-perl.sh\nindex 09a0abdff7c..d7f9fed6aee 100644\n--- a/t/lib-httpd/apply-one-time-perl.sh\n+++ b/t/lib-httpd/apply-one-time-perl.sh\n@@ -13,7 +13,7 @@ then\n \texport LC_ALL\n \n \t\"$GIT_EXEC_PATH/git-http-backend\" >out\n-\tperl -pe \"$(cat one-time-perl)\" out >out_modified\n+\t\"$PERL_PATH\" -pe \"$(cat one-time-perl)\" out >out_modified\n \n \tif cmp -s out out_modified\n \tthen\n-- \n2.40.0.824.g7b678b1f643\n\n"},{"id":"474939","messageId":"xmqq1qkxq8ab.fsf@gitster.g","threadId":"59549","inReplyTo":"20230406093602.GD2215039@coredump.intra.peff.net","subject":"Re: [PATCH] t/lib-httpd: pass PERL_PATH to CGI scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-06T16:34:20Z","receivedAt":"2023-04-06T16:34:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Subject: t/lib-httpd: pass PERL_PATH to CGI scripts\n>\n> As discussed in t/README, tests should aim to use PERL_PATH rather than\n> straight \"perl\". We usually do this automatically with a \"perl\" function\n> in test-lib.sh, but a few cases need to be handled specially.\n\nThanks, all, for a discussion that led to this simple solution that\nallows us to consistently use what the builder specifies as PERL_PATH,\nwhich we document as the way we aim to invoke \"perl\".\n\nWill queue.\n\n"},{"id":"475598","messageId":"643e5be864894_21c9542949d@chronos.notmuch","threadId":"59549","inReplyTo":"230406.86y1n5tnvk.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] global: resolve Perl executable via PATH","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-18T08:59:20Z","receivedAt":"2023-04-18T08:59:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Apr 05 2023, Jeff King wrote:\n> \n> > On Wed, Apr 05, 2023 at 09:18:20PM -0500, Felipe Contreras wrote:\n> >\n> >> On Wed, Apr 5, 2023 at 2:09 PM Jeff King wrote:\n> >> > On Wed, Apr 05, 2023 at 07:32:22PM +0200, Patrick Steinhardt wrote:\n> >> \n> >> > IMHO we should aim for fixing those inconsistencies, and then letting\n> >> > people set PERL_PATH as appropriate (even to something that will find it\n> >> > via $PATH if they want to).\n> >> \n> >> We can aim to fix all those inconsistencies *eventually* while in the\n> >> meantime make them runnable for most people *today*.\n> >> \n> >> It's not a dichotomy.\n> >\n> > It is if the proposed patches change the behavior in such a way as to\n> > make things less consistent.\n> >\n> > There are three plausible perls to run (whether intentionally or\n> > accidentally):\n> >\n> > 1. the one in PERL_PATH\n> >\n> > 2. /usr/bin/perl\n> >\n> > 3. the first one in $PATH\n> >\n> > What the code tries to do now is to consistently use (1). If there are\n> > cases that accidentally use (2), which is what I took Patrick's patch to\n> > mean, then that is a problem for people who set PERL_PATH to something\n> > else, but not for people who leave it as /usr/bin/perl. If we \"fix\"\n> > those cases by switching them to (3), then now things are less\n> > consistent for such people than when we started.\n> >\n> > But I am not clear on what those cases are (if any), and we have not\n> > seen Patrick's follow-up proposed patch.\n> >\n> > I did find one case that is accidentally doing (3), and posted a patch\n> > elsewhere in the thread to convert it to (1). If you prefer behavior\n> > (3), you might consider that a regression, but it seems meaningless\n> > given the 99% of other cases that are using (1). If you want (3) to be\n> > the behavior everywhere, then we'd need to completely change our stance\n> > on how we invoke perl, or we need to teach PERL_PATH to handle this case\n> > so that people building Git can choose their own preference (sadly I\n> > don't think \"make PERL_PATH='/usr/bin/env perl'\" quite works because we\n> > have to shell-quote it in some contexts before evaluating).\n> \n> I just want to chime in to say that I've read this whole\n> thread-at-large, and I think what you're pointing out here is correct,\n> and that we should keep hardcoding \"#!/usr/bin/perl\", and then just have\n> \"PERL_PATH\" set.\n> \n> I.e. most of Patrick's original patch is unnecessary, as we either use\n> \"$PERL_PATH\" in the Makefile already, or munge the shebang when we\n> install.\n> \n> Then the only change we should need is the one you suggested in\n> <20230406032647.GA2092142@coredump.intra.peff.net> in the side-thread.\n\nThat is true, however, the fact that something isn't *necessary* doesn't mean\nit isn't good.\n\n> Using \"env\" liket his is also incorrect.\n\nNo, it's not.\n\n> I might have a \"perl\" in my \"$PATH\" which I expect to use for e.g. by\n> .bashrc, but I don't want that perl to take priority over \"$PERL_PATH\" for\n> git when I run some test script.\n\nAnd you can keep doing that as Patrick's patch does not change that priority.\n\n\nI applied both Patrick's patch and Jeff's patch, and then did:\n\n ln -s /bin/false ~/bin/perl\n\nGuess what... Everything works fine (as it should). $PEARL_PATH should override\nthe shebangs (and it does), so it does not matter what `/usr/bin/env perl`\nreturns... unless somebody does run the offending scripts manually, in which\ncase they are on their own.\n\nSo, I think Patrick's patch should still be applied, but in practice makes no\ndifference, because Jeff's patch fixes the actual problems.\n\nOr another way to put it: Patrick's patch is unnecessary, but in my opinion\nstill desirable.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"475600","messageId":"643e5d1a3db93_21c95429444@chronos.notmuch","threadId":"59549","inReplyTo":"20230406093602.GD2215039@coredump.intra.peff.net","subject":"Re: [PATCH] t/lib-httpd: pass PERL_PATH to CGI scripts","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-18T09:04:26Z","receivedAt":"2023-04-18T09:05:56Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Thu, Apr 06, 2023 at 10:19:03AM +0200, Patrick Steinhardt wrote:\n> \n> > > Does this fix the cases you saw, or are there others?\n> > \n> > You know, let's just go with your patch. With PERL_PATH set it fixes all\n> > the issues I have observed. At some point in time I saw more issues than\n> > the one you fix here, but that's because my `config.mak` got lost\n> > without me noticing. Oops, embarassing.\n> \n> OK, that is good if there is nothing left to fix. :) Let us know if any\n> of the others pop up again.\n\nFor the record: I applied both patches and I can attest that Jeff's patch fixes\nall the problems, rendering Patrick's patch unnecessary.\n\nHowever, in my opinion Patrick's patch is still desirable and should be\napplied, even though after Jeff's patch it doesn't fix anything.\n\nIt's just a good practice.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}