threads / discuss / 4740

Making perl scripts include the correct Git.pm

Subject: Making perl scripts include the correct Git.pm

## tl;dr

6 messages between Jul 2, 2006 and Jul 4, 2006.

replies: 5people: 3as markdown or json

Petr Baudis· Jul 2, 2006, 21:40 UTC · lore
  Hi,
  the discussion of the topic became so scatterred that it's rather
difficult to follow now and I get a feeling that we are kind of running
in circles now, so this is my attempt to summarize it:
  Desired behaviour when running Git's perl scripts (ordered by
degree of necessity):
  (D1) When running installed script, it should include Git.pm from the
same installation.
  (D2) When running the testsuite, it should include Git.pm from the
source tree.
  (D3) When running script directly from source tree, it should include
Git.pm from the source tree.
  (i) The original solution passed -I on the #!/usr/bin/perl line, but
that was ugly, was prone to hit various OS limits on the shebang line
and violated both (D2) and (D3).
  (ii) My proposed second solution was to add an autogenerated line to
the Git's perl scripts saying something like:
	use lib ('instlibdir', 'srclibdir');

This fulfills all (D1), (D2) and (D3), but is perceived by Junio as "disgusting".

  (iii) The currently used solution is to effectively
	use lib ('instlibdir');

to the Git's perl scripts. This violated (D3) and (D2) too, since use lib is the last from all the @INC modifiers to be seen and thus overrides and $PERL5LIB set in the testsuite.

  (iv) Variation of (iii), probably Junio's original intention when
implementing it:
	push @INC, 'instlibdir';

This fulfills (D2). It does not fulfill (D3) per se since the user has to set $PERL5LIB manually when running Git without installing it, but it is at least fulfillable. However, most importantly this does not even fulfill (D1) since if you e.g. consider user-local installation of Git over system-wide installation of Git, local perl scripts will use the globally installed Git.pm.

  (v) If you throw away user-friendly (D3) requirement and insist
on (iii) being disgusting, this is a newly proposed possible variation
"(iv) meets (i)":
	#!/usr/bin/perl -Imarker
	@INC = map { $_ eq 'marker' ? 'instlibdir' : $_ } @INC;
(I think this is more disgusting than (iii), but tastes differ. ;)
  So, what's the way out?
  PS: Is this the only remaining problem with Git.pm or do we have
anything else to cope with, esp. before it gets considered to be a
next material?
-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam
Junio C Hamano· Jul 3, 2006, 00:02 UTC · re: Petr Baudis · lore

Re: Making perl scripts include the correct Git.pm

Petr Baudis <pasky@suse.cz> writes:
> so this is my attempt to summarize it:

Ah, our message crossed -- thanks for summarizing it. I do not particularly like any of the solution so far, but maybe the patch I just sent out to "do the normal thing unless we are running tests" might be the right thing to do.

Show 6 quoted lines
>   (ii) My proposed second solution was to add an autogenerated line to
> the Git's perl scripts saying something like:
>
> 	use lib ('instlibdir', 'srclibdir');
>
> This fulfills all (D1), (D2) and (D3)
If you have srclibdir after instlibdir, aren't you breaking D2?

And I do not think swapping them is right either. It would fullfil D1/D2/D3 but I think it has one bad side effect.

Namely, I do not want an installed script, maybe coming from the vendor, to be affected by whatever unrelated garbage the end user happens to have on the same path as the one used for building the module on potentially remote machine, so I would like to avoid including srclibdir anywhere on the include path for normal execution.

That is, if I built a binary package in /home/junio/git/git.git on my machine to be installed in /usr/lib/perl/somewhere, the users on a multi-user system can be tricked to run random stuff that happens to be in /home/junio/git/git.git/ directory on their machine.

Junio C Hamano· Jul 3, 2006, 01:38 UTC · re: Junio C Hamano · lore

Re: Making perl scripts include the correct Git.pm

Junio C Hamano <junkio@cox.net> writes:
Show 8 quoted lines
> Petr Baudis <pasky@suse.cz> writes:
>
>> so this is my attempt to summarize it:
>
> Ah, our message crossed -- thanks for summarizing it.  I do not
> particularly like any of the solution so far, but maybe the
> patch I just sent out to "do the normal thing unless we are
> running tests" might be the right thing to do.

I see you caught Merlyn so I decided the patch I sent earlier as a tentative fix and merged the series in "next". We should be able to fix Perly problems in-tree while we have his attention ;-).

Rene's merge-bases fix is also in "next". Hopefully this would make merge-recursive work by Alex and Johannes go smoother?

On the other front, I applied the --remove-empty --parents fix by Linus to "master". Since this broke gitk and qgit it might warrant a 1.4.1.1 hotfix.

Marco Costalba· Jul 4, 2006, 19:01 UTC · re: Junio C Hamano · lore

Re: Making perl scripts include the correct Git.pm

On 7/3/06, Junio C Hamano <junkio@cox.net> wrote:
Show 6 quoted lines
> Junio C Hamano <junkio@cox.net> writes:
>
> On the other front, I applied the --remove-empty --parents fix
> by Linus to "master".  Since this broke gitk and qgit it might
> warrant a 1.4.1.1 hotfix.
>

Well on the qgit side the bug is plus or less harmless, you see some spurious revisions in file annotation history and a warning message on stderr, but not a show stopper.

    Marco
Petr Baudis· Jul 3, 2006, 20:29 UTC · re: Junio C Hamano · lore

Re: Quick merge status updates.

Dear diary, on Mon, Jul 03, 2006 at 02:02:30AM CEST, I got a letter where Junio C Hamano <junkio@cox.net> said that...

Show 8 quoted lines
> Petr Baudis <pasky@suse.cz> writes:
> 
> > so this is my attempt to summarize it:
> 
> Ah, our message crossed -- thanks for summarizing it.  I do not
> particularly like any of the solution so far, but maybe the
> patch I just sent out to "do the normal thing unless we are
> running tests" might be the right thing to do.
Yes, I roughly agree. I have some gripes, patches will follow.
Show 8 quoted lines
> >   (ii) My proposed second solution was to add an autogenerated line to
> > the Git's perl scripts saying something like:
> >
> > 	use lib ('instlibdir', 'srclibdir');
> >
> > This fulfills all (D1), (D2) and (D3)
> 
> If you have srclibdir after instlibdir, aren't you breaking D2?
*Sigh*
Why can't I get it right for once? ;-)
> And I do not think swapping them is right either.  It would
> fullfil D1/D2/D3 but I think it has one bad side effect.
Yes, I agree with your reservations.
-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam
Petr Baudis· Jul 3, 2006, 21:16 UTC · re: Petr Baudis · lore

[PATCH] Use $GITPERLLIB instead of $RUNNING_GIT_TESTS and centralize @INC munging

This makes the Git perl scripts check $GITPERLLIB instead of $RUNNING_GIT_TESTS, which makes more sense if you are setting up your shell environment to use a non-installed Git instance.

It also weeds out the @INC munging from the individual scripts and makes Makefile add it during the .perl files processing, so that we can change just a single place when we modify this shared logic. It looks ugly in the scripts, too. ;-)

And instead of doing arcane things with the @INC array, we just do 'use lib' instead, which is essentialy the same thing anyway.

I first want to do three separate patches but it turned out that it's quite a lot neater when bundled together, so I hope it's ok.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---
 INSTALL                |    4 ++--
 Makefile               |    4 +++-
 git-fmt-merge-msg.perl |    5 -----
 git-mv.perl            |    5 -----
 t/test-lib.sh          |    5 ++---
 5 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/INSTALL b/INSTALL
index ed502de..4e8f883 100644
--- a/INSTALL
+++ b/INSTALL
@@ -39,8 +39,8 @@ Issues of note:
 
 	GIT_EXEC_PATH=`pwd`
 	PATH=`pwd`:$PATH
-	PERL5LIB=`pwd`/perl/blib/lib:`pwd`/perl/blib/arch/auto/Git
-	export GIT_EXEC_PATH PATH PERL5LIB
+	GITPERLLIB=`pwd`/perl/blib/lib:`pwd`/perl/blib/arch/auto/Git
+	export GIT_EXEC_PATH PATH GITPERLLIB
 
  - Git is reasonably self-sufficient, but does depend on a few external
    programs and libraries:
diff --git a/Makefile b/Makefile
index a62e8a3..bae95c2 100644
--- a/Makefile
+++ b/Makefile
@@ -552,7 +552,9 @@ common-cmds.h: Documentation/git-*.txt
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	rm -f $@ $@+
 	INSTLIBDIR=`$(MAKE) -C perl -s --no-print-directory instlibdir` && \
-	sed -e '1s|#!.*perl\(.*\)|#!$(PERL_PATH_SQ)\1|' \
+	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|1' \
+	    -e '2i\
+	        use lib (split(/:/, $$ENV{GITPERLLIB} || '\'"$$INSTLIBDIR"\''));' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    $@.perl >$@+
diff --git a/git-fmt-merge-msg.perl b/git-fmt-merge-msg.perl
index a9805dd..f86231e 100755
--- a/git-fmt-merge-msg.perl
+++ b/git-fmt-merge-msg.perl
@@ -5,11 +5,6 @@ #
 # Read .git/FETCH_HEAD and make a human readable merge message
 # by grouping branches and tags together to form a single line.
 
-BEGIN {
-	unless (exists $ENV{'RUNNING_GIT_TESTS'}) {
-		unshift @INC, '@@INSTLIBDIR@@';
-	}
-}
 use strict;
 use Git;
 use Error qw(:try);
diff --git a/git-mv.perl b/git-mv.perl
index 5134b80..322b9fd 100755
--- a/git-mv.perl
+++ b/git-mv.perl
@@ -6,11 +6,6 @@ #
 # This file is licensed under the GPL v2, or a later version
 # at the discretion of Linus Torvalds.
 
-BEGIN {
-	unless (exists $ENV{'RUNNING_GIT_TESTS'}) {
-		unshift @INC, '@@INSTLIBDIR@@';
-	}
-}
 use warnings;
 use strict;
 use Getopt::Std;
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 298c6ca..ad9796e 100755
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -206,9 +206,8 @@ PYTHON=`sed -e '1{
 	PYTHONPATH=$(pwd)/../compat
 	export PYTHONPATH
 }
-RUNNING_GIT_TESTS=YesWeAre
-PERL5LIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git
-export PERL5LIB RUNNING_GIT_TESTS
+GITPERLLIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git
+export GITPERLLIB
 test -d ../templates/blt || {
 	error "You haven't built things yet, have you?"
 }

← back to recent threads