On Sat, Apr 9, 2016 at 7:19 AM, Michael Rappazzo <rappazzo@xxxxxxxxx> wrote:
> t1500-rev-parse has many tests which change directories and leak
> environment variables. This makes it difficult to add new tests without
> minding the environment variables and current directory.
>
> Each test is now setup, executed, and cleaned up without leaving anything
> behind. Tests which have textual expectations have been converted to use
> test_cmp (which will show a diff when the test is run with --verbose).
It might be easier to review this if broken into several cleanup and
modernization patches, however, some comments below...
> Signed-off-by: Michael Rappazzo <rappazzo@xxxxxxxxx>
> ---
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> @@ -3,88 +3,571 @@
> +test_expect_success '.git/: is-bare-repository' '
> + (cd .git && test false = "$(git rev-parse --is-bare-repository)")
> +'
>
> -# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
> +test_expect_success '.git/: is-inside-git-dir' '
> + (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
Simpler:
test true = "$(git -C .git rev-parse --is-inside-git-dir)"
> +'
>
> -ROOT=$(pwd)
> +test_expect_success '.git/: is-inside-work-tree' '
> + (cd .git && test false = "$(git rev-parse --is-inside-work-tree)")
Ditto.
> +'
>
> -test_rev_parse toplevel false false true '' .git "$ROOT/.git"
> +test_expect_success '.git/: prefix' '
> + (
> + cd .git &&
> + echo >expected &&
> + git rev-parse --show-prefix >actual &&
> + test_cmp expected actual
> + )
Likewise, you could drop the entire subshell:
echo >expected &&
git -C .git rev-parse --show-prefix >actual &&
test_cmp expected actual
> +'
>
> +test_expect_success '.git/: git-dir' '
> + (
> + cd .git &&
> + echo . >expected &&
> + git rev-parse --git-dir >actual &&
> + test_cmp expected actual
> + )
Same here and for many subsequent tests (which I won't quote).
> +'
> +test_expect_success 'core.bare = true: is-bare-repository' '
> + git config core.bare true &&
> + test_when_finished "git config core.bare false" &&
> + test true = "$(git rev-parse --is-bare-repository)"
Simpler:
test_config core.bare true
and then you can drop 'test_when_finished' altogether. However, even simpler:
test true = "$(git -c core.bare=false rev-parse --is-bare-repository)"
which allows you to drop 'test_config', as well.
Ditto for subsequent tests (which I won't quote).
> +'
> +test_expect_success 'core.bare undefined: is-bare-repository' '
> + git config --unset core.bare &&
test_unconfig core.bare
> + test_when_finished "git config core.bare false" &&
Why does this need to re-instate core.bare?
Same comments for subsequent tests.
> + test false = "$(git rev-parse --is-bare-repository)"
> +'
> +test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' '
> + mkdir work &&
> + test_when_finished "rm -rf work && git config core.bare false" &&
> + (
> + cd work &&
> + export GIT_DIR=../.git &&
> + export GIT_CONFIG="$(pwd)"/../.git/config
> + git config core.bare false &&
> + test false = "$(git rev-parse --is-bare-repository)"
> + )
> +'
Same comments about -C to avoid the subshell and -c for configuration.
Also, you can do one-shot environment variable setting for the command
invocation, so the subshell goes away, and everything inside the
subshell collapses to:
test false = "$(GIT_DIR=... GIT_CONFIG=...
git -C work -c core.bare=false rev-parse ...)"
Additionally, I'm confused about why this test "reverts" the
core.bare=false by setting core.bare=false in 'test_when_finished'.
Ditto for subsequent tests.
> +test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
> + mkdir work &&
> + cp -r .git repo.git &&
> + test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
If 'cp' fails, then 'test_when_finished' will never be invoked, which
means that the cleanup will never happen; thus 'test_when_finished'
needs to be called earlier. Ditto for subsequent tests.
> + (
> + cd work &&
> + export GIT_DIR=../repo.git &&
> + export GIT_CONFIG="$(pwd)"/../repo.git/config
> + git config core.bare false &&
> + test false = "$(git rev-parse --is-bare-repository)"
> + )
> +'
Closing comments:
By using -C, -c, and one-shot environment variables, you can ditch the
subshells, and most of these tests should collapse to one or two
lines.
There seems to be a lot of repetition here. To reduce the repetition,
have you considered encoding the state which varies between tests into
a table and making the tests table-driven. Or, by encoding the varying
state in some nested for-loops? The nice thing about driving the tests
from a table or for-loops is that it is easier to see at a glance if
all cases are covered.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html