Add author and committer configuration settings

Add author and committer configuration settings

[William Hubbs]

All,

this is a re-roll of my previous patch to add separate author and
committer settings.:s/committer/committer configuration/
A

I attempted to encorporate everything from the last iteration, and all
tests still pass.

This applies to master. Please review.

[PATCH v2 1/2] config: allow giving separate author and committer
[PATCH v2 2/2] tests: add test for separate author and committer

Thanks,

William

[PATCH v2 1/2] config: allow giving separate author and committer idents

[William Hubbs]

The author.email, author.name, committer.email and committer.name
settings are analogous to the GIT_AUTHOR_* and GIT_COMMITTER_*
environment variables, but for the git config system. This allows them
to be set separately for each repository.

Git supports setting different authorship and committer
information with environment variables. However, environment variables
are set in the shell, so if different authorship and committer
information is needed for different repositories an external tool is
required.

This adds support to git config for author.email, author.name,
committer.email and committer.name  settings so this information
can be set per repository.

Also, it generalizes the fmt_ident function so it can handle author vs
committer identification.

Signed-off-by: William Hubbs <williamh@gentoo.org>
---
 Documentation/config/user.txt | 31 +++++++++++++-
 blame.c                       |  3 +-
 builtin/am.c                  |  2 +-
 builtin/commit.c              |  3 +-
 cache.h                       | 13 +++++-
 config.c                      |  6 +++
 ident.c                       | 81 +++++++++++++++++++++++++++++++++--
 log-tree.c                    |  3 +-
 sequencer.c                   |  5 +--
 9 files changed, 132 insertions(+), 15 deletions(-)

diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index b5b2ba1199..18e1ec3c1b 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -1,12 +1,39 @@
+author.email::
+	The email address used for the author of newly
+	created commits.  Defaults to the value of the
+	`GIT_AUTHOR_EMAIL` environment variable, or if
+	the environment variable is not set, the `user.email`
+	configuration variable.
+
+author.name::
+	The full name used for the author of newly created commits.
+	Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or
+	if the environment variable is not set,
+	the `user.email` configuration variable.
+
+committer.email::
+	The email address used for the committer of newly created commits.
+	Defaults to the value of the `GIT_COMMITTER_EMAIL` environment
+	variable, or if the environment variable is not set, the `user.email`
+	configuration variable.
+
+committer.name::
+	The full name used for the committer of newly created commits.
+	Defaults to the value of the `GIT_COMMITTER_NAME` environment
+	variable, or if the environment variable is not set, the `user.name`
+	configuration variable.
+
 user.email::
 	Your email address to be recorded in any newly created commits.
 	Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and
-	`EMAIL` environment variables.  See linkgit:git-commit-tree[1].
+	`EMAIL` environment variables or the `author.email` or
+	`committer.email` settings discussed above. See linkgit:git-commit-tree[1].
 
 user.name::
 	Your full name to be recorded in any newly created commits.
 	Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME`
-	environment variables.  See linkgit:git-commit-tree[1].
+	environment variables or the `author.name` or `committer.name`
+	settings discussed above. See linkgit:git-commit-tree[1].
 
 user.useConfigOnly::
 	Instruct Git to avoid trying to guess defaults for `user.email`
diff --git a/blame.c b/blame.c
index 43861437f7..c9c351eb36 100644
--- a/blame.c
+++ b/blame.c
@@ -204,7 +204,8 @@ static struct commit *fake_working_tree_commit(struct repository *r,
 
 	origin = make_origin(commit, path);
 
-	ident = fmt_ident("Not Committed Yet", "not.committed.yet", NULL, 0);
+	ident = fmt_ident("Not Committed Yet", "not.committed.yet",
+			WANT_BLANK_IDENT, NULL, 0);
 	strbuf_addstr(&msg, "tree 0000000000000000000000000000000000000000\n");
 	for (parent = commit->parents; parent; parent = parent->next)
 		strbuf_addf(&msg, "parent %s\n",
diff --git a/builtin/am.c b/builtin/am.c
index 95370313b6..53fdd22c45 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,
-			state->ignore_date ? NULL : state->author_date,
+			WANT_AUTHOR_IDENT, state->ignore_date ? NULL : state->author_date,
 			IDENT_STRICT);
 
 	if (state->committer_date_is_author_date)
diff --git a/builtin/commit.c b/builtin/commit.c
index 004b816635..f96b90daeb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -607,7 +607,8 @@ static void determine_author_info(struct strbuf *author_ident)
 		set_ident_var(&date, strbuf_detach(&date_buf, NULL));
 	}
 
-	strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));
+	strbuf_addstr(author_ident, fmt_ident(name, email, WANT_AUTHOR_IDENT, date,
+				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 49713cc5a5..f13c6e244f 100644
--- a/cache.h
+++ b/cache.h
@@ -1479,10 +1479,19 @@ int date_overflows(timestamp_t date);
 #define IDENT_STRICT	       1
 #define IDENT_NO_DATE	       2
 #define IDENT_NO_NAME	       4
+
+enum want_ident {
+	WANT_BLANK_IDENT,
+	WANT_AUTHOR_IDENT,
+	WANT_COMMITTER_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, const char *date_str, int);
-extern const char *fmt_name(const char *name, const char *email);
+extern const char *fmt_ident(const char *name, const char *email,
+		enum want_ident whose_ident,
+		const char *date_str, int);
+extern const char *fmt_name(enum want_ident);
 extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
diff --git a/config.c b/config.c
index ff521eb27a..4bd5920dea 100644
--- a/config.c
+++ b/config.c
@@ -1484,6 +1484,12 @@ int git_default_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (starts_with(var, "author."))
+		return git_ident_config(var, value, cb);
+
+	if (starts_with(var, "committer."))
+		return git_ident_config(var, value, cb);
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/ident.c b/ident.c
index 33bcf40644..a1e774c3e5 100644
--- a/ident.c
+++ b/ident.c
@@ -11,6 +11,10 @@
 static struct strbuf git_default_name = STRBUF_INIT;
 static struct strbuf git_default_email = STRBUF_INIT;
 static struct strbuf git_default_date = STRBUF_INIT;
+static struct strbuf git_author_name = STRBUF_INIT;
+static struct strbuf git_author_email = STRBUF_INIT;
+static struct strbuf git_committer_name = STRBUF_INIT;
+static struct strbuf git_committer_email = STRBUF_INIT;
 static int default_email_is_bogus;
 static int default_name_is_bogus;
 
@@ -355,13 +359,19 @@ N_("\n"
    "\n");
 
 const char *fmt_ident(const char *name, const char *email,
-		      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);
 	int want_date = !(flag & IDENT_NO_DATE);
 	int want_name = !(flag & IDENT_NO_NAME);
 
+	if (!email) {
+		if (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)
+			email = git_author_email.buf;
+		else if (whose_ident == WANT_COMMITTER_IDENT && git_committer_email.len)
+			email = git_committer_email.buf;
+	}
 	if (!email) {
 		if (strict && ident_use_config_only
 		    && !(ident_config_given & IDENT_MAIL_GIVEN)) {
@@ -377,6 +387,13 @@ const char *fmt_ident(const char *name, const char *email,
 
 	if (want_name) {
 		int using_default = 0;
+		if (!name) {
+			if (whose_ident == WANT_AUTHOR_IDENT && git_author_name.len)
+				name = git_author_name.buf;
+			else if (whose_ident == WANT_COMMITTER_IDENT &&
+					git_committer_name.len)
+				name = git_committer_name.buf;
+		}
 		if (!name) {
 			if (strict && ident_use_config_only
 			    && !(ident_config_given & IDENT_NAME_GIVEN)) {
@@ -425,9 +442,25 @@ const char *fmt_ident(const char *name, const char *email,
 	return ident.buf;
 }
 
-const char *fmt_name(const char *name, const char *email)
+const char *fmt_name(enum want_ident whose_ident)
 {
-	return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE);
+	char *name = NULL;
+	char *email = NULL;
+
+	switch (whose_ident) {
+	case WANT_BLANK_IDENT:
+		break;
+	case WANT_AUTHOR_IDENT:
+		name = getenv("GIT_AUTHOR_NAME");
+		email = getenv("GIT_AUTHOR_EMAIL");
+		break;
+	case WANT_COMMITTER_IDENT:
+		name = getenv("GIT_COMMITTER_NAME");
+		email = getenv("GIT_COMMITTER_EMAIL");
+		break;
+	}
+	return fmt_ident(name, email, whose_ident, NULL,
+			IDENT_STRICT | IDENT_NO_DATE);
 }
 
 const char *git_author_info(int flag)
@@ -438,6 +471,7 @@ const char *git_author_info(int flag)
 		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 			 getenv("GIT_AUTHOR_EMAIL"),
+			 WANT_AUTHOR_IDENT,
 			 getenv("GIT_AUTHOR_DATE"),
 			 flag);
 }
@@ -450,6 +484,7 @@ const char *git_committer_info(int flag)
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
+			 WANT_COMMITTER_IDENT,
 			 getenv("GIT_COMMITTER_DATE"),
 			 flag);
 }
@@ -480,6 +515,46 @@ int git_ident_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 
+	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);
diff --git a/log-tree.c b/log-tree.c
index 10680c139e..43ef4f4300 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -687,8 +687,7 @@ void show_log(struct rev_info *opt)
 	 */
 	if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
 		ctx.need_8bit_cte =
-			has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
-					       getenv("GIT_COMMITTER_EMAIL")));
+			has_non_ascii(fmt_name(WANT_COMMITTER_IDENT));
 	ctx.date_mode = opt->date_mode;
 	ctx.date_mode_explicit = opt->date_mode_explicit;
 	ctx.abbrev = opt->diffopt.abbrev;
diff --git a/sequencer.c b/sequencer.c
index f5370f4965..3505d52bb9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -836,7 +836,7 @@ static const char *read_author_ident(struct strbuf *buf)
 	}
 
 	strbuf_reset(&out);
-	strbuf_addstr(&out, fmt_ident(name, email, date, 0));
+	strbuf_addstr(&out, fmt_ident(name, email, WANT_AUTHOR_IDENT, date, 0));
 	strbuf_swap(buf, &out);
 	strbuf_release(&out);
 	free(name);
@@ -4087,8 +4087,7 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
 	int has_footer;
 
 	strbuf_addstr(&sob, sign_off_header);
-	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
-				getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT));
 	strbuf_addch(&sob, '\n');
 
 	if (!ignore_footer)
-- 
2.19.2

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

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 25 2019, William Hubbs wrote:

> The author.email, author.name, committer.email and committer.name
> settings are analogous to the GIT_AUTHOR_* and GIT_COMMITTER_*
> environment variables, but for the git config system. This allows them
> to be set separately for each repository.
>
> Git supports setting different authorship and committer
> information with environment variables. However, environment variables
> are set in the shell, so if different authorship and committer
> information is needed for different repositories an external tool is
> required.
>
> This adds support to git config for author.email, author.name,
> committer.email and committer.name  settings so this information
> can be set per repository.
>
> Also, it generalizes the fmt_ident function so it can handle author vs
> committer identification.

I have not tested this in any detail...

> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> index b5b2ba1199..18e1ec3c1b 100644
> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -1,12 +1,39 @@
> +author.email::
> +	The email address used for the author of newly
> +	created commits.  Defaults to the value of the
> +	`GIT_AUTHOR_EMAIL` environment variable, or if
> +	the environment variable is not set, the `user.email`
> +	configuration variable.
> +
> +author.name::
> +	The full name used for the author of newly created commits.
> +	Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or
> +	if the environment variable is not set,
> +	the `user.email` configuration variable.
> +
> +committer.email::
> +	The email address used for the committer of newly created commits.
> +	Defaults to the value of the `GIT_COMMITTER_EMAIL` environment
> +	variable, or if the environment variable is not set, the `user.email`
> +	configuration variable.
> +
> +committer.name::
> +	The full name used for the committer of newly created commits.
> +	Defaults to the value of the `GIT_COMMITTER_NAME` environment
> +	variable, or if the environment variable is not set, the `user.name`
> +	configuration variable.
> +
>  user.email::
>  	Your email address to be recorded in any newly created commits.
>  	Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and
> -	`EMAIL` environment variables.  See linkgit:git-commit-tree[1].
> +	`EMAIL` environment variables or the `author.email` or
> +	`committer.email` settings discussed above. See linkgit:git-commit-tree[1].
>
>  user.name::
>  	Your full name to be recorded in any newly created commits.
>  	Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME`
> -	environment variables.  See linkgit:git-commit-tree[1].
> +	environment variables or the `author.name` or `committer.name`
> +	settings discussed above. See linkgit:git-commit-tree[1].

Looks correct, although I wonder if we're at the point where it would be
better to present this info as a table.

> diff --git a/builtin/am.c b/builtin/am.c
> index 95370313b6..53fdd22c45 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,
> -			state->ignore_date ? NULL : state->author_date,
> +			WANT_AUTHOR_IDENT, state->ignore_date ? NULL : state->author_date,

This & a few other things in this series take the code beyond 79
characters.

>  			IDENT_STRICT);
>
>  	if (state->committer_date_is_author_date)
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 004b816635..f96b90daeb 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -607,7 +607,8 @@ static void determine_author_info(struct strbuf *author_ident)
>  		set_ident_var(&date, strbuf_detach(&date_buf, NULL));
>  	}
>
> -	strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));
> +	strbuf_addstr(author_ident, fmt_ident(name, email, WANT_AUTHOR_IDENT, date,
> +				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 49713cc5a5..f13c6e244f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1479,10 +1479,19 @@ int date_overflows(timestamp_t date);
>  #define IDENT_STRICT	       1
>  #define IDENT_NO_DATE	       2
>  #define IDENT_NO_NAME	       4
> +
> +enum want_ident {
> +	WANT_BLANK_IDENT,
> +	WANT_AUTHOR_IDENT,
> +	WANT_COMMITTER_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, const char *date_str, int);
> -extern const char *fmt_name(const char *name, const char *email);
> +extern const char *fmt_ident(const char *name, const char *email,
> +		enum want_ident whose_ident,
> +		const char *date_str, int);
> +extern const char *fmt_name(enum want_ident);
>  extern const char *ident_default_name(void);
>  extern const char *ident_default_email(void);
>  extern const char *git_editor(void);
> diff --git a/config.c b/config.c
> index ff521eb27a..4bd5920dea 100644
> --- a/config.c
> +++ b/config.c
> @@ -1484,6 +1484,12 @@ int git_default_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>
> +	if (starts_with(var, "author."))
> +		return git_ident_config(var, value, cb);
> +
> +	if (starts_with(var, "committer."))
> +		return git_ident_config(var, value, cb);
> +
>  	/* Add other config variables here and to Documentation/config.txt. */
>  	return 0;
>  }
> diff --git a/ident.c b/ident.c
> index 33bcf40644..a1e774c3e5 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -11,6 +11,10 @@
>  static struct strbuf git_default_name = STRBUF_INIT;
>  static struct strbuf git_default_email = STRBUF_INIT;
>  static struct strbuf git_default_date = STRBUF_INIT;
> +static struct strbuf git_author_name = STRBUF_INIT;
> +static struct strbuf git_author_email = STRBUF_INIT;
> +static struct strbuf git_committer_name = STRBUF_INIT;
> +static struct strbuf git_committer_email = STRBUF_INIT;
>  static int default_email_is_bogus;
>  static int default_name_is_bogus;
>
> @@ -355,13 +359,19 @@ N_("\n"
>     "\n");
>
>  const char *fmt_ident(const char *name, const char *email,
> -		      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);
>  	int want_date = !(flag & IDENT_NO_DATE);
>  	int want_name = !(flag & IDENT_NO_NAME);
>
> +	if (!email) {
> +		if (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)
> +			email = git_author_email.buf;
> +		else if (whose_ident == WANT_COMMITTER_IDENT && git_committer_email.len)
> +			email = git_committer_email.buf;
> +	}
>  	if (!email) {
>  		if (strict && ident_use_config_only
>  		    && !(ident_config_given & IDENT_MAIL_GIVEN)) {
> @@ -377,6 +387,13 @@ const char *fmt_ident(const char *name, const char *email,
>
>  	if (want_name) {
>  		int using_default = 0;
> +		if (!name) {
> +			if (whose_ident == WANT_AUTHOR_IDENT && git_author_name.len)
> +				name = git_author_name.buf;
> +			else if (whose_ident == WANT_COMMITTER_IDENT &&
> +					git_committer_name.len)
> +				name = git_committer_name.buf;
> +		}
>  		if (!name) {
>  			if (strict && ident_use_config_only
>  			    && !(ident_config_given & IDENT_NAME_GIVEN)) {
> @@ -425,9 +442,25 @@ const char *fmt_ident(const char *name, const char *email,
>  	return ident.buf;
>  }
>
> -const char *fmt_name(const char *name, const char *email)
> +const char *fmt_name(enum want_ident whose_ident)
>  {
> -	return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE);
> +	char *name = NULL;
> +	char *email = NULL;
> +
> +	switch (whose_ident) {
> +	case WANT_BLANK_IDENT:
> +		break;
> +	case WANT_AUTHOR_IDENT:
> +		name = getenv("GIT_AUTHOR_NAME");
> +		email = getenv("GIT_AUTHOR_EMAIL");
> +		break;
> +	case WANT_COMMITTER_IDENT:
> +		name = getenv("GIT_COMMITTER_NAME");
> +		email = getenv("GIT_COMMITTER_EMAIL");
> +		break;
> +	}
> +	return fmt_ident(name, email, whose_ident, NULL,
> +			IDENT_STRICT | IDENT_NO_DATE);
>  }
>
>  const char *git_author_info(int flag)
> @@ -438,6 +471,7 @@ const char *git_author_info(int flag)
>  		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
>  	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
>  			 getenv("GIT_AUTHOR_EMAIL"),
> +			 WANT_AUTHOR_IDENT,
>  			 getenv("GIT_AUTHOR_DATE"),
>  			 flag);
>  }
> @@ -450,6 +484,7 @@ const char *git_committer_info(int flag)
>  		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
>  	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
>  			 getenv("GIT_COMMITTER_EMAIL"),
> +			 WANT_COMMITTER_IDENT,
>  			 getenv("GIT_COMMITTER_DATE"),
>  			 flag);
>  }
> @@ -480,6 +515,46 @@ int git_ident_config(const char *var, const char *value, void *data)
>  		return 0;
>  	}
>
> +	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;
> +	}
> +

This whole thing should be split into a static function. It's the same
code copy/pasted 4x times just with a differnet value for "var", the
strbuf variable & IDENT_*_GIVEN.

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

[William Hubbs]

On Fri, Jan 25, 2019 at 11:58:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jan 25 2019, William Hubbs wrote:
> 
> > diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> > index b5b2ba1199..18e1ec3c1b 100644
> > --- a/Documentation/config/user.txt
> > +++ b/Documentation/config/user.txt
> > @@ -1,12 +1,39 @@
> > +author.email::
> > +	The email address used for the author of newly
> > +	created commits.  Defaults to the value of the
> > +	`GIT_AUTHOR_EMAIL` environment variable, or if
> > +	the environment variable is not set, the `user.email`
> > +	configuration variable.
> > +
> > +author.name::
> > +	The full name used for the author of newly created commits.
> > +	Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or
> > +	if the environment variable is not set,
> > +	the `user.email` configuration variable.
> > +
> > +committer.email::
> > +	The email address used for the committer of newly created commits.
> > +	Defaults to the value of the `GIT_COMMITTER_EMAIL` environment
> > +	variable, or if the environment variable is not set, the `user.email`
> > +	configuration variable.
> > +
> > +committer.name::
> > +	The full name used for the committer of newly created commits.
> > +	Defaults to the value of the `GIT_COMMITTER_NAME` environment
> > +	variable, or if the environment variable is not set, the `user.name`
> > +	configuration variable.
> > +
> >  user.email::
> >  	Your email address to be recorded in any newly created commits.
> >  	Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and
> > -	`EMAIL` environment variables.  See linkgit:git-commit-tree[1].
> > +	`EMAIL` environment variables or the `author.email` or
> > +	`committer.email` settings discussed above. See linkgit:git-commit-tree[1].
> >
> >  user.name::
> >  	Your full name to be recorded in any newly created commits.
> >  	Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME`
> > -	environment variables.  See linkgit:git-commit-tree[1].
> > +	environment variables or the `author.name` or `committer.name`
> > +	settings discussed above. See linkgit:git-commit-tree[1].
> 
> Looks correct, although I wonder if we're at the point where it would be
> better to present this info as a table.

Maybe, but can we have someone do that in a separate patch? I ask
because the documentation is not in a markup language and that would
make setting up a table difficult for me at best with my screen reader.

> > diff --git a/builtin/am.c b/builtin/am.c
> > index 95370313b6..53fdd22c45 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,
> > -			state->ignore_date ? NULL : state->author_date,
> > +			WANT_AUTHOR_IDENT, state->ignore_date ? NULL : state->author_date,
> 
> This & a few other things in this series take the code beyond 79
> characters.

This doesn't look like it is beyond 79 characters to me, but that may be
because I use a tab stop width of 4.

Can you reply again and flag the lines that are longer than 79
characters?

Thanks,

William

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

[Junio C Hamano]

William Hubbs <williamh@gentoo.org> writes:

> This doesn't look like it is beyond 79 characters to me, but that may be
> because I use a tab stop width of 4.

In this project, a tab skips to multiple of 8.

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

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 28 2019, William Hubbs wrote:

> On Fri, Jan 25, 2019 at 11:58:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Fri, Jan 25 2019, William Hubbs wrote:
>>
>> > diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
>> > index b5b2ba1199..18e1ec3c1b 100644
>> > --- a/Documentation/config/user.txt
>> > +++ b/Documentation/config/user.txt
>> > @@ -1,12 +1,39 @@
>> > +author.email::
>> > +	The email address used for the author of newly
>> > +	created commits.  Defaults to the value of the
>> > +	`GIT_AUTHOR_EMAIL` environment variable, or if
>> > +	the environment variable is not set, the `user.email`
>> > +	configuration variable.
>> > +
>> > +author.name::
>> > +	The full name used for the author of newly created commits.
>> > +	Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or
>> > +	if the environment variable is not set,
>> > +	the `user.email` configuration variable.
>> > +
>> > +committer.email::
>> > +	The email address used for the committer of newly created commits.
>> > +	Defaults to the value of the `GIT_COMMITTER_EMAIL` environment
>> > +	variable, or if the environment variable is not set, the `user.email`
>> > +	configuration variable.
>> > +
>> > +committer.name::
>> > +	The full name used for the committer of newly created commits.
>> > +	Defaults to the value of the `GIT_COMMITTER_NAME` environment
>> > +	variable, or if the environment variable is not set, the `user.name`
>> > +	configuration variable.
>> > +
>> >  user.email::
>> >  	Your email address to be recorded in any newly created commits.
>> >  	Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and
>> > -	`EMAIL` environment variables.  See linkgit:git-commit-tree[1].
>> > +	`EMAIL` environment variables or the `author.email` or
>> > +	`committer.email` settings discussed above. See linkgit:git-commit-tree[1].
>> >
>> >  user.name::
>> >  	Your full name to be recorded in any newly created commits.
>> >  	Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME`
>> > -	environment variables.  See linkgit:git-commit-tree[1].
>> > +	environment variables or the `author.name` or `committer.name`
>> > +	settings discussed above. See linkgit:git-commit-tree[1].
>>
>> Looks correct, although I wonder if we're at the point where it would be
>> better to present this info as a table.
>
> Maybe, but can we have someone do that in a separate patch? I ask
> because the documentation is not in a markup language and that would
> make setting up a table difficult for me at best with my screen reader.

I'm happy to help if you'd like. I had a thinko with "table", and I
think our asciidoc dialect doesn't support it (maybe I'm wrong), but
thinking about it again we could just describe these variables all in
the same documentation. As in this hunk (which you could squash in):

BEGIN QUOTE
diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index 18e1ec3c1b..ad3c43cf47 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -1,39 +1,20 @@
-author.email::
-	The email address used for the author of newly
-	created commits.  Defaults to the value of the
-	`GIT_AUTHOR_EMAIL` environment variable, or if
-	the environment variable is not set, the `user.email`
-	configuration variable.
-
+user.name::
+user.email::
 author.name::
-	The full name used for the author of newly created commits.
-	Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or
-	if the environment variable is not set,
-	the `user.email` configuration variable.
-
-committer.email::
-	The email address used for the committer of newly created commits.
-	Defaults to the value of the `GIT_COMMITTER_EMAIL` environment
-	variable, or if the environment variable is not set, the `user.email`
-	configuration variable.
-
+author.email::
 committer.name::
-	The full name used for the committer of newly created commits.
-	Defaults to the value of the `GIT_COMMITTER_NAME` environment
-	variable, or if the environment variable is not set, the `user.name`
-	configuration variable.
-
-user.email::
-	Your email address to be recorded in any newly created commits.
-	Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and
-	`EMAIL` environment variables or the `author.email` or
-	`committer.email` settings discussed above. See linkgit:git-commit-tree[1].
-
-user.name::
-	Your full name to be recorded in any newly created commits.
-	Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME`
-	environment variables or the `author.name` or `committer.name`
-	settings discussed above. See linkgit:git-commit-tree[1].
+committer.email::
+	The `user.name` and `user.email` variables determine what ends
+	up in the `author` and `committer` field of commit
+	objects. These config variables will be overridden by
+	`GIT_COMMITTER_NAME` and `GIT_COMMITTER_EMAIL`,
++
+Most users should have no reason to set the `author.*` and
+`committer.*` variables, but can do so to e.g. set different a
+different E-Mail for the `committer` field. Like the `user.name` and
+`user.email` variables, these can be overridden in the environment
+with `GIT_AUTHOR_NAME`, `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME` and
+`GIT_COMMITTER_EMAIL`.

 user.useConfigOnly::
 	Instruct Git to avoid trying to guess defaults for `user.email`
END QUOTE

Another thing I spotted while hacking that up is that the
git-commit-tree docs we're pointing to as the full explanation haven't
been updated. They still just talk about user.{name,email}.

And poking at this a bit more I see that something about this is
introducing new edge cases into our "you haven't set user.name or
user.email" logic. I.e. if I do:

    GIT_AUTHOR_NAME=hi GIT_AUTHOR_EMAIL=blah GIT_COMMITTER_NAME="hello" ./git-commit -a -m"hi"

I end up with an object like:

    author hi <blah> 1548705397 +0100
    committer hello <avar@nix.is> 1548705397 +0100

I.e. here I haven't supplied the committer E-Mail but it was inferred
(and a warning was printed to that effect). But if I do the same thing
with setting author.name etc. for all fields except committer.email I'll
get an empty ("<>") field for the committer E-Mail, even though it
printed "Your name and email address were configured automatically based
on your username and hostname".


>> > diff --git a/builtin/am.c b/builtin/am.c
>> > index 95370313b6..53fdd22c45 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,
>> > -			state->ignore_date ? NULL : state->author_date,
>> > +			WANT_AUTHOR_IDENT, state->ignore_date ? NULL : state->author_date,
>>
>> This & a few other things in this series take the code beyond 79
>> characters.
>
> This doesn't look like it is beyond 79 characters to me, but that may be
> because I use a tab stop width of 4.
>
> Can you reply again and flag the lines that are longer than 79
> characters?

I see Junio replied to this already.

Adjusting for limited time, I'm happy to help out with this series,
particularly if you have visual (screen reader) issues that make some of
this prohibitive for you. Just say what you need.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I'm happy to help if you'd like. I had a thinko with "table", and I
> think our asciidoc dialect doesn't support it (maybe I'm wrong), but
> thinking about it again we could just describe these variables all in
> the same documentation. As in this hunk (which you could squash in):
>
> BEGIN QUOTE
> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> index 18e1ec3c1b..ad3c43cf47 100644
> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -1,39 +1,20 @@
> +user.name::
> +user.email::
>  author.name::
> +author.email::
>  committer.name::
> +committer.email::
> +	The `user.name` and `user.email` variables determine what ends
> +	up in the `author` and `committer` field of commit
> +	objects. These config variables will be overridden by
> +	`GIT_COMMITTER_NAME` and `GIT_COMMITTER_EMAIL`,

You forgot to list two more obvious ones here.

> ++
> +Most users should have no reason to set the `author.*` and
> +`committer.*` variables, but can do so to e.g. set different a
> +different E-Mail for the `committer` field. Like the `user.name` and
> +`user.email` variables, these can be overridden in the environment
> +with `GIT_AUTHOR_NAME`, `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME` and
> +`GIT_COMMITTER_EMAIL`.

I do not see a strong reason to say "most users should have no
reason", especially without arguing why (and I do not think this is
a place to make such an argument, either).

    The `user.*` variables can be used to set both author and committer
    names and e-mail addresses to the same value; users who want to set
    the committer and author identities differently can use the
    `author.*` and `committer.*` variables.

or something along that line, perhaps?

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

[William Hubbs]

On Mon, Jan 28, 2019 at 09:04:15PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> Adjusting for limited time, I'm happy to help out with this series,
> particularly if you have visual (screen reader) issues that make some of
> this prohibitive for you. Just say what you need.

I guess the best way to handle formatting issues would be, during your
normal review, make a note directly below the affected line about what
is wrong with the formatting and I'll fix it during the next pass.

Or, another option would be to reply with a diff that I can apply on top
of my patch.

Thanks,

William

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

[William Hubbs]

On Fri, Jan 25, 2019 at 11:58:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jan 25 2019, William Hubbs wrote:
> 
> > @@ -480,6 +515,46 @@ int git_ident_config(const char *var, const char *value, void *data)
> >  		return 0;
> >  	}
> >
> > +	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;
> > +	}
> > +
> 
> This whole thing should be split into a static function. It's the same
> code copy/pasted 4x times just with a differnet value for "var", the
> strbuf variable & IDENT_*_GIVEN.

I have moved most of this into a separate function in the next version
of the patch. However, I do not see a way to factor it down further. Let
me know what you think when I resend.

Also, if you see anything  longer than 79 characters, please let me know
where the long lines are and I have no problem reformatting them.

Thanks much.

William

[PATCH v2 2/2] tests: add test for separate author and committer idents

[William Hubbs]

Signed-off-by: William Hubbs <williamh@gentoo.org>
---
 t/t7517-per-repo-email.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
index 231b8cc19d..06c7c0fb78 100755
--- a/t/t7517-per-repo-email.sh
+++ b/t/t7517-per-repo-email.sh
@@ -85,4 +85,21 @@ test_expect_success REBASE_P \
 	test_must_fail git rebase -p master
 '
 
+test_expect_success \
+	'author and committer config settings override user config settings' '
+	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
+	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
+	git config user.name user &&
+	git config user.email user@example.com &&
+	git config author.name author &&
+	git config author.email author@example.com &&
+	git config committer.name committer &&
+	git config committer.email committer@example.com &&
+	test_commit config-names &&
+	[ "$(git log --format=%an -1)" = "author" ] &&
+	[ "$(git log --format=%ae -1)" = "author@example.com" ] &&
+	[ "$(git log --format=%cn -1)" = "committer" ] &&
+	[ "$(git log --format=%ce -1)" = "committer@example.com" ]
+'
+
 test_done
-- 
2.19.2

Re: [PATCH v2 2/2] tests: add test for separate author and committer idents

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 25 2019, William Hubbs wrote:

> Signed-off-by: William Hubbs <williamh@gentoo.org>
> ---
>  t/t7517-per-repo-email.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
> index 231b8cc19d..06c7c0fb78 100755
> --- a/t/t7517-per-repo-email.sh
> +++ b/t/t7517-per-repo-email.sh
> @@ -85,4 +85,21 @@ test_expect_success REBASE_P \
>  	test_must_fail git rebase -p master
>  '

Let's include this in the main patch. We don't split up tests into their
own patches like this.

> +test_expect_success \
> +	'author and committer config settings override user config settings' '

This can just be on one line. We're not strict about 79 characters in
tests.

> +	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
> +	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&

Fine, but FYI sets these variables for the rest of the test.

But more importantly there should be a test for how the various override
interactions between the config & env variables work. I.e. whether
GIT_COMMITTER_NAME set in the env will override "user.email" etc.

> +	git config user.name user &&
> +	git config user.email user@example.com &&
> +	git config author.name author &&
> +	git config author.email author@example.com &&
> +	git config committer.name committer &&
> +	git config committer.email committer@example.com &&

This should use "test_config" so it'll be unset after this test.

> +	test_commit config-names &&
> +	[ "$(git log --format=%an -1)" = "author" ] &&
> +	[ "$(git log --format=%ae -1)" = "author@example.com" ] &&
> +	[ "$(git log --format=%cn -1)" = "committer" ] &&
> +	[ "$(git log --format=%ce -1)" = "committer@example.com" ]

Should use something like test_cmp so that on failure we see what the
difference is. I'd just do:

    cat >expected <<EOF... &&
    git log --format="an:%an%nae:%ae[...]" -1 >actual &&
    test_cmp ...

> +'
> +
>  test_done

Re: [PATCH v2 2/2] tests: add test for separate author and committer idents

[William Hubbs]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

On Sat, Jan 26, 2019 at 12:05:08AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jan 25 2019, William Hubbs wrote:

...

> > +	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
> > +	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
> 
> Fine, but FYI sets these variables for the rest of the test.

I'm not quite sure what you mean by this. I want the environment
variables to be *unset*. I don't want them to override anything in the
config file for this test.

Are you saying they will not be set for the test unless I set them,
so I don't need the SANE_UNSET calls?

Thanks,

William

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2 2/2] tests: add test for separate author and committer idents

[Ævar Arnfjörð Bjarmason]

On Sat, Jan 26 2019, William Hubbs wrote:

> On Sat, Jan 26, 2019 at 12:05:08AM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Fri, Jan 25 2019, William Hubbs wrote:
>
> ...
>
>> > +	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
>> > +	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
>>
>> Fine, but FYI sets these variables for the rest of the test.
>
> I'm not quite sure what you mean by this. I want the environment
> variables to be *unset*. I don't want them to override anything in the
> config file for this test.
>
> Are you saying they will not be set for the test unless I set them,
> so I don't need the SANE_UNSET calls?

Sorry for not being clear. I just meant that unlike "test_config" the
"sane_unset" function won't reset the state at the end of the
"test_expect_success".

Right now it doesn't matter in practice since this is the last test
before "test_done", but as tests are added we tend to leak state between
them, which is why we use these "unset at the end" helper functions.

But unlike with config that doesn't matter in this case, since we want
these unset anyway.

Which, looking at this again, you'd only want if a previous test in the
file was leaking its state. That's not the case, so this isn't needed
and you can just apply this on top:

    diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
    index 06c7c0fb78..e5845b0b86 100755
    --- a/t/t7517-per-repo-email.sh
    +++ b/t/t7517-per-repo-email.sh
    @@ -87,8 +87,6 @@ test_expect_success REBASE_P \

     test_expect_success \
            'author and committer config settings override user config settings' '
    -       sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
    -       sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
            git config user.name user &&
            git config user.email user@example.com &&
            git config author.name author &&

You don't need to be paranoid and unset these, we already unset GIT_*
variables that aren't on a whitelist in test-lib.sh, see 'A call to
"unset"' there.

Re: [PATCH v2 2/2] tests: add test for separate author and committer idents

[Eric Sunshine]

On Sat, Jan 26, 2019 at 3:53 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Which, looking at this again, you'd only want if a previous test in the
> file was leaking its state. That's not the case, so this isn't needed
> and you can just apply this on top:
>
>      test_expect_success \
>             'author and committer config settings override user config settings' '
>     -       sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
>     -       sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
>             git config user.name user &&
>             git config user.email user@example.com &&
>             git config author.name author &&

Aside from future-proofing against a test being inserted before this
one which does set those environment variables, these invocations of
sane_unset() serve the additional purpose of documenting the interplay
of configuration and environment, and further indicate to readers that
the test author took this into consideration (rather than merely
slapping together the test without thought). As a reviewer and reader
of the test, I appreciate the additional context the sane_unset()
calls provide, thus think it makes sense to retain them.

Re: [PATCH v2 2/2] tests: add test for separate author and committer idents

[Ævar Arnfjörð Bjarmason]

On Sun, Jan 27 2019, Eric Sunshine wrote:

> On Sat, Jan 26, 2019 at 3:53 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Which, looking at this again, you'd only want if a previous test in the
>> file was leaking its state. That's not the case, so this isn't needed
>> and you can just apply this on top:
>>
>>      test_expect_success \
>>             'author and committer config settings override user config settings' '
>>     -       sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
>>     -       sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
>>             git config user.name user &&
>>             git config user.email user@example.com &&
>>             git config author.name author &&
>
> Aside from future-proofing against a test being inserted before this
> one which does set those environment variables, these invocations of
> sane_unset() serve the additional purpose of documenting the interplay
> of configuration and environment, and further indicate to readers that
> the test author took this into consideration (rather than merely
> slapping together the test without thought). As a reviewer and reader
> of the test, I appreciate the additional context the sane_unset()
> calls provide, thus think it makes sense to retain them.

As noted in <875zuc49uj.fsf@evledraar.gmail.com> ("various override
interactions") there should definitely be more tests where the
combination of config & env is tested for.

But I don't see how it makes things clearer to unset a bunch of
variables previous tests didn't set. If we applied that to our test
suite much of it would be pointlessly unsetting various GIT_*
variables.

Better to assume other tests have cleaned up their own state, and when
it's not the case fix it.