From a9ebc43bd07e219922d1dab17df319f1cae5d600 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 Jul 2012 16:27:55 -0400 Subject: [PATCH 1/6] t7502: clean up fake_editor tests Using write_script saves us a few lines of code, and means we consistently use $SHELL_PATH. We can also drop the setting of the $pwd variable from $(pwd). In the first instance, there is no reason to use it (we can just use $(pwd) directly two lines later, since we are interpolating the here-document). In the second instance, it is totally pointless and probably just a cut-and-paste from the first instance. Finally, we can use a non-interpolating here document for the final script, which saves some quoting. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7502-commit.sh | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index 181456aa9..ddce53a96 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -266,13 +266,10 @@ test_expect_success 'committer is automatic' ' test_i18ncmp expect actual ' -pwd=`pwd` -cat >> .git/FAKE_EDITOR << EOF -#! /bin/sh -echo editor started > "$pwd/.git/result" +write_script .git/FAKE_EDITOR < "$(pwd)/.git/result" exit 0 EOF -chmod +x .git/FAKE_EDITOR test_expect_success 'do not fire editor in the presence of conflicts' ' @@ -300,9 +297,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' ' test "$(cat .git/result)" = "editor not started" ' -pwd=`pwd` -cat >.git/FAKE_EDITOR <.git/FAKE_EDITOR <"\$1" + cat "$1.orig" +) >"$1" EOF echo '## Custom template' >template From 34565f27fef10546a62c734c55c0d280aad63460 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 Jul 2012 16:28:00 -0400 Subject: [PATCH 2/6] t7502: properly quote GIT_EDITOR One of the tests tries to ensure that editor is not run due to an early failure. However, it needs to quote the pathname of the trash directory used in $GIT_EDITOR, since git will pass it along to the shell. In other words, the test would pass whether the code was correct or not, since the unquoted editor specification would never run. We never noticed the problem because the code is indeed correct, so git-commit never even tried to run the editor. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7502-commit.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index ddce53a96..3f9fb55a4 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -290,7 +290,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' ' test_must_fail git cherry-pick -n master && echo "editor not started" >.git/result && ( - GIT_EDITOR="$(pwd)/.git/FAKE_EDITOR" && + GIT_EDITOR="\"$(pwd)/.git/FAKE_EDITOR\"" && export GIT_EDITOR && test_must_fail git commit ) && From 1f4bf34578e93e8f5fa06652695e839bc51ea90f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 Jul 2012 16:30:29 -0400 Subject: [PATCH 3/6] t7502: narrow checks for author/committer name in template t7502.20 and t7502.21 check that the author and committer name are mentioned in the commit message template under certain circumstances. However, they end up checking a much larger and unnecessary portion of the template. Let's narrow their checks to the specific lines. While we're at it, let's give these tests more descriptive names, so their purposes are more obvious. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7502-commit.sh | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index 3f9fb55a4..efecb060f 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -235,24 +235,15 @@ test_expect_success 'cleanup commit messages (strip,-F,-e): output' ' test_i18ncmp expect actual ' -echo "# -# Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> -#" >> expect - -test_expect_success 'author different from committer' ' +test_expect_success 'message shows author when it is not equal to committer' ' echo >>negative && test_might_fail git commit -e -m "sample" && - head -n 7 .git/COMMIT_EDITMSG >actual && - test_i18ncmp expect actual + test_i18ngrep \ + "^# Author: *A U Thor \$" \ + .git/COMMIT_EDITMSG ' -mv expect expect.tmp -sed '$d' < expect.tmp > expect -rm -f expect.tmp -echo "# Committer: -#" >> expect - -test_expect_success 'committer is automatic' ' +test_expect_success 'message shows committer when it is automatic' ' echo >>negative && ( @@ -261,9 +252,9 @@ test_expect_success 'committer is automatic' ' # must fail because there is no change test_must_fail git commit -e -m "sample" ) && - head -n 8 .git/COMMIT_EDITMSG | \ - sed "s/^# Committer: .*/# Committer:/" >actual - test_i18ncmp expect actual + # the ident is calculated from the system, so we cannot + # check the actual value, only that it is there + test_i18ngrep "^# Committer: " .git/COMMIT_EDITMSG ' write_script .git/FAKE_EDITOR < Date: Thu, 26 Jul 2012 16:31:15 -0400 Subject: [PATCH 4/6] t7502: drop confusing test_might_fail call In t7502.20, we run "git commit" and check that it warns us that the author and committer identity are not the same (this is always the case in the test environment, since we set up the idents differently). Instead of actually making a commit, we have a clean index, so the "git commit" we run will fail. This is marked as might_fail, which is not really correct; it will always fail since there is nothing to commit. However, the only reason not to do a complete commit would be to see the intermediate state of the COMMIT_EDITMSG file when the commit is not completed. We don't need to care about this, though; even a complete commit will leave COMMIT_EDITMSG for us to view. By doing a real commit and dropping the might_fail, we are more robust against other unforeseen failures of "git commit" that might influence our test result. It might seem less robust to depend on the fact that "git commit" leaves COMMIT_EDITMSG in place after a successful commit. However, that brings this test in line with others parts of the script, which make the same assumption. Furthermore, if that ever does change, the right solution is not to prevent commit from completing, but to set EDITOR to a script that will record the contents we see. After all, the point of these tests is to check what the user sees in their EDITOR, so that would be the most direct test. For now, though, we can continue to use the "shortcut" that COMMIT_EDITMSG is left intact. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7502-commit.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index efecb060f..d261b8252 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -237,7 +237,7 @@ test_expect_success 'cleanup commit messages (strip,-F,-e): output' ' test_expect_success 'message shows author when it is not equal to committer' ' echo >>negative && - test_might_fail git commit -e -m "sample" && + git commit -e -m "sample" -a && test_i18ngrep \ "^# Author: *A U Thor \$" \ .git/COMMIT_EDITMSG From 1d7dc26498b5d7b7879d579f40be63210c50400c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 Jul 2012 16:32:31 -0400 Subject: [PATCH 5/6] t7502: handle systems where auto-identity is broken Test t7502.21 checks whether we write the committer name into COMMIT_EDITMSG when it has been automatically determined. However, not all systems can produce valid automatic identities. Prior to f20f387 (commit: check committer identity more strictly), this test worked even when we did not have a valid automatic identity, since it did not run the strict test until after we had generated the template. That commit tightened the check to fail early (since we would fail later, anyway), meaning that systems without a valid GECOS name or hostname would fail the test. We cannot just work around this, because it depends on configuration outside the control of the test script. Therefore we introduce a new test_prerequisite to run this test only on systems where automatic ident works at all. As a result, we can drop the confusing test_must_fail bit from the test. The intent was that by giving "git commit" invalid input (namely, nothing to commit), that it would stop at a predictable point, whether we had a valid identity or not, from which we could view the contents of COMMIT_EDITMSG. Since that assumption no longer holds, and we can only run the test when we have a valid identity, there is no reason not to let commit run to completion. That lets us be more robust to other unforeseen failures. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7502-commit.sh | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index d261b8252..c444812a4 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -243,14 +243,21 @@ test_expect_success 'message shows author when it is not equal to committer' ' .git/COMMIT_EDITMSG ' -test_expect_success 'message shows committer when it is automatic' ' +test_expect_success 'setup auto-ident prerequisite' ' + if (sane_unset GIT_COMMITTER_EMAIL && + sane_unset GIT_COMMITTER_NAME && + git var GIT_COMMITTER_IDENT); then + test_set_prereq AUTOIDENT + fi +' + +test_expect_success AUTOIDENT 'message shows committer when it is automatic' ' echo >>negative && ( sane_unset GIT_COMMITTER_EMAIL && sane_unset GIT_COMMITTER_NAME && - # must fail because there is no change - test_must_fail git commit -e -m "sample" + git commit -e -m "sample" -a ) && # the ident is calculated from the system, so we cannot # check the actual value, only that it is there From 8c8b3bc3f4e859be0af49f91e0d1831a9ae50324 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 Jul 2012 16:32:50 -0400 Subject: [PATCH 6/6] t7502: test early quit from commit with bad ident In commit f20f387, "git commit" notices and dies much earlier when we have a bogus commit identity. That commit did not add a test because we cannot do so reliably (namely, we can only trigger the behavior on a system where the automatically generated identity is bogus). However, now that we have a prerequisite check for this feature, we can add a test that will at least run on systems that produce such a bogus identity. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7502-commit.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index c444812a4..deb187eb7 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -248,6 +248,8 @@ test_expect_success 'setup auto-ident prerequisite' ' sane_unset GIT_COMMITTER_NAME && git var GIT_COMMITTER_IDENT); then test_set_prereq AUTOIDENT + else + test_set_prereq NOAUTOIDENT fi ' @@ -269,6 +271,21 @@ echo editor started > "$(pwd)/.git/result" exit 0 EOF +test_expect_success NOAUTOIDENT 'do not fire editor when committer is bogus' ' + >.git/result + >expect && + + echo >>negative && + ( + sane_unset GIT_COMMITTER_EMAIL && + sane_unset GIT_COMMITTER_NAME && + GIT_EDITOR="\"$(pwd)/.git/FAKE_EDITOR\"" && + export GIT_EDITOR && + test_must_fail git commit -e -m sample -a + ) && + test_cmp expect .git/result +' + test_expect_success 'do not fire editor in the presence of conflicts' ' git clean -f &&