{"thread":{"id":"31103","subject":"Test \"t/t7502-commit.sh\" failed","startedAt":"2012-07-26T06:27:52Z","lastAt":"2012-07-26T21:15:12Z","messageCount":15,"participants":["Jiang Xin","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"195817","messageId":"CANYiYbHbMw1HpvoCr4yBbWF=Q9Hoc1Zsq3-WoTrx4aQg7R0e4g@mail.gmail.com","threadId":"31103","inReplyTo":null,"subject":"Test \"t/t7502-commit.sh\" failed","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2012-07-26T06:27:52Z","receivedAt":"2012-07-26T06:27:52Z","isPatch":false,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Test \"t/t7502-commit.sh\" failed. I guess it's commit\nv1.7.9.7-1-gf20f387 which breaks it.\n\n $ git log -1 --oneline --stat v1.7.9.7-1-gf20f387\n f20f commit: check committer identity more strictly\n builtin/commit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nResult of t/t7502-commit.sh:\n\nnot ok - 21 committer is automatic\n#\n#\n# echo >>negative &&\n# (\n# sane_unset GIT_COMMITTER_EMAIL &&\n# sane_unset GIT_COMMITTER_NAME &&\n# # must fail because there is no change\n# test_must_fail git commit -e -m \"sample\"\n# ) &&\n# head -n 8 .git/COMMIT_EDITMSG | \\\n# sed \"s/^# Committer: .*/# Committer:/\" >actual\n# test_i18ncmp expect actual\n#\n\nContents of file expect:\n\n sample\n\n # Please enter the commit message for your changes. Lines starting\n # with '#' will be ignored, and an empty message aborts the commit.\n #\n # Author: A U Thor \n # Committer:\n #\n\nContents of file actual:\n\n sample\n\n-- \nJiang Xin\n"},{"id":"195851","messageId":"20120726130348.GA965@sigill.intra.peff.net","threadId":"31103","inReplyTo":"CANYiYbHbMw1HpvoCr4yBbWF=Q9Hoc1Zsq3-WoTrx4aQg7R0e4g@mail.gmail.com","subject":"Re: Test \"t/t7502-commit.sh\" failed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T13:03:49Z","receivedAt":"2012-07-26T13:03:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 26, 2012 at 02:27:52PM +0800, Jiang Xin wrote:\n\n> Test \"t/t7502-commit.sh\" failed. I guess it's commit\n> v1.7.9.7-1-gf20f387 which breaks it.\n> \n> $ git log -1 --oneline --stat v1.7.9.7-1-gf20f387\n> f20f commit: check committer identity more strictly\n> builtin/commit.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> Result of t/t7502-commit.sh:\n> \n> not ok - 21 committer is automatic\n> #\n> #\n> # echo >>negative &&\n> # (\n> # sane_unset GIT_COMMITTER_EMAIL &&\n> # sane_unset GIT_COMMITTER_NAME &&\n> # # must fail because there is no change\n> # test_must_fail git commit -e -m \"sample\"\n> # ) &&\n> # head -n 8 .git/COMMIT_EDITMSG | \\\n> # sed \"s/^# Committer: .*/# Committer:/\" >actual\n> # test_i18ncmp expect actual\n> #\n\nHmm. It doesn't fail here, but that is probably because I have my name\nset properly in /etc/passwd. I think the test is bogus, though. From the\nresults you report:\n\n> Contents of file expect:\n> \n> sample\n> \n> # Please enter the commit message for your changes. Lines starting\n> # with '#' will be ignored, and an empty message aborts the commit.\n> #\n> # Author: A U Thor \n> # Committer:\n> #\n> \n> Contents of file actual:\n> \n> sample\n\nThe test is expecting us to write out the commit template for the user\nto edit. But the whole point of f20f387 is to fail early, before we ask\nthe user to edit the template. So the test is trying to check that we\nwrote _something_ into the Committer field, even though that something\nwould not necessarily be used to make the commit object (because the\ncode path for the commit object is going to be much stricter).\n\nI am not sure that the test is really all that useful. The point seems\nto be that we fall back to some kind of system-based ident, but that is\nnot portable. On some systems we can, and on some we can't, depending on\nthings like how /etc/passwd and the hostname are configured.\n\nI'll see if I can make it more robust, but I think the right solution\nmay simply be to rip out the test.\n\n-Peff\n"},{"id":"195864","messageId":"7vtxwu8orw.fsf@alter.siamese.dyndns.org","threadId":"31103","inReplyTo":"20120726130348.GA965@sigill.intra.peff.net","subject":"Re: Test \"t/t7502-commit.sh\" failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T16:34:27Z","receivedAt":"2012-07-26T16:34:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jul 26, 2012 at 02:27:52PM +0800, Jiang Xin wrote:\n> ...\n>> not ok - 21 committer is automatic\n>> #\n>> #\n>> # echo >>negative &&\n>> # (\n>> # sane_unset GIT_COMMITTER_EMAIL &&\n>> # sane_unset GIT_COMMITTER_NAME &&\n>> # # must fail because there is no change\n>> # test_must_fail git commit -e -m \"sample\"\n>> # ) &&\n>> # head -n 8 .git/COMMIT_EDITMSG | \\\n>> # sed \"s/^# Committer: .*/# Committer:/\" >actual\n>> # test_i18ncmp expect actual\n>> #\n>\n> Hmm. It doesn't fail here, but that is probably because I have my name\n> set properly in /etc/passwd. I think the test is bogus, though. From the\n> results you report:\n>\n>> Contents of file expect:\n>> \n>> sample\n>> \n>> # Please enter the commit message for your changes. Lines starting\n>> # with '#' will be ignored, and an empty message aborts the commit.\n>> #\n>> # Author: A U Thor \n>> # Committer:\n>> #\n>> \n>> Contents of file actual:\n>> \n>> sample\n>\n> The test is expecting us to write out the commit template for the user\n> to edit. But the whole point of f20f387 is to fail early, before we ask\n> the user to edit the template. So the test is trying to check that we\n> wrote _something_ into the Committer field, even though that something\n> would not necessarily be used to make the commit object (because the\n> code path for the commit object is going to be much stricter).\n>\n> I am not sure that the test is really all that useful. The point seems\n> to be that we fall back to some kind of system-based ident, but that is\n> not portable.\n\nI think the point is to make sure that the \"# Committer:\" line is\ngiven to the reader to remind that we took the codepath that comes\nup with a committer ident by using untrustworthy heuristics. You\nare correct that the usefulness of the value of system-based ident\nvaries between systems (that is why it is stripped out with sed),\nthough.\n\nYou earlier gave a reason why f20f387 (commit: check committer identity\nmore strictly, 2012-07-23) does not have a test for it; I think the\nsame reason applies why this test is unworkable.\n\nA related tangent; all the test vectors in this script seems to be\ntoo wide, and we probably would want to narrow them for what each\ntest wants to see. For example, the test in question only wants to\nsee \"# Committer: \" and it does not matter\nif the template was rewritten in future versions of Git so that it\ndoes not begin with \"# Please enter...\". Similarly, the one\nprevious only wants to see \"# Author: \".\n"},{"id":"195869","messageId":"20120726171256.GC13942@sigill.intra.peff.net","threadId":"31103","inReplyTo":"7vtxwu8orw.fsf@alter.siamese.dyndns.org","subject":"Re: Test \"t/t7502-commit.sh\" failed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T17:12:56Z","receivedAt":"2012-07-26T17:12:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 26, 2012 at 09:34:27AM -0700, Junio C Hamano wrote:\n\n> >> not ok - 21 committer is automatic\n> [...]\n> > I am not sure that the test is really all that useful. The point seems\n> > to be that we fall back to some kind of system-based ident, but that is\n> > not portable.\n> \n> I think the point is to make sure that the \"# Committer:\" line is\n> given to the reader to remind that we took the codepath that comes\n> up with a committer ident by using untrustworthy heuristics. You\n> are correct that the usefulness of the value of system-based ident\n> varies between systems (that is why it is stripped out with sed),\n> though.\n\nAh, right. I was led astray by the crappy test title. When viewed with\nthe test immediately prior (which checks that \"Author:\" is shown in the\ntemplate), it makes more sense.\n\n> You earlier gave a reason why f20f387 (commit: check committer identity\n> more strictly, 2012-07-23) does not have a test for it; I think the\n> same reason applies why this test is unworkable.\n\nRight. You can check this only when \"git var GIT_COMMITTER_IDENT\" works,\nand you can check the f20f387 behavior only when it does _not_ work. So\nwe could do something like:\n\n (sane_unset GIT_COMMITTER_NAME &&\n sane_unset GIT_COMMITTER_EMAIL &&\n git var GIT_COMMITTER_IDENT >/dev/null) &&\n test_set_prereq AUTOIDENT ||\n test_set_prereq NOAUTOIDENT\n\n test_expect_success AUTOIDENT \\\n 'mention auto ident in commit template'\n '...'\n\n test_expect_success NOAUTOIDENT \\\n 'git rejects bogus ident before starting editor'\n '...'\n\nBut it is somewhat unsatisfying to only get random test coverage\ndepending on how your system happens to be configured. I guess we\nsomewhat have that already with the case-insensitivity tests.\n\nDo we want to go that route, or just drop this test completely?\n\n> A related tangent; all the test vectors in this script seems to be\n> too wide, and we probably would want to narrow them for what each\n> test wants to see. For example, the test in question only wants to\n> see \"# Committer: \" and it does not matter\n> if the template was rewritten in future versions of Git so that it\n> does not begin with \"# Please enter...\". Similarly, the one\n> previous only wants to see \"# Author: \".\n\nAgreed. They should probably just i18ngrep for \"^# Committer: \" or\nsimilar.\n\n-Peff\n"},{"id":"195885","messageId":"7v8ve672ar.fsf@alter.siamese.dyndns.org","threadId":"31103","inReplyTo":"20120726171256.GC13942@sigill.intra.peff.net","subject":"Re: Test \"t/t7502-commit.sh\" failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T19:25:16Z","receivedAt":"2012-07-26T19:25:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Right. You can check this only when \"git var GIT_COMMITTER_IDENT\" works,\n> and you can check the f20f387 behavior only when it does _not_ work. So\n> we could do something like:\n>\n> (sane_unset GIT_COMMITTER_NAME &&\n> sane_unset GIT_COMMITTER_EMAIL &&\n> git var GIT_COMMITTER_IDENT >/dev/null) &&\n> test_set_prereq AUTOIDENT ||\n> test_set_prereq NOAUTOIDENT\n>\n> test_expect_success AUTOIDENT \\\n> 'mention auto ident in commit template'\n> '...'\n>\n> test_expect_success NOAUTOIDENT \\\n> 'git rejects bogus ident before starting editor'\n> '...'\n>\n> But it is somewhat unsatisfying to only get random test coverage\n> depending on how your system happens to be configured. I guess we\n> somewhat have that already with the case-insensitivity tests.\n>\n> Do we want to go that route, or just drop this test completely?\n\nThere are three cases with respect to ident:\n\n - There is a user-configured one;\n\n - We derive one from the system and that is syntactically correct,\n but we know from the past experience the system is often\n misconfigured.\n\n - We derive one from the system and that is empty.\n\nBefore your tightening commit, the latter two cases were treated the\nsame way and gave the reminder to the user. After the tightening,\nthese were separated into two and give different results.\n\nPerhaps the tightening was not such a good idea in the first place?\nThe user would have seen a bad committer ident in the log editing\nsession without the new code anyway, so perhaps we should have added\na messasge e.g. \"Abort the editor session and do not edit further;\nfix your ident first--this commit will fail anyway\" there, or\nsomething?\n\nThe second case can lead to commits that the user may have to redo\nwith filter-branch, as the command does not even error out. And we\ndo want to make sure that the user is given a chance to verify that\nthe commit will carry a name that the user is happy with with this\ntest. I think that is far more important than a user on a system\nwith broken GECOS field has to run the editor twice.\n"},{"id":"195886","messageId":"20120726193359.GA28711@sigill.intra.peff.net","threadId":"31103","inReplyTo":"7v8ve672ar.fsf@alter.siamese.dyndns.org","subject":"Re: Test \"t/t7502-commit.sh\" failed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T19:33:59Z","receivedAt":"2012-07-26T19:33:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 26, 2012 at 12:25:16PM -0700, Junio C Hamano wrote:\n\n> There are three cases with respect to ident:\n> \n> - There is a user-configured one;\n> \n> - We derive one from the system and that is syntactically correct,\n> but we know from the past experience the system is often\n> misconfigured.\n> \n> - We derive one from the system and that is empty.\n\nYes, modulo s/empty/bogus in some way/ in the last one (e.g., we will\ncomplain about both empty names and bogus hostnames).\n\n> Before your tightening commit, the latter two cases were treated the\n> same way and gave the reminder to the user. After the tightening,\n> these were separated into two and give different results.\n\nSort of. They were _not_ treated the same in the long run, as the second\none is OK, but the third case would eventually fail. It is only that\nthey were treated the same for the first half of git-commit running,\nmeaning it got as far as running the editor.\n\nThe problem is that the test relied on git-commit reaching a certain\npoint before failing in case 3. But that is a bad assumption of the\ntest, and one that actively hurts users (who would rather git fail\nearly).\n\n> Perhaps the tightening was not such a good idea in the first place?\n> The user would have seen a bad committer ident in the log editing\n> session without the new code anyway, so perhaps we should have added\n> a messasge e.g. \"Abort the editor session and do not edit further;\n> fix your ident first--this commit will fail anyway\" there, or\n> something?\n\nWe could do that, but why? We _know_ it's going to fail, so why not just\nfail?\n\n> The second case can lead to commits that the user may have to redo\n> with filter-branch, as the command does not even error out.\n\nSure. And that's why we show the committer at all. In other words, we\nhave three levels of confidence with three outcomes:\n\n 1. The user definitely told us who they are. Proceed without warning.\n\n 2. We guessed who the user is, and we have reasonable confidence that\n it is right. Proceed, but warn.\n\n 3. We guessed who the user is, but we know that it is syntactically\n bad. Do not proceed.\n\nThat has always been the behavior, and was not changed by the recent\ntightening. Only _when_ we stopped proceeding changed, and that is not\nsomething I think this test should care about.\n\n> And we do want to make sure that the user is given a chance to verify\n> that the commit will carry a name that the user is happy with with\n> this test. I think that is far more important than a user on a system\n> with broken GECOS field has to run the editor twice.\n\nYes, and they are given that chance. This is not about the behavior of\ngit, but about stupid assumptions by the test.\n\nI'm close to finished with a series that I think will at least make it\nbetter. Stay tuned.\n\n-Peff\n"},{"id":"195889","messageId":"7v4nou70gl.fsf@alter.siamese.dyndns.org","threadId":"31103","inReplyTo":"20120726193359.GA28711@sigill.intra.peff.net","subject":"Re: Test \"t/t7502-commit.sh\" failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T20:04:58Z","receivedAt":"2012-07-26T20:04:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, and they are given that chance. This is not about the behavior of\n> git, but about stupid assumptions by the test.\n\nOK.\n\n> I'm close to finished with a series that I think will at least make it\n> better. Stay tuned.\n\n;-)\n"},{"id":"195891","messageId":"20120726202644.GA15043@sigill.intra.peff.net","threadId":"31103","inReplyTo":"20120726193359.GA28711@sigill.intra.peff.net","subject":"[PATCH 0/6] t7502 fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T20:26:44Z","receivedAt":"2012-07-26T20:26:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 26, 2012 at 03:33:59PM -0400, Jeff King wrote:\n\n> I'm close to finished with a series that I think will at least make it\n> better. Stay tuned.\n\nHere it is. I was able to replicate the original problem by munging my\n/etc/passwd, and I've confirmed that this series fixes it by skipping\nthe related test. I also added in a test for the early-quit behavior\nintroduced by f20f387. It can't run everywhere, of course, but it seems\nwe have at least one tester whose system will run it (and I did confirm\nthat it works with my munged /etc/passwd).\n\nOne other option is that we could introduce a\nGIT_PRETEND_THIS_IS_MY_GECOS_NAME variable. That would let us wrong both\nsides of the test on all systems. I just hate to add an interface that\nwill be carried around in production code just for the sake of a test.\n\n [1/6]: t7502: clean up fake_editor tests\n [2/6]: t7502: properly quote GIT_EDITOR\n [3/6]: t7502: narrow checks for author/committer name in template\n [4/6]: t7502: drop confusing test_might_fail call\n [5/6]: t7502: handle systems where auto-identity is broken\n [6/6]: t7502: test early quit from commit with bad ident\n\n-Peff\n"},{"id":"195892","messageId":"20120726202754.GA16048@sigill.intra.peff.net","threadId":"31103","inReplyTo":"20120726202644.GA15043@sigill.intra.peff.net","subject":"[PATCH 1/6] t7502: clean up fake_editor tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T20:27:55Z","receivedAt":"2012-07-26T20:27:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using write_script saves us a few lines of code, and means\nwe consistently use $SHELL_PATH.\n\nWe can also drop the setting of the $pwd variable from\n$(pwd). In the first instance, there is no reason to use it\n(we can just use $(pwd) directly two lines later, since we\nare interpolating the here-document). In the second\ninstance, it is totally pointless and probably just a\ncut-and-paste from the first instance.\n\nFinally, we can use a non-interpolating here document for\nthe final script, which saves some quoting.\n\nSigned-off-by: Jeff King \n---\n t/t7502-commit.sh | 20 +++++++-------------\n 1 file changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 181456a..ddce53a 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -266,13 +266,10 @@ test_expect_success 'committer is automatic' '\n \ttest_i18ncmp expect actual\n '\n \n-pwd=`pwd`\n-cat >> .git/FAKE_EDITOR << EOF\n-#! /bin/sh\n-echo editor started > \"$pwd/.git/result\"\n+write_script .git/FAKE_EDITOR < \"$(pwd)/.git/result\"\n exit 0\n EOF\n-chmod +x .git/FAKE_EDITOR\n \n test_expect_success 'do not fire editor in the presence of conflicts' '\n \n@@ -300,9 +297,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' '\n \ttest \"$(cat .git/result)\" = \"editor not started\"\n '\n \n-pwd=`pwd`\n-cat >.git/FAKE_EDITOR <.git/FAKE_EDITOR <\"\\$1\"\n+\tcat \"$1.orig\"\n+) >\"$1\"\n EOF\n \n echo '## Custom template' >template\n-- \n1.7.11.3.8.ge78f547\n"},{"id":"195893","messageId":"20120726202759.GB16048@sigill.intra.peff.net","threadId":"31103","inReplyTo":"20120726202644.GA15043@sigill.intra.peff.net","subject":"[PATCH 2/6] t7502: properly quote GIT_EDITOR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T20:28:00Z","receivedAt":"2012-07-26T20:28:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of the tests tries to ensure that editor is not run due\nto an early failure. However, it needs to quote the pathname\nof the trash directory used in $GIT_EDITOR, since git will\npass it along to the shell. In other words, the test would\npass whether the code was correct or not, since the unquoted\neditor specification would never run.\n\nWe never noticed the problem because the code is indeed\ncorrect, so git-commit never even tried to run the editor.\n\nSigned-off-by: Jeff King \n---\n t/t7502-commit.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex ddce53a..3f9fb55 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -290,7 +290,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' '\n \ttest_must_fail git cherry-pick -n master &&\n \techo \"editor not started\" >.git/result &&\n \t(\n-\t\tGIT_EDITOR=\"$(pwd)/.git/FAKE_EDITOR\" &&\n+\t\tGIT_EDITOR=\"\\\"$(pwd)/.git/FAKE_EDITOR\\\"\" &&\n \t\texport GIT_EDITOR &&\n \t\ttest_must_fail git commit\n \t) &&\n-- \n1.7.11.3.8.ge78f547\n"},{"id":"195894","messageId":"20120726203029.GC16048@sigill.intra.peff.net","threadId":"31103","inReplyTo":"20120726202644.GA15043@sigill.intra.peff.net","subject":"[PATCH 3/6] t7502: narrow checks for author/committer name in template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T20:30:29Z","receivedAt":"2012-07-26T20:30:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"t7502.20 and t7502.21 check that the author and committer\nname are mentioned in the commit message template under\ncertain circumstances. However, they end up checking a much\nlarger and unnecessary portion of the template. Let's narrow\ntheir checks to the specific lines.\n\nWhile we're at it, let's give these tests more descriptive\nnames, so their purposes are more obvious.\n\nSigned-off-by: Jeff King \n---\nThe test just above still checks the \"Please write your...\" part of the\nmessage. But it is purely about using \"-F -e --cleanup=strip\", and is\ntesting whether we correctly include the instructions. I guess we could\nlimit it to just checking for \"^# \" or something if we didn't want to\ndepend on the actual text.\n\nI'm inclined to just leave it for now.\n\n t/t7502-commit.sh | 25 ++++++++-----------------\n 1 file changed, 8 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 3f9fb55..efecb06 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -235,24 +235,15 @@ test_expect_success 'cleanup commit messages (strip,-F,-e): output' '\n \ttest_i18ncmp expect actual\n '\n \n-echo \"#\n-# Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>\n-#\" >> expect\n-\n-test_expect_success 'author different from committer' '\n+test_expect_success 'message shows author when it is not equal to committer' '\n \techo >>negative &&\n \ttest_might_fail git commit -e -m \"sample\" &&\n-\thead -n 7 .git/COMMIT_EDITMSG >actual &&\n-\ttest_i18ncmp expect actual\n+\ttest_i18ngrep \\\n+\t \"^# Author: *A U Thor \\$\" \\\n+\t .git/COMMIT_EDITMSG\n '\n \n-mv expect expect.tmp\n-sed '$d' < expect.tmp > expect\n-rm -f expect.tmp\n-echo \"# Committer:\n-#\" >> expect\n-\n-test_expect_success 'committer is automatic' '\n+test_expect_success 'message shows committer when it is automatic' '\n \n \techo >>negative &&\n \t(\n@@ -261,9 +252,9 @@ test_expect_success 'committer is automatic' '\n \t\t# must fail because there is no change\n \t\ttest_must_fail git commit -e -m \"sample\"\n \t) &&\n-\thead -n 8 .git/COMMIT_EDITMSG |\t\\\n-\tsed \"s/^# Committer: .*/# Committer:/\" >actual\n-\ttest_i18ncmp expect actual\n+\t# the ident is calculated from the system, so we cannot\n+\t# check the actual value, only that it is there\n+\ttest_i18ngrep \"^# Committer: \" .git/COMMIT_EDITMSG\n '\n \n write_script .git/FAKE_EDITOR <\n---\nSorry for the long explanation. It took me a long time to figure out why\nthis test_might_fail is there at all, so I wanted to present my full\nanalysis as to why it's OK to remove.\n\n t/t7502-commit.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex efecb06..d261b82 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -237,7 +237,7 @@ test_expect_success 'cleanup commit messages (strip,-F,-e): output' '\n \n test_expect_success 'message shows author when it is not equal to committer' '\n \techo >>negative &&\n-\ttest_might_fail git commit -e -m \"sample\" &&\n+\tgit commit -e -m \"sample\" -a &&\n \ttest_i18ngrep \\\n \t \"^# Author: *A U Thor \\$\" \\\n \t .git/COMMIT_EDITMSG\n-- \n1.7.11.3.8.ge78f547\n"},{"id":"195896","messageId":"20120726203231.GE16048@sigill.intra.peff.net","threadId":"31103","inReplyTo":"20120726202644.GA15043@sigill.intra.peff.net","subject":"[PATCH 5/6] t7502: handle systems where auto-identity is broken","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T20:32:31Z","receivedAt":"2012-07-26T20:32:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Test t7502.21 checks whether we write the committer name\ninto COMMIT_EDITMSG when it has been automatically\ndetermined. However, not all systems can produce valid\nautomatic identities.\n\nPrior to f20f387 (commit: check committer identity more\nstrictly), this test worked even when we did not have a\nvalid automatic identity, since it did not run the strict\ntest until after we had generated the template. That commit\ntightened the check to fail early (since we would fail\nlater, anyway), meaning that systems without a valid GECOS\nname or hostname would fail the test.\n\nWe cannot just work around this, because it depends on\nconfiguration outside the control of the test script.\nTherefore we introduce a new test_prerequisite to run this\ntest only on systems where automatic ident works at all.\n\nAs a result, we can drop the confusing test_must_fail bit\nfrom the test. The intent was that by giving \"git commit\"\ninvalid input (namely, nothing to commit), that it would\nstop at a predictable point, whether we had a valid identity\nor not, from which we could view the contents of\nCOMMIT_EDITMSG. Since that assumption no longer holds, and\nwe can only run the test when we have a valid identity,\nthere is no reason not to let commit run to completion. That\nlets us be more robust to other unforeseen failures.\n\nSigned-off-by: Jeff King \n---\nThis should fix the test failure that started the thread. Please let me\nknow if it doesn't.\n\n t/t7502-commit.sh | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex d261b82..c444812 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -243,14 +243,21 @@ test_expect_success 'message shows author when it is not equal to committer' '\n \t .git/COMMIT_EDITMSG\n '\n \n-test_expect_success 'message shows committer when it is automatic' '\n+test_expect_success 'setup auto-ident prerequisite' '\n+\tif (sane_unset GIT_COMMITTER_EMAIL &&\n+\t sane_unset GIT_COMMITTER_NAME &&\n+\t git var GIT_COMMITTER_IDENT); then\n+\t\ttest_set_prereq AUTOIDENT\n+\tfi\n+'\n+\n+test_expect_success AUTOIDENT 'message shows committer when it is automatic' '\n \n \techo >>negative &&\n \t(\n \t\tsane_unset GIT_COMMITTER_EMAIL &&\n \t\tsane_unset GIT_COMMITTER_NAME &&\n-\t\t# must fail because there is no change\n-\t\ttest_must_fail git commit -e -m \"sample\"\n+\t\tgit commit -e -m \"sample\" -a\n \t) &&\n \t# the ident is calculated from the system, so we cannot\n \t# check the actual value, only that it is there\n-- \n1.7.11.3.8.ge78f547\n"},{"id":"195897","messageId":"20120726203250.GF16048@sigill.intra.peff.net","threadId":"31103","inReplyTo":"20120726202644.GA15043@sigill.intra.peff.net","subject":"[PATCH 6/6] t7502: test early quit from commit with bad ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T20:32:50Z","receivedAt":"2012-07-26T20:32:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In commit f20f387, \"git commit\" notices and dies much\nearlier when we have a bogus commit identity. That commit\ndid not add a test because we cannot do so reliably (namely,\nwe can only trigger the behavior on a system where the\nautomatically generated identity is bogus). However, now\nthat we have a prerequisite check for this feature, we can\nadd a test that will at least run on systems that produce\nsuch a bogus identity.\n\nSigned-off-by: Jeff King \n---\n t/t7502-commit.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex c444812..deb187e 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -248,6 +248,8 @@ test_expect_success 'setup auto-ident prerequisite' '\n \t sane_unset GIT_COMMITTER_NAME &&\n \t git var GIT_COMMITTER_IDENT); then\n \t\ttest_set_prereq AUTOIDENT\n+\telse\n+\t\ttest_set_prereq NOAUTOIDENT\n \tfi\n '\n \n@@ -269,6 +271,21 @@ echo editor started > \"$(pwd)/.git/result\"\n exit 0\n EOF\n \n+test_expect_success NOAUTOIDENT 'do not fire editor when committer is bogus' '\n+\t>.git/result\n+\t>expect &&\n+\n+\techo >>negative &&\n+\t(\n+\t\tsane_unset GIT_COMMITTER_EMAIL &&\n+\t\tsane_unset GIT_COMMITTER_NAME &&\n+\t\tGIT_EDITOR=\"\\\"$(pwd)/.git/FAKE_EDITOR\\\"\" &&\n+\t\texport GIT_EDITOR &&\n+\t\ttest_must_fail git commit -e -m sample -a\n+\t) &&\n+\ttest_cmp expect .git/result\n+'\n+\n test_expect_success 'do not fire editor in the presence of conflicts' '\n \n \tgit clean -f &&\n-- \n1.7.11.3.8.ge78f547\n"},{"id":"195899","messageId":"7vobn25in3.fsf@alter.siamese.dyndns.org","threadId":"31103","inReplyTo":"20120726202644.GA15043@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] t7502 fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T21:15:12Z","receivedAt":"2012-07-26T21:15:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All patches look very sensible. Thanks.\n"}]}