{"thread":{"id":"32228","subject":"[PATCH 0/5] jk/send-email-sender-prompt loose ends","startedAt":"2012-11-28T18:25:35Z","lastAt":"2012-11-28T21:37:19Z","messageCount":12,"participants":["Jeff King","Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"204175","messageId":"20121128182534.GA21020@sigill.intra.peff.net","threadId":"32228","inReplyTo":null,"subject":"[PATCH 0/5] jk/send-email-sender-prompt loose ends","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T18:25:35Z","receivedAt":"2012-11-28T18:25:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are the cleanups and refactorings split out from my\njk/send-email-sender-prompt series. They can go right on master and are\nindependent of Felipe's fc/send-email-no-sender-prompt topic.\n\n  [1/5]: test-lib: allow negation of prerequisites\n\nSame as before. I think this is a useful feature for the test suite (and\n2/5 depends on it).\n\n  [2/5]: t7502: factor out autoident prerequisite\n\nSame as before. Patch 5/5 depends on it.\n\n  [3/5]: ident: make user_ident_explicitly_given static\n\nSame as before. Cleanup.\n\n  [4/5]: ident: keep separate \"explicit\" flags for author and committer\n\nSame as before. Cleanup. I do not know if anybody will ever care about\nthe corner cases it fixes, so it is really just being defensive for\nfuture code.\n\n  [5/5]: t: add tests for \"git var\"\n\nTests split out from he \"git var can take multiple values\" patch.\n\nDropped were:\n\n  - \"git var\" can take multiple values. Nobody really cares about doing\n    so, and it's an external interface, so we'd have to support it\n    forever.\n\n  - exporting \"explicit ident\" flag via \"git var\"; same reasoning as\n    above\n\n  - Git.pm supporting explicit ident; ditto\n\n  - send-email prompting change; obsoleted by Felipe's patch\n\n-Peff\n"},{"id":"204176","messageId":"20121128182559.GA17122@sigill.intra.peff.net","threadId":"32228","inReplyTo":"20121128182534.GA21020@sigill.intra.peff.net","subject":"[PATCH 1/5] test-lib: allow negation of prerequisites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T18:25:59Z","receivedAt":"2012-11-28T18:25:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You can set and test a prerequisite like this:\n\n  test_set_prereq FOO\n  test_have_prereq FOO && echo yes\n\nYou can negate the test in the shell like this:\n\n  ! test_have_prereq && echo no\n\nHowever, when you are using the automatic prerequisite\nchecking in test_expect_*, there is no opportunity to use\nthe shell negation.  This patch introduces the syntax \"!FOO\"\nto indicate that the test should only run if a prerequisite\nis not meant.\n\nOne alternative is to set an explicit negative prerequisite,\nlike:\n\n  if system_has_foo; then\n\t  test_set_prereq FOO\n  else\n\t  test_set_prereq NO_FOO\n  fi\n\nHowever, this doesn't work for lazy prerequisites, which\nassociate a single test with a single name. We could teach\nthe lazy prereq evaluator to set both forms, but the code\nchange ends up quite similar to this one (because we still\nneed to convert NO_FOO into FOO to find the correct lazy\nscript).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0000-basic.sh        | 32 ++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh | 21 ++++++++++++++++++++-\n 2 files changed, 52 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 08677df..562cf41 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -115,6 +115,38 @@ then\n \texit 1\n fi\n \n+test_lazy_prereq LAZY_TRUE true\n+havetrue=no\n+test_expect_success LAZY_TRUE 'test runs if lazy prereq is satisfied' '\n+\thavetrue=yes\n+'\n+donthavetrue=yes\n+test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '\n+\tdonthavetrue=no\n+'\n+\n+if test \"$havetrue$donthavetrue\" != yesyes\n+then\n+\tsay 'bug in test framework: lazy prerequisites do not work'\n+\texit 1\n+fi\n+\n+test_lazy_prereq LAZY_FALSE false\n+nothavefalse=no\n+test_expect_success !LAZY_FALSE 'negative lazy prereqs checked' '\n+\tnothavefalse=yes\n+'\n+havefalse=yes\n+test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '\n+\thavefalse=no\n+'\n+\n+if test \"$nothavefalse$havefalse\" != yesyes\n+then\n+\tsay 'bug in test framework: negative lazy prerequisites do not work'\n+\texit 1\n+fi\n+\n clean=no\n test_expect_success 'tests clean up after themselves' '\n \ttest_when_finished clean=yes\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8889ba5..22a4f8f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -275,6 +275,15 @@ test_have_prereq () {\n \n \tfor prerequisite\n \tdo\n+\t\tcase \"$prerequisite\" in\n+\t\t!*)\n+\t\t\tnegative_prereq=t\n+\t\t\tprerequisite=${prerequisite#!}\n+\t\t\t;;\n+\t\t*)\n+\t\t\tnegative_prereq=\n+\t\tesac\n+\n \t\tcase \" $lazily_tested_prereq \" in\n \t\t*\" $prerequisite \"*)\n \t\t\t;;\n@@ -294,10 +303,20 @@ test_have_prereq () {\n \t\ttotal_prereq=$(($total_prereq + 1))\n \t\tcase \"$satisfied_prereq\" in\n \t\t*\" $prerequisite \"*)\n+\t\t\tsatisfied_this_prereq=t\n+\t\t\t;;\n+\t\t*)\n+\t\t\tsatisfied_this_prereq=\n+\t\tesac\n+\n+\t\tcase \"$satisfied_this_prereq,$negative_prereq\" in\n+\t\tt,|,t)\n \t\t\tok_prereq=$(($ok_prereq + 1))\n \t\t\t;;\n \t\t*)\n-\t\t\t# Keep a list of missing prerequisites\n+\t\t\t# Keep a list of missing prerequisites; restore\n+\t\t\t# the negative marker if necessary.\n+\t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n \t\t\tif test -z \"$missing_prereq\"\n \t\t\tthen\n \t\t\t\tmissing_prereq=$prerequisite\n-- \n1.8.0.207.gdf2154c\n"},{"id":"204177","messageId":"20121128182606.GB17122@sigill.intra.peff.net","threadId":"32228","inReplyTo":"20121128182534.GA21020@sigill.intra.peff.net","subject":"[PATCH 2/5] t7502: factor out autoident prerequisite","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T18:26:06Z","receivedAt":"2012-11-28T18:26:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"t7502 checks the behavior of commit when we can and cannot\ndetermine a valid committer ident. Let's move that into\ntest-lib as a lazy prerequisite so other scripts can use it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t7502-commit.sh | 12 +-----------\n t/test-lib.sh     |  6 ++++++\n 2 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex deb187e..1a5cb69 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -243,16 +243,6 @@ test_expect_success 'message shows author when it is not equal to committer' '\n \t  .git/COMMIT_EDITMSG\n '\n \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-\telse\n-\t\ttest_set_prereq NOAUTOIDENT\n-\tfi\n-'\n-\n test_expect_success AUTOIDENT 'message shows committer when it is automatic' '\n \n \techo >>negative &&\n@@ -271,7 +261,7 @@ 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+test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '\n \t>.git/result\n \t>expect &&\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 489bc80..0334a9e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -738,6 +738,12 @@ test_lazy_prereq UTF8_NFD_TO_NFC '\n \tesac\n '\n \n+test_lazy_prereq AUTOIDENT '\n+\tsane_unset GIT_AUTHOR_NAME &&\n+\tsane_unset GIT_AUTHOR_EMAIL &&\n+\tgit var GIT_AUTHOR_IDENT\n+'\n+\n # When the tests are run as root, permission tests will report that\n # things are writable when they shouldn't be.\n test -w / || test_set_prereq SANITY\n-- \n1.8.0.207.gdf2154c\n"},{"id":"204178","messageId":"20121128182613.GC17122@sigill.intra.peff.net","threadId":"32228","inReplyTo":"20121128182534.GA21020@sigill.intra.peff.net","subject":"[PATCH 3/5] ident: make user_ident_explicitly_given static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T18:26:13Z","receivedAt":"2012-11-28T18:26:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In v1.5.6-rc0~56^2 (2008-05-04) \"user_ident_explicitly_given\"\nwas introduced as a global for communication between config,\nident, and builtin-commit.  In v1.7.0-rc0~72^2 (2010-01-07)\nreaders switched to using the common wrapper\nuser_ident_sufficiently_given().  After v1.7.11-rc1~15^2~18\n(2012-05-21), the var is only written in ident.c.\n\nNow we can make it static, which will enable further\nrefactoring without worrying about upsetting other code.\n\nSigned-off-by: Jeff King <peff@peff.net>\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n cache.h | 4 ----\n ident.c | 6 +++++-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex dbd8018..50d9eea 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1149,10 +1149,6 @@ struct config_include_data {\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n-#define IDENT_NAME_GIVEN 01\n-#define IDENT_MAIL_GIVEN 02\n-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n-extern int user_ident_explicitly_given;\n extern int user_ident_sufficiently_given(void);\n \n extern const char *git_commit_encoding;\ndiff --git a/ident.c b/ident.c\nindex a4bf206..733d69d 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -10,7 +10,11 @@\n static struct strbuf git_default_name = STRBUF_INIT;\n static struct strbuf git_default_email = STRBUF_INIT;\n static char git_default_date[50];\n-int user_ident_explicitly_given;\n+\n+#define IDENT_NAME_GIVEN 01\n+#define IDENT_MAIL_GIVEN 02\n+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n+static int user_ident_explicitly_given;\n \n #ifdef NO_GECOS_IN_PWENT\n #define get_gecos(ignored) \"&\"\n-- \n1.8.0.207.gdf2154c\n"},{"id":"204179","messageId":"20121128182626.GD17122@sigill.intra.peff.net","threadId":"32228","inReplyTo":"20121128182534.GA21020@sigill.intra.peff.net","subject":"[PATCH 4/5] ident: keep separate \"explicit\" flags for author and committer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T18:26:26Z","receivedAt":"2012-11-28T18:26:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep track of whether the user ident was given to us\nexplicitly, or if we guessed at it from system parameters\nlike username and hostname. However, we kept only a single\nvariable. This covers the common cases (because the author\nand committer will usually come from the same explicit\nsource), but can miss two cases:\n\n  1. GIT_COMMITTER_* is set explicitly, but we fallback for\n     GIT_AUTHOR. We claim the ident is explicit, even though\n     the author is not.\n\n  2. GIT_AUTHOR_* is set and we ask for author ident, but\n     not committer ident. We will claim the ident is\n     implicit, even though it is explicit.\n\nThis patch uses two variables instead of one, updates both\nwhen we set the \"fallback\" values, and updates them\nindividually when we read from the environment.\n\nRather than keep user_ident_sufficiently_given as a\ncompatibility wrapper, we update the only two callers to\ncheck the committer_ident, which matches their intent and\nwhat was happening already.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit.c |  4 ++--\n cache.h          |  3 ++-\n ident.c          | 32 +++++++++++++++++++++++++-------\n 3 files changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1dd2ec5..d6dd3df 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\tident_shown++ ? \"\" : \"\\n\",\n \t\t\t\tauthor_ident->buf);\n \n-\t\tif (!user_ident_sufficiently_given())\n+\t\tif (!committer_ident_sufficiently_given())\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n \t\t\t\t_(\"%s\"\n \t\t\t\t\"Committer: %s\"),\n@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \t\tstrbuf_addstr(&format, \"\\n Author: \");\n \t\tstrbuf_addbuf_percentquote(&format, &author_ident);\n \t}\n-\tif (!user_ident_sufficiently_given()) {\n+\tif (!committer_ident_sufficiently_given()) {\n \t\tstrbuf_addstr(&format, \"\\n Committer: \");\n \t\tstrbuf_addbuf_percentquote(&format, &committer_ident);\n \t\tif (advice_implicit_identity) {\ndiff --git a/cache.h b/cache.h\nindex 50d9eea..18fdd18 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1149,7 +1149,8 @@ struct config_include_data {\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n-extern int user_ident_sufficiently_given(void);\n+extern int committer_ident_sufficiently_given(void);\n+extern int author_ident_sufficiently_given(void);\n \n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\ndiff --git a/ident.c b/ident.c\nindex 733d69d..ac9672f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -14,7 +14,8 @@ static char git_default_date[50];\n #define IDENT_NAME_GIVEN 01\n #define IDENT_MAIL_GIVEN 02\n #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n-static int user_ident_explicitly_given;\n+static int committer_ident_explicitly_given;\n+static int author_ident_explicitly_given;\n \n #ifdef NO_GECOS_IN_PWENT\n #define get_gecos(ignored) \"&\"\n@@ -113,7 +114,8 @@ const char *ident_default_email(void)\n \n \t\tif (email && email[0]) {\n \t\t\tstrbuf_addstr(&git_default_email, email);\n-\t\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\t} else\n \t\t\tcopy_email(xgetpwuid_self(), &git_default_email);\n \t\tstrbuf_trim(&git_default_email);\n@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)\n \n const char *git_author_info(int flag)\n {\n+\tif (getenv(\"GIT_AUTHOR_NAME\"))\n+\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\tif (getenv(\"GIT_AUTHOR_EMAIL\"))\n+\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_AUTHOR_NAME\"),\n \t\t\t getenv(\"GIT_AUTHOR_EMAIL\"),\n \t\t\t getenv(\"GIT_AUTHOR_DATE\"),\n@@ -340,16 +346,16 @@ const char *git_author_info(int flag)\n const char *git_committer_info(int flag)\n {\n \tif (getenv(\"GIT_COMMITTER_NAME\"))\n-\t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \tif (getenv(\"GIT_COMMITTER_EMAIL\"))\n-\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n \t\t\t getenv(\"GIT_COMMITTER_EMAIL\"),\n \t\t\t getenv(\"GIT_COMMITTER_DATE\"),\n \t\t\t flag);\n }\n \n-int user_ident_sufficiently_given(void)\n+static int ident_is_sufficient(int user_ident_explicitly_given)\n {\n #ifndef WINDOWS\n \treturn (user_ident_explicitly_given & IDENT_MAIL_GIVEN);\n@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)\n #endif\n }\n \n+int committer_ident_sufficiently_given(void)\n+{\n+\treturn ident_is_sufficient(committer_ident_explicitly_given);\n+}\n+\n+int author_ident_sufficiently_given(void)\n+{\n+\treturn ident_is_sufficient(author_ident_explicitly_given);\n+}\n+\n int git_ident_config(const char *var, const char *value, void *data)\n {\n \tif (!strcmp(var, \"user.name\")) {\n@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\t\treturn config_error_nonbool(var);\n \t\tstrbuf_reset(&git_default_name);\n \t\tstrbuf_addstr(&git_default_name, value);\n-\t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \t\treturn 0;\n \t}\n \n@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\t\treturn config_error_nonbool(var);\n \t\tstrbuf_reset(&git_default_email);\n \t\tstrbuf_addstr(&git_default_email, value);\n-\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\treturn 0;\n \t}\n \n-- \n1.8.0.207.gdf2154c\n"},{"id":"204180","messageId":"20121128182643.GE17122@sigill.intra.peff.net","threadId":"32228","inReplyTo":"20121128182534.GA21020@sigill.intra.peff.net","subject":"[PATCH 5/5] t: add tests for \"git var\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T18:26:43Z","receivedAt":"2012-11-28T18:26:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not currently have any explicit tests for \"git var\" at\nall (though we do exercise it to some degree as a part of\nother tests). Let's add a few basic sanity checks.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0007-git-var.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 49 insertions(+)\n create mode 100755 t/t0007-git-var.sh\n\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nnew file mode 100755\nindex 0000000..5868a87\n--- /dev/null\n+++ b/t/t0007-git-var.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='basic sanity checks for git var'\n+. ./test-lib.sh\n+\n+test_expect_success 'get GIT_AUTHOR_IDENT' '\n+\ttest_tick &&\n+\techo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\" >expect &&\n+\tgit var GIT_AUTHOR_IDENT >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'get GIT_COMMITTER_IDENT' '\n+\ttest_tick &&\n+\techo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\" >expect &&\n+\tgit var GIT_COMMITTER_IDENT >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success !AUTOIDENT 'requested identites are strict' '\n+\t(\n+\t\tsane_unset GIT_COMMITTER_NAME &&\n+\t\tsane_unset GIT_COMMITTER_EMAIL &&\n+\t\ttest_must_fail git var GIT_COMMITTER_IDENT\n+\t)\n+'\n+\n+# For git var -l, we check only a representative variable;\n+# testing the whole output would make our test too brittle with\n+# respect to unrelated changes in the test suite's environment.\n+test_expect_success 'git var -l lists variables' '\n+\tgit var -l >actual &&\n+\techo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\" >expect &&\n+\tsed -n s/GIT_AUTHOR_IDENT=//p <actual >actual.author &&\n+\ttest_cmp expect actual.author\n+'\n+\n+test_expect_success 'git var -l lists config' '\n+\tgit var -l >actual &&\n+\techo false >expect &&\n+\tsed -n s/core\\\\.bare=//p <actual >actual.bare &&\n+\ttest_cmp expect actual.bare\n+'\n+\n+test_expect_success 'listing and asking for variables are exclusive' '\n+\ttest_must_fail git var -l GIT_COMMITTER_IDENT\n+'\n+\n+test_done\n-- \n1.8.0.207.gdf2154c\n"},{"id":"204183","messageId":"20121128184229.GA3993@sigill.intra.peff.net","threadId":"32228","inReplyTo":"20121128182534.GA21020@sigill.intra.peff.net","subject":"[PATCH 6/5] t9001: check send-email behavior with implicit sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T18:42:29Z","receivedAt":"2012-11-28T18:42:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 28, 2012 at 01:25:35PM -0500, Jeff King wrote:\n\n> Here are the cleanups and refactorings split out from my\n> jk/send-email-sender-prompt series. They can go right on master and are\n> independent of Felipe's fc/send-email-no-sender-prompt topic.\n> [...]\n> Dropped were:\n> [...]\n>   - send-email prompting change; obsoleted by Felipe's patch\n\nHere is one more patch pulling out the extra tests from my final commit.\nIt needs to go on top of the merge of Felipe's series and the one I just\nsent. The former because of the new prompting behavior, and the latter\nbecause of the AUTOIDENT prerequisites.\n\nIf it's simpler, my whole series can just go on top of Felipe's patch\n(or vice versa).\n\n-- >8 --\nSubject: [PATCH] t9001: check send-email behavior with implicit sender\n\nWe allow send-email to use an implicitly-defined identity\nfor the sender (because there is still a confirmation step),\nbut we abort when we cannot generate such an identity. Let's\nmake sure that we test this.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t9001-send-email.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex c5d66cf..27edfa8 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -201,6 +201,34 @@ test_expect_success $PREREQ 'Prompting works' '\n \t\tgrep \"^To: to@example.com\\$\" msgtxt1\n '\n \n+test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '\n+       clean_fake_sendmail &&\n+       (sane_unset GIT_AUTHOR_NAME &&\n+\tsane_unset GIT_AUTHOR_EMAIL &&\n+\tsane_unset GIT_COMMITTER_NAME &&\n+\tsane_unset GIT_COMMITTER_EMAIL &&\n+\tGIT_SEND_EMAIL_NOTTY=1 git send-email \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--to=to@example.com \\\n+\t\t$patches \\\n+\t\t</dev/null 2>errors\n+       )\n+'\n+\n+test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '\n+       clean_fake_sendmail &&\n+       (sane_unset GIT_AUTHOR_NAME &&\n+\tsane_unset GIT_AUTHOR_EMAIL &&\n+\tsane_unset GIT_COMMITTER_NAME &&\n+\tsane_unset GIT_COMMITTER_EMAIL &&\n+\tGIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&\n+\ttest_must_fail git send-email \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t$patches </dev/null 2>errors &&\n+\ttest_i18ngrep \"tell me who you are\" errors\n+       )\n+'\n+\n test_expect_success $PREREQ 'tocmd works' '\n \tclean_fake_sendmail &&\n \tcp $patches tocmd.patch &&\n-- \n1.8.0.207.gdf2154c\n"},{"id":"204184","messageId":"7vk3t5v9q1.fsf@alter.siamese.dyndns.org","threadId":"32228","inReplyTo":"20121128184229.GA3993@sigill.intra.peff.net","subject":"Re: [PATCH 6/5] t9001: check send-email behavior with implicit sender","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-28T18:55:02Z","receivedAt":"2012-11-28T18:55:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Nov 28, 2012 at 01:25:35PM -0500, Jeff King wrote:\n>\n>> Here are the cleanups and refactorings split out from my\n>> jk/send-email-sender-prompt series. They can go right on master and are\n>> independent of Felipe's fc/send-email-no-sender-prompt topic.\n>> [...]\n>> Dropped were:\n>> [...]\n>>   - send-email prompting change; obsoleted by Felipe's patch\n>\n> Here is one more patch pulling out the extra tests from my final commit.\n> It needs to go on top of the merge of Felipe's series and the one I just\n> sent. The former because of the new prompting behavior, and the latter\n> because of the AUTOIDENT prerequisites.\n>\n> If it's simpler, my whole series can just go on top of Felipe's patch\n> (or vice versa).\n>\n> -- >8 --\n> Subject: [PATCH] t9001: check send-email behavior with implicit sender\n>\n> We allow send-email to use an implicitly-defined identity\n> for the sender (because there is still a confirmation step),\n> but we abort when we cannot generate such an identity. Let's\n> make sure that we test this.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t9001-send-email.sh | 28 ++++++++++++++++++++++++++++\n>  1 file changed, 28 insertions(+)\n>\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index c5d66cf..27edfa8 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -201,6 +201,34 @@ test_expect_success $PREREQ 'Prompting works' '\n>  \t\tgrep \"^To: to@example.com\\$\" msgtxt1\n>  '\n>  \n> +test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '\n> +       clean_fake_sendmail &&\n> +       (sane_unset GIT_AUTHOR_NAME &&\n> +\tsane_unset GIT_AUTHOR_EMAIL &&\n> +\tsane_unset GIT_COMMITTER_NAME &&\n> +\tsane_unset GIT_COMMITTER_EMAIL &&\n> +\tGIT_SEND_EMAIL_NOTTY=1 git send-email \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> +\t\t--to=to@example.com \\\n> +\t\t$patches \\\n> +\t\t</dev/null 2>errors\n> +       )\n> +'\n> +\n> +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '\n> +       clean_fake_sendmail &&\n> +       (sane_unset GIT_AUTHOR_NAME &&\n> +\tsane_unset GIT_AUTHOR_EMAIL &&\n> +\tsane_unset GIT_COMMITTER_NAME &&\n> +\tsane_unset GIT_COMMITTER_EMAIL &&\n> +\tGIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&\n> +\ttest_must_fail git send-email \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> +\t\t$patches </dev/null 2>errors &&\n> +\ttest_i18ngrep \"tell me who you are\" errors\n> +       )\n> +'\n\nThe difference between these two tests should solely come from\nAUTOIDENT and nothing else, so it would be better to see their\ncommand line arguments to match; the former is with --to and the\nlatter is without in this patch but I do not think you meant them to\ndiffer that way.\n\n\n\n>  test_expect_success $PREREQ 'tocmd works' '\n>  \tclean_fake_sendmail &&\n>  \tcp $patches tocmd.patch &&\n"},{"id":"204195","messageId":"20121128200626.GA4292@sigill.intra.peff.net","threadId":"32228","inReplyTo":"7vk3t5v9q1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/5] t9001: check send-email behavior with implicit sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T20:06:26Z","receivedAt":"2012-11-28T20:06:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 28, 2012 at 10:55:02AM -0800, Junio C Hamano wrote:\n\n> > +test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '\n> > +       clean_fake_sendmail &&\n> > +       (sane_unset GIT_AUTHOR_NAME &&\n> > +\tsane_unset GIT_AUTHOR_EMAIL &&\n> > +\tsane_unset GIT_COMMITTER_NAME &&\n> > +\tsane_unset GIT_COMMITTER_EMAIL &&\n> > +\tGIT_SEND_EMAIL_NOTTY=1 git send-email \\\n> > +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> > +\t\t--to=to@example.com \\\n> > +\t\t$patches \\\n> > +\t\t</dev/null 2>errors\n> > +       )\n> > +'\n> > +\n> > +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '\n> > +       clean_fake_sendmail &&\n> > +       (sane_unset GIT_AUTHOR_NAME &&\n> > +\tsane_unset GIT_AUTHOR_EMAIL &&\n> > +\tsane_unset GIT_COMMITTER_NAME &&\n> > +\tsane_unset GIT_COMMITTER_EMAIL &&\n> > +\tGIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&\n> > +\ttest_must_fail git send-email \\\n> > +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> > +\t\t$patches </dev/null 2>errors &&\n> > +\ttest_i18ngrep \"tell me who you are\" errors\n> > +       )\n> > +'\n> \n> The difference between these two tests should solely come from\n> AUTOIDENT and nothing else, so it would be better to see their\n> command line arguments to match; the former is with --to and the\n> latter is without in this patch but I do not think you meant them to\n> differ that way.\n\nYeah, that makes sense. The top one originally was testing that we\nstill prompted in the autoident case, and so had some echos piped into\nsend-email. I simplified it to use --to, but I agree it is even better\nto show that the commands are identical.\n\nHere's a cleaned up version that makes it more obvious the commands are\nthe same (it also fixes a few minor whitespace problems on the\nindentation, which you can see from the quoting above).\n\n-- >8 --\nSubject: [PATCH] t9001: check send-email behavior with implicit sender\n\nWe allow send-email to use an implicitly-defined identity\nfor the sender (because there is still a confirmation step),\nbut we abort when we cannot generate such an identity. Let's\nmake sure that we test this.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t9001-send-email.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex c5d66cf..97d6f4c 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -201,6 +201,34 @@ test_expect_success $PREREQ 'Prompting works' '\n \t\tgrep \"^To: to@example.com\\$\" msgtxt1\n '\n \n+test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '\n+\tclean_fake_sendmail &&\n+\t(sane_unset GIT_AUTHOR_NAME &&\n+\tsane_unset GIT_AUTHOR_EMAIL &&\n+\tsane_unset GIT_COMMITTER_NAME &&\n+\tsane_unset GIT_COMMITTER_EMAIL &&\n+\tGIT_SEND_EMAIL_NOTTY=1 git send-email \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--to=to@example.com \\\n+\t\t$patches </dev/null 2>errors\n+\t)\n+'\n+\n+test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '\n+\tclean_fake_sendmail &&\n+\t(sane_unset GIT_AUTHOR_NAME &&\n+\tsane_unset GIT_AUTHOR_EMAIL &&\n+\tsane_unset GIT_COMMITTER_NAME &&\n+\tsane_unset GIT_COMMITTER_EMAIL &&\n+\tGIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&\n+\ttest_must_fail git send-email \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--to=to@example.com \\\n+\t\t$patches </dev/null 2>errors &&\n+\ttest_i18ngrep \"tell me who you are\" errors\n+\t)\n+'\n+\n test_expect_success $PREREQ 'tocmd works' '\n \tclean_fake_sendmail &&\n \tcp $patches tocmd.patch &&\n-- \n1.8.0.207.gdf2154c\n"},{"id":"204198","messageId":"20121128201713.GA9249@sigill.intra.peff.net","threadId":"32228","inReplyTo":"20121128200626.GA4292@sigill.intra.peff.net","subject":"Re: [PATCH 6/5] t9001: check send-email behavior with implicit sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-28T20:17:13Z","receivedAt":"2012-11-28T20:17:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 28, 2012 at 03:06:26PM -0500, Jeff King wrote:\n\n> Here's a cleaned up version that makes it more obvious the commands are\n> the same (it also fixes a few minor whitespace problems on the\n> indentation, which you can see from the quoting above).\n\nI wondered how painful it would be to actually run the command and then\nconditionally check the results inside the test. I ended up with this:\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex c5d66cf..9c8fac1 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -201,6 +201,30 @@ test_expect_success $PREREQ 'Prompting works' '\n \t\tgrep \"^To: to@example.com\\$\" msgtxt1\n '\n \n+test_expect_success $PREREQ 'handle implicit ident' '\n+\tclean_fake_sendmail &&\n+\t(\n+\t\tsane_unset GIT_AUTHOR_NAME &&\n+\t\tsane_unset GIT_AUTHOR_EMAIL &&\n+\t\tsane_unset GIT_COMMITTER_NAME &&\n+\t\tsane_unset GIT_COMMITTER_EMAIL &&\n+\t\tGIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&\n+\t\t{\n+\t\t\tgit send-email \\\n+\t\t\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t\t\t--to=to@example.com \\\n+\t\t\t\t$patches </dev/null 2>errors;\n+\t\t\texit_code=$?\n+\t\t} &&\n+\t\tif test_have_prereq AUTOIDENT; then\n+\t\t\ttest $exit_code = 0\n+\t\telse\n+\t\t\ttest $exit_code != 0 &&\n+\t\t\ttest_i18ngrep \"tell me who you are\" errors\n+\t\tfi\n+\t)\n+'\n+\n test_expect_success $PREREQ 'tocmd works' '\n \tclean_fake_sendmail &&\n \tcp $patches tocmd.patch &&\n\nwhich does work, though it is less nice that the protections and nice\nsyntax of \"test_must_fail\" must be abandoned (unless we want to do\nsomething horrible like 'test_must_fail sh -c \"exit $exit_code\"'.\nI could go either way.\n\n-Peff\n"},{"id":"204202","messageId":"7vpq2xtq66.fsf@alter.siamese.dyndns.org","threadId":"32228","inReplyTo":"20121128201713.GA9249@sigill.intra.peff.net","subject":"Re: [PATCH 6/5] t9001: check send-email behavior with implicit sender","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-28T20:42:41Z","receivedAt":"2012-11-28T20:42:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Nov 28, 2012 at 03:06:26PM -0500, Jeff King wrote:\n>\n>> Here's a cleaned up version that makes it more obvious the commands are\n>> the same (it also fixes a few minor whitespace problems on the\n>> indentation, which you can see from the quoting above).\n>\n> I wondered how painful it would be to actually run the command and then\n> conditionally check the results inside the test. I ended up with this:\n> ...\n> which does work, though it is less nice that the protections and nice\n> syntax of \"test_must_fail\" must be abandoned (unless we want to do\n> something horrible like 'test_must_fail sh -c \"exit $exit_code\"'.\n> I could go either way.\n\nA change along that line did cross my mind, and I agree that it does\nclarify that we are testing the same command produces an expected\nresult, the expectation may be different depending on external\nconditions, and we can figure out what the expected values are\nbefore running the test.  I do not think we use such a pattern in\nvery many places, though.\n\nThe differences in \"expected results\" are generally not limited to\nthe final outcome in $? (e.g. your \"else\" side not just checks $?\nindicates an error, but makes sure that we got an expected error\nmessage), which means that the if statement at the end has to be\nthere in some form anyway, which in turn means that a new test\nhelper function to abstract this pattern out further would not\nbuy us very much (either it would be too complex and bug prone to\nimplement, or it would be too simplistic and limited).\n\nI'd prefer to leave these as two separate tests for now like your\nprevious patch.  People interested in consolidating/simplifying can\nfirst audit the existing tests to find other instances of this\npattern to see if it is worth doing, and then come up with an\nabstraction to be shared among them.\n\nThanks.\n"},{"id":"204209","messageId":"CAMP44s1uVK-m_jczhVui0Z_Tcv-vtSVU8E1uVKY7QdQvQhMQFg@mail.gmail.com","threadId":"32228","inReplyTo":"20121128182534.GA21020@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] jk/send-email-sender-prompt loose ends","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-28T21:37:19Z","receivedAt":"2012-11-28T21:37:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Nov 28, 2012 at 7:25 PM, Jeff King <peff@peff.net> wrote:\n> Here are the cleanups and refactorings split out from my\n> jk/send-email-sender-prompt series. They can go right on master and are\n> independent of Felipe's fc/send-email-no-sender-prompt topic.\n>\n>   [1/5]: test-lib: allow negation of prerequisites\n>\n> Same as before. I think this is a useful feature for the test suite (and\n> 2/5 depends on it).\n>\n>   [2/5]: t7502: factor out autoident prerequisite\n>\n> Same as before. Patch 5/5 depends on it.\n>\n>   [3/5]: ident: make user_ident_explicitly_given static\n>\n> Same as before. Cleanup.\n>\n>   [4/5]: ident: keep separate \"explicit\" flags for author and committer\n>\n> Same as before. Cleanup. I do not know if anybody will ever care about\n> the corner cases it fixes, so it is really just being defensive for\n> future code.\n>\n>   [5/5]: t: add tests for \"git var\"\n>\n> Tests split out from he \"git var can take multiple values\" patch.\n>\n> Dropped were:\n>\n>   - \"git var\" can take multiple values. Nobody really cares about doing\n>     so, and it's an external interface, so we'd have to support it\n>     forever.\n>\n>   - exporting \"explicit ident\" flag via \"git var\"; same reasoning as\n>     above\n>\n>   - Git.pm supporting explicit ident; ditto\n>\n>   - send-email prompting change; obsoleted by Felipe's patch\n\nFor what it's worth; they look good to me. However, I think it's worth\nmentioning that there are no functional changes.\n\n-- \nFelipe Contreras\n"}]}