From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: David Tran <unsignedzero@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls
Date: Fri, 21 Mar 2014 14:24:31 -0700 [thread overview]
Message-ID: <xmqqy503s0s0.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20140320231433.GC8479@sigill.intra.peff.net> (Jeff King's message of "Thu, 20 Mar 2014 19:14:33 -0400")
Jeff King <peff@peff.net> writes:
> Several test scripts manually unset GIT_CONFIG and other
> GIT_* variables. These are generally taken care of for us by
> test-lib.sh already.
>
> Unsetting these is not only useless, but can be confusing to
> a reader, who may wonder why some tests in a script unset
> them and others do not (t0001 is particularly guilty of this
> inconsistency, probably because many of its tests predate
> the test-lib.sh environment-cleansing).
> Note that we cannot always get rid of such unsetting. For
> example, t9130 can drop the GIT_CONFIG unset, but not the
> GIT_DIR one, because lib-git-svn.sh sets the latter. And in
> t1000, we unset GIT_TEMPLATE_DIR, which is explicitly
> initialized by test-lib.sh.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I suppose one could make an argument that test-lib.sh may later change
> the set of variables it clears, and these unsets are documenting an
> explicit need of each test. I'd find that more compelling if it were
> actually applied consistently.
Hmph. I am looking at "git show HEAD^:t/t0001-init.sh" after
applying this patch, and it does look consistently done with
GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a
cursory read it is done consistently for tests on non-bare
repositories).
So I would actually agree with your alternative interpretation
"Unsetting these is useless, but it does serve documentation
purpose---without having to see what the state of the environment
when the subprocess is started, the reader can understand what is
being tested", rather than the one in the log message.
Having said that, I am perfectly OK with the change to t0001 in this
patch, if we added at the very beginning of the test sequence a
comment that says:
Below, creation and use of repositories are tested with various
combinations of environment settings and command line flags.
They are done inside subshells to avoid leaking temporary
environment settings to later tests *and* assumes that the
initial environment does not have have GIT_DIR, GIT_CONFIG, and
GIT_WORK_TREE defined.
or something.
> t/t0001-init.sh | 15 ---------------
> t/t9130-git-svn-authors-file.sh | 1 -
> t/t9400-git-cvsserver-server.sh | 1 -
> 3 files changed, 17 deletions(-)
>
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 9fb582b..ddc8160 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -25,7 +25,6 @@ check_config () {
>
> test_expect_success 'plain' '
> (
> - sane_unset GIT_DIR GIT_WORK_TREE &&
> mkdir plain &&
> cd plain &&
> git init
> @@ -35,7 +34,6 @@ test_expect_success 'plain' '
>
> test_expect_success 'plain nested in bare' '
> (
> - sane_unset GIT_DIR GIT_WORK_TREE &&
> git init --bare bare-ancestor.git &&
> cd bare-ancestor.git &&
> mkdir plain-nested &&
> @@ -47,7 +45,6 @@ test_expect_success 'plain nested in bare' '
>
> test_expect_success 'plain through aliased command, outside any git repo' '
> (
> - sane_unset GIT_DIR GIT_WORK_TREE &&
> HOME=$(pwd)/alias-config &&
> export HOME &&
> mkdir alias-config &&
> @@ -65,7 +62,6 @@ test_expect_success 'plain through aliased command, outside any git repo' '
>
> test_expect_failure 'plain nested through aliased command' '
> (
> - sane_unset GIT_DIR GIT_WORK_TREE &&
> git init plain-ancestor-aliased &&
> cd plain-ancestor-aliased &&
> echo "[alias] aliasedinit = init" >>.git/config &&
> @@ -78,7 +74,6 @@ test_expect_failure 'plain nested through aliased command' '
>
> test_expect_failure 'plain nested in bare through aliased command' '
> (
> - sane_unset GIT_DIR GIT_WORK_TREE &&
> git init --bare bare-ancestor-aliased.git &&
> cd bare-ancestor-aliased.git &&
> echo "[alias] aliasedinit = init" >>config &&
> @@ -91,7 +86,6 @@ test_expect_failure 'plain nested in bare through aliased command' '
>
> test_expect_success 'plain with GIT_WORK_TREE' '
> if (
> - sane_unset GIT_DIR &&
> mkdir plain-wt &&
> cd plain-wt &&
> GIT_WORK_TREE=$(pwd) git init
> @@ -104,7 +98,6 @@ test_expect_success 'plain with GIT_WORK_TREE' '
>
> test_expect_success 'plain bare' '
> (
> - sane_unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&
> mkdir plain-bare-1 &&
> cd plain-bare-1 &&
> git --bare init
> @@ -114,7 +107,6 @@ test_expect_success 'plain bare' '
>
> test_expect_success 'plain bare with GIT_WORK_TREE' '
> if (
> - sane_unset GIT_DIR GIT_CONFIG &&
> mkdir plain-bare-2 &&
> cd plain-bare-2 &&
> GIT_WORK_TREE=$(pwd) git --bare init
> @@ -128,7 +120,6 @@ test_expect_success 'plain bare with GIT_WORK_TREE' '
> test_expect_success 'GIT_DIR bare' '
>
> (
> - sane_unset GIT_CONFIG &&
> mkdir git-dir-bare.git &&
> GIT_DIR=git-dir-bare.git git init
> ) &&
> @@ -138,7 +129,6 @@ test_expect_success 'GIT_DIR bare' '
> test_expect_success 'init --bare' '
>
> (
> - sane_unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&
> mkdir init-bare.git &&
> cd init-bare.git &&
> git init --bare
> @@ -149,7 +139,6 @@ test_expect_success 'init --bare' '
> test_expect_success 'GIT_DIR non-bare' '
>
> (
> - sane_unset GIT_CONFIG &&
> mkdir non-bare &&
> cd non-bare &&
> GIT_DIR=.git git init
> @@ -160,7 +149,6 @@ test_expect_success 'GIT_DIR non-bare' '
> test_expect_success 'GIT_DIR & GIT_WORK_TREE (1)' '
>
> (
> - sane_unset GIT_CONFIG &&
> mkdir git-dir-wt-1.git &&
> GIT_WORK_TREE=$(pwd) GIT_DIR=git-dir-wt-1.git git init
> ) &&
> @@ -170,7 +158,6 @@ test_expect_success 'GIT_DIR & GIT_WORK_TREE (1)' '
> test_expect_success 'GIT_DIR & GIT_WORK_TREE (2)' '
>
> if (
> - sane_unset GIT_CONFIG &&
> mkdir git-dir-wt-2.git &&
> GIT_WORK_TREE=$(pwd) GIT_DIR=git-dir-wt-2.git git --bare init
> )
> @@ -183,8 +170,6 @@ test_expect_success 'GIT_DIR & GIT_WORK_TREE (2)' '
> test_expect_success 'reinit' '
>
> (
> - sane_unset GIT_CONFIG GIT_WORK_TREE GIT_CONFIG &&
> -
> mkdir again &&
> cd again &&
> git init >out1 2>err1 &&
> diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
> index c3443ce..a812783 100755
> --- a/t/t9130-git-svn-authors-file.sh
> +++ b/t/t9130-git-svn-authors-file.sh
> @@ -97,7 +97,6 @@ test_expect_success 'fresh clone with svn.authors-file in config' '
> test x = x"$(git config svn.authorsfile)" &&
> test_config="$HOME"/.gitconfig &&
> sane_unset GIT_DIR &&
> - sane_unset GIT_CONFIG &&
> git config --global \
> svn.authorsfile "$HOME"/svn-authors &&
> test x"$HOME"/svn-authors = x"$(git config svn.authorsfile)" &&
> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> index 3edc408..ed98e64 100755
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -25,7 +25,6 @@ perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
> test_done
> }
>
> -unset GIT_DIR GIT_CONFIG
> WORKDIR=$(pwd)
> SERVERDIR=$(pwd)/gitcvs.git
> git_config="$SERVERDIR/config"
next prev parent reply other threads:[~2014-03-21 21:24 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <244284@gmane.comp.version-control.git>
2014-03-18 12:08 ` [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell David Tran
2014-03-18 20:37 ` Junio C Hamano
2014-03-18 21:45 ` Jeff King
2014-03-18 22:16 ` Junio C Hamano
2014-03-18 23:06 ` Jeff King
2014-03-19 17:28 ` Junio C Hamano
2014-03-20 23:11 ` [PATCH 0/12] GIT_CONFIG in the test suite Jeff King
2014-03-20 23:13 ` [PATCH 01/12] t/Makefile: stop setting GIT_CONFIG Jeff King
2014-03-20 23:13 ` [PATCH 02/12] t/test-lib: drop redundant unset of GIT_CONFIG Jeff King
2014-03-20 23:14 ` [PATCH 03/12] t: drop useless sane_unset GIT_* calls Jeff King
2014-03-21 21:24 ` Junio C Hamano [this message]
2014-03-24 21:56 ` Jeff King
2014-03-24 22:06 ` Junio C Hamano
2014-03-25 4:56 ` Junio C Hamano
2014-03-20 23:15 ` [PATCH 04/12] t: stop using GIT_CONFIG to cross repo boundaries Jeff King
2014-03-21 21:26 ` Junio C Hamano
2014-03-24 22:00 ` Jeff King
2014-03-20 23:15 ` [PATCH 05/12] t: prefer "git config --file" to GIT_CONFIG with test_must_fail Jeff King
2014-03-20 23:17 ` [PATCH 06/12] t: prefer "git config --file" to GIT_CONFIG Jeff King
2014-03-20 23:17 ` [PATCH 07/12] t0001: make symlink reinit test more careful Jeff King
2014-03-20 23:17 ` [PATCH 08/12] t0001: use test_path_is_* Jeff King
2014-03-20 23:18 ` [PATCH 09/12] t0001: use test_config_global Jeff King
2014-03-20 23:19 ` [PATCH 10/12] t0001: use test_must_fail Jeff King
2014-03-20 23:21 ` [PATCH 11/12] t0001: drop useless subshells Jeff King
2014-03-21 20:27 ` Eric Sunshine
2014-03-20 23:23 ` [PATCH 12/12] t0001: drop subshells just for "cd" Jeff King
2014-03-18 22:36 ` [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell Eric Sunshine
2014-03-18 20:52 ` Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqy503s0s0.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=unsignedzero@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.