On Mon, May 26, 2014 at 2:56 PM, Caleb Thompson <cjaysson@xxxxxxxxx> wrote:
> t/t7507-commit-verbose.sh was using a global test_set_editor call to
> build its environment.
>
> Rather than building global state with test_set_editor at the beginning
> of the file, move test_set_editor calls into each test.
Rather than repeating in prose what the patch itself says more
concisely and precisely, explain the reason for this change. For
instance, you might replace the above two sentences with something
like this (or better):
Improve robustness against global state changes by having each
test set up the test-editor it requires rather than relying upon
the editor set once at script start.
> Besides being inline with current practices, it also allows the tests
s/inline/in line/
> which required GIT_EDITOR=cat to avoid using a subshell and simplify
> their logic.
"required" sounds odd here. Perhaps:
...which set GIT_EDITOR=cat manually...
More below.
> Signed-off-by: Caleb Thompson <caleb@xxxxxxxxxxxxxxxx>
> ---
> t/t7507-commit-verbose.sh | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index e62d921..310b68b 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -6,7 +6,6 @@ test_description='verbose commit template'
> write_script check-for-diff <<-EOF
> exec grep '^diff --git' "\$1"
> EOF
> -test_set_editor "$(pwd)/check-for-diff"
>
> cat >message <<'EOF'
> subject
> @@ -21,10 +20,12 @@ test_expect_success 'setup' '
> '
>
> test_expect_success 'initial commit shows verbose diff' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> git commit --amend -v
> '
>
> test_expect_success 'second commit' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> echo content modified >file &&
> git add file &&
> git commit -F message
This test does not invoke the test-editor at all, so it's misleading
to insert test_set_editor here.
> @@ -36,11 +37,13 @@ check_message() {
> }
>
> test_expect_success 'verbose diff is stripped out' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> git commit --amend -v &&
> check_message message
> '
>
> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> test_config diff.mnemonicprefix true &&
> git commit --amend -v &&
> check_message message
> @@ -59,16 +62,19 @@ index 0000000..f95c11d
> EOF
>
> test_expect_success 'diff in message is retained without -v' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> git commit --amend -F diff &&
> check_message diff
> '
Also misleading, unnecessary test_set_editor.
> test_expect_success 'diff in message is retained with -v' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> git commit --amend -F diff -v &&
> check_message diff
> '
Ditto.
> test_expect_success 'submodule log is stripped out too with -v' '
> + test_set_editor "$(pwd)/check-for-diff" &&
Unnecessary. The editor isn't invoked until the test_must_fail line,
and by then you've already overridden it with 'test_set_editor cat'.
> test_config diff.submodule log &&
> git submodule add ./. sub &&
> git commit -m "sub added" &&
> @@ -77,20 +83,14 @@ test_expect_success 'submodule log is stripped out too with -v' '
> echo "more" >>file &&
> git commit -a -m "submodule commit"
> ) &&
> - (
> - GIT_EDITOR=cat &&
> - export GIT_EDITOR &&
> - test_must_fail git commit -a -v 2>err
> - ) &&
> + test_set_editor cat &&
> + test_must_fail git commit -a -v 2>err
Broken &&-chain.
> test_i18ngrep "Aborting commit due to empty commit message." err
> '
>
> test_expect_success 'verbose diff is stripped out with set core.commentChar' '
> - (
> - GIT_EDITOR=cat &&
> - export GIT_EDITOR &&
> - test_must_fail git -c core.commentchar=";" commit -a -v 2>err
> - ) &&
> + test_set_editor cat &&
> + test_must_fail git -c core.commentchar=";" commit -a -v 2>err
Broken &&-chain.
> test_i18ngrep "Aborting commit due to empty commit message." err
> '
>
> --
> 1.9.3
>
--
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