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

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

[Jeff King]

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

[PATCH 3/5] ident: make user_ident_explicitly_given static

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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

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

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

[Jeff King]

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
+       )
+'
+
 test_expect_success $PREREQ 'tocmd works' '
 	clean_fake_sendmail &&
 	cp $patches tocmd.patch &&
-- 
1.8.0.207.gdf2154c

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 &&

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

[Jeff King]

On Wed, Nov 28, 2012 at 10:55:02AM -0800, Junio C Hamano wrote:

> > +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(+)

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

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

[Jeff King]

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:

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Felipe Contreras]

On Wed, Nov 28, 2012 at 7:25 PM, Jeff King <peff@peff.net> 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.
>
>   [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