Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
Benoît Person benoit.per...@ensimag.fr writes:
Do we want to add that ':' unconditionally? Could GITPERLLIB be
empty?
For now, GITPERLLIB is only used in that kind of statements:
use lib (split(/:/, $ENV{GITPERLLIB} || ... ));
The trailing ':' does not really matter, split will ignore it.
That may be true with the current use, but For now leaves funny
taste, doesn't it?
It is not too much trouble to fix by writing
GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib'${GITPERLLIB:+:$GITPERLLIB}
and we will not have to be worried about future changes breaking
today's assumption.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
For now, GITPERLLIB is only used in that kind of statements:
use lib (split(/:/, $ENV{GITPERLLIB} || ... ));
The trailing ':' does not really matter, split will ignore it.
That may be true with the current use, but For now leaves funny
taste, doesn't it?
definitely. For me, the issue was that we were trading that funny
taste for less
readable code in wrap-for-bin.sh but with that default-substitution
construct, it seems
worth it. (a huge if-block seemed even funnier in fact :) )
It is not too much trouble to fix by writing
GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib'${GITPERLLIB:+:$GITPERLLIB}
and we will not have to be worried about future changes breaking
today's assumption.
That construct is really nice :) did not know there was such a thing
in bash, thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
On 07/03/2013 12:39 AM, benoit.per...@ensimag.fr wrote: From: Benoit Person benoit.per...@ensimag.fr For now, bin-wrappers (based on wrap-for-bin.sh) redefine some environnement variables (like $GITPERLLIB). If we want to chain to those scripts and define one of those variables before, our changes will be overwritten. This patch simply makes the bin-wrappers prepend their modifications rather than redefine the vars. Your commit message makes it sound like GITPERLLIB is only one example of a general class of environment variables fixed by the patch, but in fact the patch address *only* GITPERLLIB. Please make the commit message better match the patch. And maybe you could mention for posterity whether you have seen other variables that will need fixing vs. you did an audit and think that GITPERLLIB is the only problematic one vs. you haven't looked for other similar problems at all. This could save some time for the next person with a similar itch and/or maybe inspire somebody to do an audit. Thanks, Michael Signed-off-by: Benoit Person benoit.per...@ensimag.fr Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr --- wrap-for-bin.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh index 53a8dd0..dbebe49 100644 --- a/wrap-for-bin.sh +++ b/wrap-for-bin.sh @@ -14,7 +14,7 @@ else GIT_TEMPLATE_DIR='@@BUILD_DIR@@/templates/blt' export GIT_TEMPLATE_DIR fi -GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib' +GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib:'$GITPERLLIB GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale' PATH='@@BUILD_DIR@@/bin-wrappers:'$PATH export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
benoit.per...@ensimag.fr writes: diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh index 53a8dd0..dbebe49 100644 --- a/wrap-for-bin.sh +++ b/wrap-for-bin.sh @@ -14,7 +14,7 @@ else GIT_TEMPLATE_DIR='@@BUILD_DIR@@/templates/blt' export GIT_TEMPLATE_DIR fi -GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib' +GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib:'$GITPERLLIB Then you need to do something like this to prevent broken $GITPERLLIB in user's configuration from interfering with the testsuite: --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -92,6 +92,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $($PERL_PATH -e ' print join(\n, @vars); ') unset XDG_CONFIG_HOME +unset GITPERLLIB GIT_AUTHOR_EMAIL=aut...@example.com GIT_AUTHOR_NAME='A U Thor' GIT_COMMITTER_EMAIL=commit...@example.com -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
Matthieu Moy matthieu@grenoble-inp.fr writes: benoit.per...@ensimag.fr writes: diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh index 53a8dd0..dbebe49 100644 --- a/wrap-for-bin.sh +++ b/wrap-for-bin.sh @@ -14,7 +14,7 @@ else GIT_TEMPLATE_DIR='@@BUILD_DIR@@/templates/blt' export GIT_TEMPLATE_DIR fi -GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib' +GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib:'$GITPERLLIB Then you need to do something like this to prevent broken $GITPERLLIB in user's configuration from interfering with the testsuite: --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -92,6 +92,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $($PERL_PATH -e ' print join(\n, @vars); ') unset XDG_CONFIG_HOME +unset GITPERLLIB GIT_AUTHOR_EMAIL=aut...@example.com GIT_AUTHOR_NAME='A U Thor' GIT_COMMITTER_EMAIL=commit...@example.com Yes, that is a good point. It introduces a chicken-and-egg circularity for git-mw tests to use the common test infrastructure by dot-sourcing this file, though, no? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@grenoble-inp.fr writes: benoit.per...@ensimag.fr writes: diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh index 53a8dd0..dbebe49 100644 --- a/wrap-for-bin.sh +++ b/wrap-for-bin.sh @@ -14,7 +14,7 @@ else GIT_TEMPLATE_DIR='@@BUILD_DIR@@/templates/blt' export GIT_TEMPLATE_DIR fi -GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib' +GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib:'$GITPERLLIB Then you need to do something like this to prevent broken $GITPERLLIB in user's configuration from interfering with the testsuite: --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -92,6 +92,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $($PERL_PATH -e ' print join(\n, @vars); ') unset XDG_CONFIG_HOME +unset GITPERLLIB GIT_AUTHOR_EMAIL=aut...@example.com GIT_AUTHOR_NAME='A U Thor' GIT_COMMITTER_EMAIL=commit...@example.com Yes, that is a good point. It introduces a chicken-and-egg circularity for git-mw tests to use the common test infrastructure by dot-sourcing this file, though, no? I don't get it. It Git Mediawiki's tests, the tests scripts source test-lib.sh, that unsets GITPERLLIB. Then, it calls the mw-to-git/bin-wrapper/git that sets it properly, and calls the toplevel's bin-wrapper. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
Matthieu Moy matthieu@grenoble-inp.fr writes: I don't get it. It Git Mediawiki's tests, the tests scripts source test-lib.sh, that unsets GITPERLLIB. Then, it calls the mw-to-git/bin-wrapper/git that sets it properly, and calls the toplevel's bin-wrapper. Ah, OK. I somehow was assuming that a wrapper sets GITPERLLIB to point at mediawiki stuff before running tests, which is the other way around. Please ignore the noise, and thanks for a dose of sanity. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
benoit.per...@ensimag.fr writes: From: Benoit Person benoit.per...@ensimag.fr For now, bin-wrappers (based on wrap-for-bin.sh) redefine some environnement variables (like $GITPERLLIB). If we want to chain to those scripts and define one of those variables before, our changes will be overwritten. This patch simply makes the bin-wrappers prepend their modifications rather than redefine the vars. Signed-off-by: Benoit Person benoit.per...@ensimag.fr Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr --- wrap-for-bin.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh index 53a8dd0..dbebe49 100644 --- a/wrap-for-bin.sh +++ b/wrap-for-bin.sh @@ -14,7 +14,7 @@ else GIT_TEMPLATE_DIR='@@BUILD_DIR@@/templates/blt' export GIT_TEMPLATE_DIR fi -GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib' +GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib:'$GITPERLLIB Do we want to add that ':' unconditionally? Could GITPERLLIB be empty? GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale' PATH='@@BUILD_DIR@@/bin-wrappers:'$PATH export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/7] wrap-for-bin: Make bin-wrappers chainable
Do we want to add that ':' unconditionally? Could GITPERLLIB be
empty?
For now, GITPERLLIB is only used in that kind of statements:
use lib (split(/:/, $ENV{GITPERLLIB} || ... ));
The trailing ':' does not really matter, split will ignore it.
With the current codebase, I think it's nicer to do it that way (makes
wrap-for-bin.sh more readable).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
