threads / patch / 32228

patch, 5 partsjk/send-email-sender-prompt loose ends

Subject: [PATCH 0/5] jk/send-email-sender-prompt loose ends

## tl;dr

12 messages between Nov 28, 2012 and Nov 28, 2012. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Jeff King· Nov 28, 2012, 18:25 UTC · lore

Here are the cleanups and refactorings split out from my jk/send-email-sender-prompt series. They can go right on master and are independent of Felipe's fc/send-email-no-sender-prompt topic.

  [1/5]: test-lib: allow negation of prerequisites

Same as before. I think this is a useful feature for the test suite (and 2/5 depends on it).

  [2/5]: t7502: factor out autoident prerequisite
Same as before. Patch 5/5 depends on it.
  [3/5]: ident: make user_ident_explicitly_given static
Same as before. Cleanup.
  [4/5]: ident: keep separate "explicit" flags for author and committer

Same as before. Cleanup. I do not know if anybody will ever care about the corner cases it fixes, so it is really just being defensive for future code.

  [5/5]: t: add tests for "git var"
Tests split out from he "git var can take multiple values" patch.
Dropped were:
  - "git var" can take multiple values. Nobody really cares about doing
    so, and it's an external interface, so we'd have to support it
    forever.
  - exporting "explicit ident" flag via "git var"; same reasoning as
    above
  - Git.pm supporting explicit ident; ditto
  - send-email prompting change; obsoleted by Felipe's patch
-Peff
Jeff King· Nov 28, 2012, 18:25 UTC · re: Jeff King · lore

[PATCH 1/5] test-lib: allow negation of prerequisites

You can set and test a prerequisite like this:
  test_set_prereq FOO
  test_have_prereq FOO && echo yes
You can negate the test in the shell like this:
  ! test_have_prereq && echo no

However, when you are using the automatic prerequisite checking in test_expect_*, there is no opportunity to use the shell negation. This patch introduces the syntax "!FOO" to indicate that the test should only run if a prerequisite is not meant.

One alternative is to set an explicit negative prerequisite, like:

  if system_has_foo; then
	  test_set_prereq FOO
  else
	  test_set_prereq NO_FOO
  fi

However, this doesn't work for lazy prerequisites, which associate a single test with a single name. We could teach the lazy prereq evaluator to set both forms, but the code change ends up quite similar to this one (because we still need to convert NO_FOO into FOO to find the correct lazy script).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0000-basic.sh        | 32 ++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh | 21 ++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)
Show changes to 2 files +52 −1

t/t0000-basic.sh, t/test-lib-functions.sh

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 08677df..562cf41 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -115,6 +115,38 @@ then
 	exit 1
 fi
 
+test_lazy_prereq LAZY_TRUE true
+havetrue=no
+test_expect_success LAZY_TRUE 'test runs if lazy prereq is satisfied' '
+	havetrue=yes
+'
+donthavetrue=yes
+test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
+	donthavetrue=no
+'
+
+if test "$havetrue$donthavetrue" != yesyes
+then
+	say 'bug in test framework: lazy prerequisites do not work'
+	exit 1
+fi
+
+test_lazy_prereq LAZY_FALSE false
+nothavefalse=no
+test_expect_success !LAZY_FALSE 'negative lazy prereqs checked' '
+	nothavefalse=yes
+'
+havefalse=yes
+test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '
+	havefalse=no
+'
+
+if test "$nothavefalse$havefalse" != yesyes
+then
+	say 'bug in test framework: negative lazy prerequisites do not work'
+	exit 1
+fi
+
 clean=no
 test_expect_success 'tests clean up after themselves' '
 	test_when_finished clean=yes
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8889ba5..22a4f8f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -275,6 +275,15 @@ test_have_prereq () {
 
 	for prerequisite
 	do
+		case "$prerequisite" in
+		!*)
+			negative_prereq=t
+			prerequisite=${prerequisite#!}
+			;;
+		*)
+			negative_prereq=
+		esac
+
 		case " $lazily_tested_prereq " in
 		*" $prerequisite "*)
 			;;
@@ -294,10 +303,20 @@ test_have_prereq () {
 		total_prereq=$(($total_prereq + 1))
 		case "$satisfied_prereq" in
 		*" $prerequisite "*)
+			satisfied_this_prereq=t
+			;;
+		*)
+			satisfied_this_prereq=
+		esac
+
+		case "$satisfied_this_prereq,$negative_prereq" in
+		t,|,t)
 			ok_prereq=$(($ok_prereq + 1))
 			;;
 		*)
-			# Keep a list of missing prerequisites
+			# Keep a list of missing prerequisites; restore
+			# the negative marker if necessary.
+			prerequisite=${negative_prereq:+!}$prerequisite
 			if test -z "$missing_prereq"
 			then
 				missing_prereq=$prerequisite
-- 
1.8.0.207.gdf2154c
Jeff King· Nov 28, 2012, 18:26 UTC · re: Jeff King · lore

[PATCH 2/5] t7502: factor out autoident prerequisite

t7502 checks the behavior of commit when we can and cannot determine a valid committer ident. Let's move that into test-lib as a lazy prerequisite so other scripts can use it.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7502-commit.sh | 12 +-----------
 t/test-lib.sh     |  6 ++++++
 2 files changed, 7 insertions(+), 11 deletions(-)
Show changes to 2 files +7 −11

t/t7502-commit.sh, t/test-lib.sh

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index deb187e..1a5cb69 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -243,16 +243,6 @@ test_expect_success 'message shows author when it is not equal to committer' '
 	  .git/COMMIT_EDITMSG
 '
 
-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
-	else
-		test_set_prereq NOAUTOIDENT
-	fi
-'
-
 test_expect_success AUTOIDENT 'message shows committer when it is automatic' '
 
 	echo >>negative &&
@@ -271,7 +261,7 @@ echo editor started > "$(pwd)/.git/result"
 exit 0
 EOF
 
-test_expect_success NOAUTOIDENT 'do not fire editor when committer is bogus' '
+test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
 	>.git/result
 	>expect &&
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 489bc80..0334a9e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -738,6 +738,12 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
 	esac
 '
 
+test_lazy_prereq AUTOIDENT '
+	sane_unset GIT_AUTHOR_NAME &&
+	sane_unset GIT_AUTHOR_EMAIL &&
+	git var GIT_AUTHOR_IDENT
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY
-- 
1.8.0.207.gdf2154c
Jeff King· Nov 28, 2012, 18:26 UTC · re: Jeff King · lore

[PATCH 3/5] ident: make user_ident_explicitly_given static

In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given" was introduced as a global for communication between config, ident, and builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07) readers switched to using the common wrapper user_ident_sufficiently_given(). After v1.7.11-rc1~15^2~18 (2012-05-21), the var is only written in ident.c.

Now we can make it static, which will enable further refactoring without worrying about upsetting other code.

Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 cache.h | 4 ----
 ident.c | 6 +++++-
 2 files changed, 5 insertions(+), 5 deletions(-)
Show changes to 2 files +5 −5

cache.h, ident.c

diff --git a/cache.h b/cache.h
index dbd8018..50d9eea 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,10 +1149,6 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-#define IDENT_NAME_GIVEN 01
-#define IDENT_MAIL_GIVEN 02
-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-extern int user_ident_explicitly_given;
 extern int user_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
diff --git a/ident.c b/ident.c
index a4bf206..733d69d 100644
--- a/ident.c
+++ b/ident.c
@@ -10,7 +10,11 @@
 static struct strbuf git_default_name = STRBUF_INIT;
 static struct strbuf git_default_email = STRBUF_INIT;
 static char git_default_date[50];
-int user_ident_explicitly_given;
+
+#define IDENT_NAME_GIVEN 01
+#define IDENT_MAIL_GIVEN 02
+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
+static int user_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
-- 
1.8.0.207.gdf2154c
Jeff King· Nov 28, 2012, 18:26 UTC · re: Jeff King · lore

[PATCH 4/5] ident: keep separate "explicit" flags for author and committer

We keep track of whether the user ident was given to us explicitly, or if we guessed at it from system parameters like username and hostname. However, we kept only a single variable. This covers the common cases (because the author and committer will usually come from the same explicit source), but can miss two cases:

  1. GIT_COMMITTER_* is set explicitly, but we fallback for
     GIT_AUTHOR. We claim the ident is explicit, even though
     the author is not.
  2. GIT_AUTHOR_* is set and we ask for author ident, but
     not committer ident. We will claim the ident is
     implicit, even though it is explicit.

This patch uses two variables instead of one, updates both when we set the "fallback" values, and updates them individually when we read from the environment.

Rather than keep user_ident_sufficiently_given as a compatibility wrapper, we update the only two callers to check the committer_ident, which matches their intent and what was happening already.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c |  4 ++--
 cache.h          |  3 ++-
 ident.c          | 32 +++++++++++++++++++++++++-------
 3 files changed, 29 insertions(+), 10 deletions(-)
Show changes to 3 files +29 −10

builtin/commit.c, cache.h, ident.c

diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..d6dd3df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				ident_shown++ ? "" : "\n",
 				author_ident->buf);
 
-		if (!user_ident_sufficiently_given())
+		if (!committer_ident_sufficiently_given())
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 				_("%s"
 				"Committer: %s"),
@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 		strbuf_addstr(&format, "\n Author: ");
 		strbuf_addbuf_percentquote(&format, &author_ident);
 	}
-	if (!user_ident_sufficiently_given()) {
+	if (!committer_ident_sufficiently_given()) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
 		if (advice_implicit_identity) {
diff --git a/cache.h b/cache.h
index 50d9eea..18fdd18 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,7 +1149,8 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-extern int user_ident_sufficiently_given(void);
+extern int committer_ident_sufficiently_given(void);
+extern int author_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
diff --git a/ident.c b/ident.c
index 733d69d..ac9672f 100644
--- a/ident.c
+++ b/ident.c
@@ -14,7 +14,8 @@ static char git_default_date[50];
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-static int user_ident_explicitly_given;
+static int committer_ident_explicitly_given;
+static int author_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
@@ -113,7 +114,8 @@ const char *ident_default_email(void)
 
 		if (email && email[0]) {
 			strbuf_addstr(&git_default_email, email);
-			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else
 			copy_email(xgetpwuid_self(), &git_default_email);
 		strbuf_trim(&git_default_email);
@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)
 
 const char *git_author_info(int flag)
 {
+	if (getenv("GIT_AUTHOR_NAME"))
+		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+	if (getenv("GIT_AUTHOR_EMAIL"))
+		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 			 getenv("GIT_AUTHOR_EMAIL"),
 			 getenv("GIT_AUTHOR_DATE"),
@@ -340,16 +346,16 @@ const char *git_author_info(int flag)
 const char *git_committer_info(int flag)
 {
 	if (getenv("GIT_COMMITTER_NAME"))
-		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_COMMITTER_EMAIL"))
-		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
 			 getenv("GIT_COMMITTER_DATE"),
 			 flag);
 }
 
-int user_ident_sufficiently_given(void)
+static int ident_is_sufficient(int user_ident_explicitly_given)
 {
 #ifndef WINDOWS
 	return (user_ident_explicitly_given & IDENT_MAIL_GIVEN);
@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)
 #endif
 }
 
+int committer_ident_sufficiently_given(void)
+{
+	return ident_is_sufficient(committer_ident_explicitly_given);
+}
+
+int author_ident_sufficiently_given(void)
+{
+	return ident_is_sufficient(author_ident_explicitly_given);
+}
+
 int git_ident_config(const char *var, const char *value, void *data)
 {
 	if (!strcmp(var, "user.name")) {
@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 			return config_error_nonbool(var);
 		strbuf_reset(&git_default_name);
 		strbuf_addstr(&git_default_name, value);
-		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
 
@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 			return config_error_nonbool(var);
 		strbuf_reset(&git_default_email);
 		strbuf_addstr(&git_default_email, value);
-		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
 
-- 
1.8.0.207.gdf2154c
Jeff King· Nov 28, 2012, 18:26 UTC · re: Jeff King · lore

[PATCH 5/5] t: add tests for "git var"

We do not currently have any explicit tests for "git var" at all (though we do exercise it to some degree as a part of other tests). Let's add a few basic sanity checks.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0007-git-var.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100755 t/t0007-git-var.sh
Show changes to t/t0007-git-var.sh +49 −0
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
new file mode 100755
index 0000000..5868a87
--- /dev/null
+++ b/t/t0007-git-var.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+
+test_description='basic sanity checks for git var'
+. ./test-lib.sh
+
+test_expect_success 'get GIT_AUTHOR_IDENT' '
+	test_tick &&
+	echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+	git var GIT_AUTHOR_IDENT >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'get GIT_COMMITTER_IDENT' '
+	test_tick &&
+	echo "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" >expect &&
+	git var GIT_COMMITTER_IDENT >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success !AUTOIDENT 'requested identites are strict' '
+	(
+		sane_unset GIT_COMMITTER_NAME &&
+		sane_unset GIT_COMMITTER_EMAIL &&
+		test_must_fail git var GIT_COMMITTER_IDENT
+	)
+'
+
+# For git var -l, we check only a representative variable;
+# testing the whole output would make our test too brittle with
+# respect to unrelated changes in the test suite's environment.
+test_expect_success 'git var -l lists variables' '
+	git var -l >actual &&
+	echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+	sed -n s/GIT_AUTHOR_IDENT=//p <actual >actual.author &&
+	test_cmp expect actual.author
+'
+
+test_expect_success 'git var -l lists config' '
+	git var -l >actual &&
+	echo false >expect &&
+	sed -n s/core\\.bare=//p <actual >actual.bare &&
+	test_cmp expect actual.bare
+'
+
+test_expect_success 'listing and asking for variables are exclusive' '
+	test_must_fail git var -l GIT_COMMITTER_IDENT
+'
+
+test_done
-- 
1.8.0.207.gdf2154c
Jeff King· Nov 28, 2012, 18:42 UTC · re: Jeff King · lore

[PATCH 6/5] t9001: check send-email behavior with implicit sender

On Wed, Nov 28, 2012 at 01:25:35PM -0500, Jeff King wrote:
Show 7 quoted lines
> Here are the cleanups and refactorings split out from my
> jk/send-email-sender-prompt series. They can go right on master and are
> independent of Felipe's fc/send-email-no-sender-prompt topic.
> [...]
> Dropped were:
> [...]
>   - send-email prompting change; obsoleted by Felipe's patch

Here is one more patch pulling out the extra tests from my final commit. It needs to go on top of the merge of Felipe's series and the one I just sent. The former because of the new prompting behavior, and the latter because of the AUTOIDENT prerequisites.

If it's simpler, my whole series can just go on top of Felipe's patch (or vice versa).

-- >8 --
Subject: [PATCH] t9001: check send-email behavior with implicit sender

We allow send-email to use an implicitly-defined identity for the sender (because there is still a confirmation step), but we abort when we cannot generate such an identity. Let's make sure that we test this.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t9001-send-email.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
Show changes to t/t9001-send-email.sh +28 −0
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index c5d66cf..27edfa8 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,34 @@ test_expect_success $PREREQ 'Prompting works' '
 		grep "^To: to@example.com\$" msgtxt1
 '
 
+test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
+       clean_fake_sendmail &&
+       (sane_unset GIT_AUTHOR_NAME &&
+	sane_unset GIT_AUTHOR_EMAIL &&
+	sane_unset GIT_COMMITTER_NAME &&
+	sane_unset GIT_COMMITTER_EMAIL &&
+	GIT_SEND_EMAIL_NOTTY=1 git send-email \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--to=to@example.com \
+		$patches \
+		</dev/null 2>errors
+       )
+'
+
+test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
+       clean_fake_sendmail &&
+       (sane_unset GIT_AUTHOR_NAME &&
+	sane_unset GIT_AUTHOR_EMAIL &&
+	sane_unset GIT_COMMITTER_NAME &&
+	sane_unset GIT_COMMITTER_EMAIL &&
+	GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
+	test_must_fail git send-email \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$patches </dev/null 2>errors &&
+	test_i18ngrep "tell me who you are" errors
+       )
+'
+
 test_expect_success $PREREQ 'tocmd works' '
 	clean_fake_sendmail &&
 	cp $patches tocmd.patch &&
-- 
1.8.0.207.gdf2154c
Junio C Hamano· Nov 28, 2012, 18:55 UTC · re: Jeff King · lore

Re: [PATCH 6/5] t9001: check send-email behavior with implicit sender

Jeff King <peff@peff.net> writes:
Show 66 quoted lines
> On Wed, Nov 28, 2012 at 01:25:35PM -0500, Jeff King wrote:
>
>> Here are the cleanups and refactorings split out from my
>> jk/send-email-sender-prompt series. They can go right on master and are
>> independent of Felipe's fc/send-email-no-sender-prompt topic.
>> [...]
>> Dropped were:
>> [...]
>>   - send-email prompting change; obsoleted by Felipe's patch
>
> Here is one more patch pulling out the extra tests from my final commit.
> It needs to go on top of the merge of Felipe's series and the one I just
> sent. The former because of the new prompting behavior, and the latter
> because of the AUTOIDENT prerequisites.
>
> If it's simpler, my whole series can just go on top of Felipe's patch
> (or vice versa).
>
> -- >8 --
> Subject: [PATCH] t9001: check send-email behavior with implicit sender
>
> We allow send-email to use an implicitly-defined identity
> for the sender (because there is still a confirmation step),
> but we abort when we cannot generate such an identity. Let's
> make sure that we test this.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t9001-send-email.sh | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index c5d66cf..27edfa8 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -201,6 +201,34 @@ test_expect_success $PREREQ 'Prompting works' '
>  		grep "^To: to@example.com\$" msgtxt1
>  '
>  
> +test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
> +       clean_fake_sendmail &&
> +       (sane_unset GIT_AUTHOR_NAME &&
> +	sane_unset GIT_AUTHOR_EMAIL &&
> +	sane_unset GIT_COMMITTER_NAME &&
> +	sane_unset GIT_COMMITTER_EMAIL &&
> +	GIT_SEND_EMAIL_NOTTY=1 git send-email \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		--to=to@example.com \
> +		$patches \
> +		</dev/null 2>errors
> +       )
> +'
> +
> +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
> +       clean_fake_sendmail &&
> +       (sane_unset GIT_AUTHOR_NAME &&
> +	sane_unset GIT_AUTHOR_EMAIL &&
> +	sane_unset GIT_COMMITTER_NAME &&
> +	sane_unset GIT_COMMITTER_EMAIL &&
> +	GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
> +	test_must_fail git send-email \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		$patches </dev/null 2>errors &&
> +	test_i18ngrep "tell me who you are" errors
> +       )
> +'

The difference between these two tests should solely come from AUTOIDENT and nothing else, so it would be better to see their command line arguments to match; the former is with --to and the latter is without in this patch but I do not think you meant them to differ that way.

>  test_expect_success $PREREQ 'tocmd works' '
>  	clean_fake_sendmail &&
>  	cp $patches tocmd.patch &&
Jeff King· Nov 28, 2012, 20:06 UTC · re: Junio C Hamano · lore

Re: [PATCH 6/5] t9001: check send-email behavior with implicit sender

On Wed, Nov 28, 2012 at 10:55:02AM -0800, Junio C Hamano wrote:
Show 33 quoted lines
> > +test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
> > +       clean_fake_sendmail &&
> > +       (sane_unset GIT_AUTHOR_NAME &&
> > +	sane_unset GIT_AUTHOR_EMAIL &&
> > +	sane_unset GIT_COMMITTER_NAME &&
> > +	sane_unset GIT_COMMITTER_EMAIL &&
> > +	GIT_SEND_EMAIL_NOTTY=1 git send-email \
> > +		--smtp-server="$(pwd)/fake.sendmail" \
> > +		--to=to@example.com \
> > +		$patches \
> > +		</dev/null 2>errors
> > +       )
> > +'
> > +
> > +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
> > +       clean_fake_sendmail &&
> > +       (sane_unset GIT_AUTHOR_NAME &&
> > +	sane_unset GIT_AUTHOR_EMAIL &&
> > +	sane_unset GIT_COMMITTER_NAME &&
> > +	sane_unset GIT_COMMITTER_EMAIL &&
> > +	GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
> > +	test_must_fail git send-email \
> > +		--smtp-server="$(pwd)/fake.sendmail" \
> > +		$patches </dev/null 2>errors &&
> > +	test_i18ngrep "tell me who you are" errors
> > +       )
> > +'
> 
> The difference between these two tests should solely come from
> AUTOIDENT and nothing else, so it would be better to see their
> command line arguments to match; the former is with --to and the
> latter is without in this patch but I do not think you meant them to
> differ that way.

Yeah, that makes sense. The top one originally was testing that we still prompted in the autoident case, and so had some echos piped into send-email. I simplified it to use --to, but I agree it is even better to show that the commands are identical.

Here's a cleaned up version that makes it more obvious the commands are the same (it also fixes a few minor whitespace problems on the indentation, which you can see from the quoting above).

-- >8 --
Subject: [PATCH] t9001: check send-email behavior with implicit sender

We allow send-email to use an implicitly-defined identity for the sender (because there is still a confirmation step), but we abort when we cannot generate such an identity. Let's make sure that we test this.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t9001-send-email.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
Show changes to t/t9001-send-email.sh +28 −0
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index c5d66cf..97d6f4c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,34 @@ test_expect_success $PREREQ 'Prompting works' '
 		grep "^To: to@example.com\$" msgtxt1
 '
 
+test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
+	clean_fake_sendmail &&
+	(sane_unset GIT_AUTHOR_NAME &&
+	sane_unset GIT_AUTHOR_EMAIL &&
+	sane_unset GIT_COMMITTER_NAME &&
+	sane_unset GIT_COMMITTER_EMAIL &&
+	GIT_SEND_EMAIL_NOTTY=1 git send-email \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--to=to@example.com \
+		$patches </dev/null 2>errors
+	)
+'
+
+test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
+	clean_fake_sendmail &&
+	(sane_unset GIT_AUTHOR_NAME &&
+	sane_unset GIT_AUTHOR_EMAIL &&
+	sane_unset GIT_COMMITTER_NAME &&
+	sane_unset GIT_COMMITTER_EMAIL &&
+	GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
+	test_must_fail git send-email \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--to=to@example.com \
+		$patches </dev/null 2>errors &&
+	test_i18ngrep "tell me who you are" errors
+	)
+'
+
 test_expect_success $PREREQ 'tocmd works' '
 	clean_fake_sendmail &&
 	cp $patches tocmd.patch &&
-- 
1.8.0.207.gdf2154c
Jeff King· Nov 28, 2012, 20:17 UTC · re: Jeff King · lore

Re: [PATCH 6/5] t9001: check send-email behavior with implicit sender

On Wed, Nov 28, 2012 at 03:06:26PM -0500, Jeff King wrote:
> Here's a cleaned up version that makes it more obvious the commands are
> the same (it also fixes a few minor whitespace problems on the
> indentation, which you can see from the quoting above).

I wondered how painful it would be to actually run the command and then conditionally check the results inside the test. I ended up with this:

Show changes to t/t9001-send-email.sh +24 −1
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index c5d66cf..9c8fac1 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,30 @@ test_expect_success $PREREQ 'Prompting works' '
 		grep "^To: to@example.com\$" msgtxt1
 '
 
+test_expect_success $PREREQ 'handle implicit ident' '
+	clean_fake_sendmail &&
+	(
+		sane_unset GIT_AUTHOR_NAME &&
+		sane_unset GIT_AUTHOR_EMAIL &&
+		sane_unset GIT_COMMITTER_NAME &&
+		sane_unset GIT_COMMITTER_EMAIL &&
+		GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
+		{
+			git send-email \
+				--smtp-server="$(pwd)/fake.sendmail" \
+				--to=to@example.com \
+				$patches </dev/null 2>errors;
+			exit_code=$?
+		} &&
+		if test_have_prereq AUTOIDENT; then
+			test $exit_code = 0
+		else
+			test $exit_code != 0 &&
+			test_i18ngrep "tell me who you are" errors
+		fi
+	)
+'
+
 test_expect_success $PREREQ 'tocmd works' '
 	clean_fake_sendmail &&
 	cp $patches tocmd.patch &&

which does work, though it is less nice that the protections and nice
syntax of "test_must_fail" must be abandoned (unless we want to do
something horrible like 'test_must_fail sh -c "exit $exit_code"'.
I could go either way.

-Peff
Junio C Hamano· Nov 28, 2012, 20:42 UTC · re: Jeff King · lore

Re: [PATCH 6/5] t9001: check send-email behavior with implicit sender

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> On Wed, Nov 28, 2012 at 03:06:26PM -0500, Jeff King wrote:
>
>> Here's a cleaned up version that makes it more obvious the commands are
>> the same (it also fixes a few minor whitespace problems on the
>> indentation, which you can see from the quoting above).
>
> I wondered how painful it would be to actually run the command and then
> conditionally check the results inside the test. I ended up with this:
> ...
> which does work, though it is less nice that the protections and nice
> syntax of "test_must_fail" must be abandoned (unless we want to do
> something horrible like 'test_must_fail sh -c "exit $exit_code"'.
> I could go either way.

A change along that line did cross my mind, and I agree that it does clarify that we are testing the same command produces an expected result, the expectation may be different depending on external conditions, and we can figure out what the expected values are before running the test. I do not think we use such a pattern in very many places, though.

The differences in "expected results" are generally not limited to the final outcome in $? (e.g. your "else" side not just checks $? indicates an error, but makes sure that we got an expected error message), which means that the if statement at the end has to be there in some form anyway, which in turn means that a new test helper function to abstract this pattern out further would not buy us very much (either it would be too complex and bug prone to implement, or it would be too simplistic and limited).

I'd prefer to leave these as two separate tests for now like your previous patch. People interested in consolidating/simplifying can first audit the existing tests to find other instances of this pattern to see if it is worth doing, and then come up with an abstraction to be shared among them.

Thanks.
Felipe Contreras· Nov 28, 2012, 21:37 UTC · re: Jeff King · lore

Re: [PATCH 0/5] jk/send-email-sender-prompt loose ends

On Wed, Nov 28, 2012 at 7:25 PM, Jeff King <peff@peff.net> wrote:
Show 39 quoted lines
> Here are the cleanups and refactorings split out from my
> jk/send-email-sender-prompt series. They can go right on master and are
> independent of Felipe's fc/send-email-no-sender-prompt topic.
>
>   [1/5]: test-lib: allow negation of prerequisites
>
> Same as before. I think this is a useful feature for the test suite (and
> 2/5 depends on it).
>
>   [2/5]: t7502: factor out autoident prerequisite
>
> Same as before. Patch 5/5 depends on it.
>
>   [3/5]: ident: make user_ident_explicitly_given static
>
> Same as before. Cleanup.
>
>   [4/5]: ident: keep separate "explicit" flags for author and committer
>
> Same as before. Cleanup. I do not know if anybody will ever care about
> the corner cases it fixes, so it is really just being defensive for
> future code.
>
>   [5/5]: t: add tests for "git var"
>
> Tests split out from he "git var can take multiple values" patch.
>
> Dropped were:
>
>   - "git var" can take multiple values. Nobody really cares about doing
>     so, and it's an external interface, so we'd have to support it
>     forever.
>
>   - exporting "explicit ident" flag via "git var"; same reasoning as
>     above
>
>   - Git.pm supporting explicit ident; ditto
>
>   - send-email prompting change; obsoleted by Felipe's patch

For what it's worth; they look good to me. However, I think it's worth mentioning that there are no functional changes.

-- 
Felipe Contreras

← back to recent threads