Re: [PATCH v6 2/2] config: allow giving separate author and committer idents

[Junio C Hamano <gitster@pobox.com>]

William Hubbs <williamh@gentoo.org> writes:

> I attempted to save your patches to apply them, but didn't have any luck

I'll push a topic branch (not merged to any of the integration
branches) ab/author-committer-ident-config later today at the
https://github.com/gitster/git repository.

> Also, according to Junio's report, my patch is already merged to next,

In an earlier "What's cooking" I may have said that I plan to merge
it to 'next', but I think the plan is now to leave it there for now
until this discussion settles and the latest report should reflect
that.

I just double-checked and wh/author-committer-ident-config is not in
'next'.  Whew.

Here is a diff that turns wh/author-committer-ident-config into what
ab/author-committer-ident-config has.  There are some formatting
changes, all of which I agree with, a bogus set_ident() refactoring
that should not be used, in addition to some test changes.

 Documentation/git-commit-tree.txt |  3 +-
 builtin/am.c                      |  2 +-
 builtin/commit.c                  |  2 +-
 cache.h                           |  4 +-
 ident.c                           | 95 +++++++++++++--------------------------
 sequencer.c                       |  3 +-
 t/t7517-per-repo-email.sh         | 88 +++++++++++++++++++++++++-----------
 7 files changed, 101 insertions(+), 96 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index 002dae625e..091e3a77ca 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -88,7 +88,8 @@ if set:
 (nb "<", ">" and "\n"s are stripped)
 
 In case (some of) these environment variables are not set, the information
-is taken from the configuration items user.name and user.email, or, if not
+is taken from the configuration items user.name and user.email, or the more
+specific author.{name,email} and committer.{name,email} variables, or, if not
 present, the environment variable EMAIL, or, if that is not set,
 system user name and the hostname used for outgoing mail (taken
 from `/etc/mailname` and falling back to the fully qualified hostname when
diff --git a/builtin/am.c b/builtin/am.c
index 3727d4d267..d4a1cbe828 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1594,7 +1594,7 @@ static void do_commit(const struct am_state *state)
 	}
 
 	author = fmt_ident(state->author_name, state->author_email,
-		WANT_AUTHOR_IDENT,
+			WANT_AUTHOR_IDENT,
 			state->ignore_date ? NULL : state->author_date,
 			IDENT_STRICT);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index f96b90daeb..a7879d65d1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -608,7 +608,7 @@ static void determine_author_info(struct strbuf *author_ident)
 	}
 
 	strbuf_addstr(author_ident, fmt_ident(name, email, WANT_AUTHOR_IDENT, date,
-				IDENT_STRICT));
+					      IDENT_STRICT));
 	assert_split_ident(&author, author_ident);
 	export_one("GIT_AUTHOR_NAME", author.name_begin, author.name_end, 0);
 	export_one("GIT_AUTHOR_EMAIL", author.mail_begin, author.mail_end, 0);
diff --git a/cache.h b/cache.h
index bb78eb9a3a..ca6ba1e423 100644
--- a/cache.h
+++ b/cache.h
@@ -1489,8 +1489,8 @@ enum want_ident {
 extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email,
-		enum want_ident whose_ident,
-		const char *date_str, int);
+			     enum want_ident whose_ident,
+			     const char *date_str, int flag);
 extern const char *fmt_name(enum want_ident);
 extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
diff --git a/ident.c b/ident.c
index 9c2eb0a2d0..7c3be81ee1 100644
--- a/ident.c
+++ b/ident.c
@@ -359,7 +359,8 @@ N_("\n"
    "\n");
 
 const char *fmt_ident(const char *name, const char *email,
-		      enum want_ident whose_ident, const char *date_str, int flag)
+		      enum want_ident whose_ident, const char *date_str,
+		      int flag)
 {
 	static struct strbuf ident = STRBUF_INIT;
 	int strict = (flag & IDENT_STRICT);
@@ -508,70 +509,38 @@ int author_ident_sufficiently_given(void)
 	return ident_is_sufficient(author_ident_explicitly_given);
 }
 
-static int set_ident(const char *var, const char *value)
+static int set_ident_internal(const char *var, const char *value,
+			    struct strbuf *sb, const int flag)
 {
-	if (!strcmp(var, "author.name")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strbuf_reset(&git_author_name);
-		strbuf_addstr(&git_author_name, value);
-		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
-		ident_config_given |= IDENT_NAME_GIVEN;
-		return 0;
-	}
-
-	if (!strcmp(var, "author.email")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strbuf_reset(&git_author_email);
-		strbuf_addstr(&git_author_email, value);
-		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		ident_config_given |= IDENT_MAIL_GIVEN;
-		return 0;
-	}
-
-	if (!strcmp(var, "committer.name")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strbuf_reset(&git_committer_name);
-		strbuf_addstr(&git_committer_name, value);
-		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
-		ident_config_given |= IDENT_NAME_GIVEN;
-		return 0;
-	}
-
-	if (!strcmp(var, "committer.email")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strbuf_reset(&git_committer_email);
-		strbuf_addstr(&git_committer_email, value);
-		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		ident_config_given |= IDENT_MAIL_GIVEN;
-		return 0;
-	}
-
-	if (!strcmp(var, "user.name")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strbuf_reset(&git_default_name);
-		strbuf_addstr(&git_default_name, value);
-		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
-		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
-		ident_config_given |= IDENT_NAME_GIVEN;
-		return 0;
-	}
-
-	if (!strcmp(var, "user.email")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strbuf_reset(&git_default_email);
-		strbuf_addstr(&git_default_email, value);
-		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		ident_config_given |= IDENT_MAIL_GIVEN;
-		return 0;
-	}
+	if (!value)
+		return config_error_nonbool(var);
+	strbuf_reset(sb);
+	strbuf_addstr(sb, value);
+	author_ident_explicitly_given |= flag;
+	ident_config_given |= flag;
+	return 0;
+}
 
+static int set_ident(const char *var, const char *value)
+{
+	if (!strcmp(var, "author.name"))
+		return set_ident_internal(var, value, &git_author_name,
+					  IDENT_NAME_GIVEN);
+	else if (!strcmp(var, "author.email"))
+		return set_ident_internal(var, value, &git_author_email,
+					  IDENT_MAIL_GIVEN);
+	else if (!strcmp(var, "committer.name"))
+		return set_ident_internal(var, value, &git_committer_name,
+					  IDENT_NAME_GIVEN);
+	else if (!strcmp(var, "committer.email"))
+		return set_ident_internal(var, value, &git_committer_email,
+					  IDENT_MAIL_GIVEN);
+	else if (!strcmp(var, "user.name"))
+		return set_ident_internal(var, value, &git_default_name,
+					  IDENT_NAME_GIVEN);
+	else if (!strcmp(var, "user.email"))
+		return set_ident_internal(var, value, &git_default_email,
+					  IDENT_MAIL_GIVEN);
 	return 0;
 }
 
diff --git a/sequencer.c b/sequencer.c
index 3505d52bb9..98ba2106f6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -836,7 +836,8 @@ static const char *read_author_ident(struct strbuf *buf)
 	}
 
 	strbuf_reset(&out);
-	strbuf_addstr(&out, fmt_ident(name, email, WANT_AUTHOR_IDENT, date, 0));
+	strbuf_addstr(&out, fmt_ident(name, email, WANT_AUTHOR_IDENT, date,
+				      0));
 	strbuf_swap(buf, &out);
 	strbuf_release(&out);
 	free(name);
diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
index b2401cec3e..e0182779ed 100755
--- a/t/t7517-per-repo-email.sh
+++ b/t/t7517-per-repo-email.sh
@@ -85,15 +85,49 @@ test_expect_success REBASE_P \
 	test_must_fail git rebase -p master
 '
 
+test_expect_success 'fallbacks for GIT_* and {user,author,committer}.{name,email}' '
+	# We must have committer in the object
+	test_must_fail test_env \
+		GIT_AUTHOR_NAME=author.name \
+		GIT_AUTHOR_EMAIL=author@email \
+		GIT_COMMITTER_NAME= \
+		GIT_COMMITTER_EMAIL= \
+		test_commit A 2>stderr &&
+	test_i18ngrep "empty ident name.*not allowed" stderr &&
+
+	# With no committer E-Mail we will have an empty field
+	test_env \
+		GIT_AUTHOR_NAME=author.name \
+		GIT_AUTHOR_EMAIL=author@email \
+		GIT_COMMITTER_NAME=committer.name \
+		GIT_COMMITTER_EMAIL= \
+		test_commit B 2>stderr &&
+	echo "author.name author@email committer.name " >expected &&
+	git log --format="%an %ae %cn %ce" -1 >actual &&
+	test_cmp expected actual &&
+
+	# Environment overrides config
+	test_config user.name author.config.name &&
+	test_env \
+		GIT_AUTHOR_NAME=author.name \
+		GIT_AUTHOR_EMAIL=author@email \
+		GIT_COMMITTER_NAME=committer.name \
+		GIT_COMMITTER_EMAIL= \
+		test_commit C 2>stderr &&
+	echo "author.name author@email committer.name " >expected &&
+	git log --format="%an %ae %cn %ce" -1 >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'author.name overrides user.name' '
 	test_config user.name user &&
 	test_config user.email user@example.com &&
 	test_config author.name author &&
 	test_commit author-name-override-user &&
-	echo author user@example.com > expected-author &&
-	echo user user@example.com > expected-committer &&
-	git log --format="%an %ae" -1 > actual-author &&
-	git log --format="%cn %ce" -1 > actual-committer &&
+	echo author user@example.com >expected-author &&
+	echo user user@example.com >expected-committer &&
+	git log --format="%an %ae" -1 >actual-author &&
+	git log --format="%cn %ce" -1 >actual-committer &&
 	test_cmp expected-author actual-author &&
 	test_cmp expected-committer actual-committer
 '
@@ -103,10 +137,10 @@ test_expect_success 'author.email overrides user.email' '
 	test_config user.email user@example.com &&
 	test_config author.email author@example.com &&
 	test_commit author-email-override-user &&
-	echo user author@example.com > expected-author &&
-	echo user user@example.com > expected-committer &&
-	git log --format="%an %ae" -1 > actual-author &&
-	git log --format="%cn %ce" -1 > actual-committer &&
+	echo user author@example.com >expected-author &&
+	echo user user@example.com >expected-committer &&
+	git log --format="%an %ae" -1 >actual-author &&
+	git log --format="%cn %ce" -1 >actual-committer &&
 	test_cmp expected-author actual-author &&
 	test_cmp expected-committer actual-committer
 '
@@ -116,10 +150,10 @@ test_expect_success 'committer.name overrides user.name' '
 	test_config user.email user@example.com &&
 	test_config committer.name committer &&
 	test_commit committer-name-override-user &&
-	echo user user@example.com > expected-author &&
-	echo committer user@example.com > expected-committer &&
-	git log --format="%an %ae" -1 > actual-author &&
-	git log --format="%cn %ce" -1 > actual-committer &&
+	echo user user@example.com >expected-author &&
+	echo committer user@example.com >expected-committer &&
+	git log --format="%an %ae" -1 >actual-author &&
+	git log --format="%cn %ce" -1 >actual-committer &&
 	test_cmp expected-author actual-author &&
 	test_cmp expected-committer actual-committer
 '
@@ -129,10 +163,10 @@ test_expect_success 'committer.email overrides user.email' '
 	test_config user.email user@example.com &&
 	test_config committer.email committer@example.com &&
 	test_commit committer-email-override-user &&
-	echo user user@example.com > expected-author &&
-	echo user committer@example.com > expected-committer &&
-	git log --format="%an %ae" -1 > actual-author &&
-	git log --format="%cn %ce" -1 > actual-committer &&
+	echo user user@example.com >expected-author &&
+	echo user committer@example.com >expected-committer &&
+	git log --format="%an %ae" -1 >actual-author &&
+	git log --format="%cn %ce" -1 >actual-committer &&
 	test_cmp expected-author actual-author &&
 	test_cmp expected-committer actual-committer
 '
@@ -144,17 +178,17 @@ test_expect_success 'author and committer environment variables override config
 	test_config author.email author@example.com &&
 	test_config committer.name committer &&
 	test_config committer.email committer@example.com &&
-	GIT_AUTHOR_NAME=env_author && export GIT_AUTHOR_NAME &&
-	GIT_AUTHOR_EMAIL=env_author@example.com && export GIT_AUTHOR_EMAIL &&
-	GIT_COMMITTER_NAME=env_commit && export GIT_COMMITTER_NAME &&
-	GIT_COMMITTER_EMAIL=env_commit@example.com && export GIT_COMMITTER_EMAIL &&
-	test_commit env-override-conf &&
-	echo env_author env_author@example.com > expected-author &&
-	echo env_commit env_commit@example.com > expected-committer &&
-	git log --format="%an %ae" -1 > actual-author &&
-	git log --format="%cn %ce" -1 > actual-committer &&
-	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
-	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
+
+	test_env \
+		GIT_AUTHOR_NAME=env_author \
+		GIT_AUTHOR_EMAIL=env_author@example.com \
+		GIT_COMMITTER_NAME=env_commit \
+		GIT_COMMITTER_EMAIL=env_commit@example.com \
+		test_commit env-override-conf &&
+	echo env_author env_author@example.com >expected-author &&
+	echo env_commit env_commit@example.com >expected-committer &&
+	git log --format="%an %ae" -1 >actual-author &&
+	git log --format="%cn %ce" -1 >actual-committer &&
 	test_cmp expected-author actual-author &&
 	test_cmp expected-committer actual-committer
 '