[PATCH v5 0/1] config: allow giving separate author and committer

git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed

* [PATCH v5 0/1] config: allow giving separate author and committer
@ 2019-02-04 18:48 William Hubbs
  2019-02-04 18:48 ` [PATCH v5 1/1] config: allow giving separate author and committer idents William Hubbs
                   ` (3 more replies)
  0 siblings, 4 replies; 23+ messages in thread
From: William Hubbs @ 2019-02-04 18:48 UTC (permalink / raw)
  To: git; +Cc: williamh, chutzpah

*** BLURB HERE ***
This update adds back the reference to the EMAIL environment variable.

William Hubbs (1):
  config: allow giving separate author and committer idents

 Documentation/config/user.txt | 23 ++++++---
 blame.c                       |  3 +-
 builtin/am.c                  |  1 +
 builtin/commit.c              |  3 +-
 cache.h                       | 13 ++++-
 config.c                      |  4 +-
 ident.c                       | 92 ++++++++++++++++++++++++++++++++---
 log-tree.c                    |  3 +-
 sequencer.c                   |  5 +-
 t/t7517-per-repo-email.sh     | 74 ++++++++++++++++++++++++++++
 10 files changed, 197 insertions(+), 24 deletions(-)

-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v5 1/1] config: allow giving separate author and committer idents
  2019-02-04 18:48 [PATCH v5 0/1] config: allow giving separate author and committer William Hubbs
@ 2019-02-04 18:48 ` William Hubbs
  2019-02-05  9:16   ` Johannes Schindelin
  2019-02-05 19:52 ` [PATCH v6 0/2] New {author,committer}.{name,email} config Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 23+ messages in thread
From: William Hubbs @ 2019-02-04 18:48 UTC (permalink / raw)
  To: git; +Cc: williamh, chutzpah

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 | 23 ++++++---
 blame.c                       |  3 +-
 builtin/am.c                  |  1 +
 builtin/commit.c              |  3 +-
 cache.h                       | 13 ++++-
 config.c                      |  4 +-
 ident.c                       | 92 ++++++++++++++++++++++++++++++++---
 log-tree.c                    |  3 +-
 sequencer.c                   |  5 +-
 t/t7517-per-repo-email.sh     | 74 ++++++++++++++++++++++++++++
 10 files changed, 197 insertions(+), 24 deletions(-)

diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index b5b2ba1199..0557cbbceb 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -1,12 +1,19 @@
-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].
-
 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].
+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.
+	If you need the `author` or `committer` to be different, the
+	`author.name`, `author.email`, `committer.name` or
+	`committer.email` variables can be set.
+	Also, all of these can be overridden by the `GIT_AUTHOR_NAME`,
+	`GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME`,
+	`GIT_COMMITTER_EMAIL` and `EMAIL` environment variables.
+	See linkgit:git-commit-tree[1] for more information.
 
 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..3727d4d267 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1594,6 +1594,7 @@ static void do_commit(const struct am_state *state)
 	}
 
 	author = fmt_ident(state->author_name, state->author_email,
+		WANT_AUTHOR_IDENT,
 			state->ignore_date ? NULL : state->author_date,
 			IDENT_STRICT);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 7d2e0b61e5..98c642d993 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -608,7 +608,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 009e8b3b15..e9b870e536 100644
--- a/cache.h
+++ b/cache.h
@@ -1494,10 +1494,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..13349f308c 100644
--- a/config.c
+++ b/config.c
@@ -1445,7 +1445,9 @@ int git_default_config(const char *var, const char *value, void *cb)
 	if (starts_with(var, "core."))
 		return git_default_core_config(var, value, cb);
 
-	if (starts_with(var, "user."))
+	if (starts_with(var, "user.") ||
+		starts_with(var, "author.") ||
+			starts_with(var, "committer."))
 		return git_ident_config(var, value, cb);
 
 	if (starts_with(var, "i18n."))
diff --git a/ident.c b/ident.c
index 33bcf40644..9c2eb0a2d0 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);
 }
@@ -473,10 +508,45 @@ 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)
+static int set_ident(const char *var, const char *value)
 {
-	if (!strcmp(var, "user.useconfigonly")) {
-		ident_use_config_only = git_config_bool(var, value);
+	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;
 	}
 
@@ -505,6 +575,16 @@ int git_ident_config(const char *var, const char *value, void *data)
 	return 0;
 }
 
+int git_ident_config(const char *var, const char *value, void *data)
+{
+	if (!strcmp(var, "user.useconfigonly")) {
+		ident_use_config_only = git_config_bool(var, value);
+		return 0;
+	}
+
+	return set_ident(var, value);
+}
+
 static int buf_cmp(const char *a_begin, const char *a_end,
 		   const char *b_begin, const char *b_end)
 {
diff --git a/log-tree.c b/log-tree.c
index 3cb14256ec..1e56df62a7 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 213815dbfc..fe2a1b2fcf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -837,7 +837,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);
@@ -4094,8 +4094,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)
diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
index 231b8cc19d..b2401cec3e 100755
--- a/t/t7517-per-repo-email.sh
+++ b/t/t7517-per-repo-email.sh
@@ -85,4 +85,78 @@ test_expect_success REBASE_P \
 	test_must_fail git rebase -p master
 '
 
+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 &&
+	test_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
+test_expect_success 'author.email overrides user.email' '
+	test_config user.name user &&
+	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 &&
+	test_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
+test_expect_success 'committer.name overrides user.name' '
+	test_config user.name user &&
+	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 &&
+	test_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
+test_expect_success 'committer.email overrides user.email' '
+	test_config user.name user &&
+	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 &&
+	test_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
+test_expect_success 'author and committer environment variables override config settings' '
+	test_config user.name user &&
+	test_config user.email user@example.com &&
+	test_config author.name author &&
+	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_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
 test_done
-- 
2.19.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 1/1] config: allow giving separate author and committer idents
  2019-02-04 18:48 ` [PATCH v5 1/1] config: allow giving separate author and committer idents William Hubbs
@ 2019-02-05  9:16   ` Johannes Schindelin
  2019-02-05 18:02     ` Junio C Hamano
  0 siblings, 1 reply; 23+ messages in thread
From: Johannes Schindelin @ 2019-02-05  9:16 UTC (permalink / raw)
  To: William Hubbs; +Cc: git, chutzpah

Hi William,

On Mon, 4 Feb 2019, William Hubbs wrote:

> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> index b5b2ba1199..0557cbbceb 100644
> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -1,12 +1,19 @@
> -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].
> -
>  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].
> +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.
> +	If you need the `author` or `committer` to be different, the
> +	`author.name`, `author.email`, `committer.name` or
> +	`committer.email` variables can be set.
> +	Also, all of these can be overridden by the `GIT_AUTHOR_NAME`,
> +	`GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME`,
> +	`GIT_COMMITTER_EMAIL` and `EMAIL` environment variables.
> +	See linkgit:git-commit-tree[1] for more information.

Thank you, this looks good to me.

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 1/1] config: allow giving separate author and committer idents
  2019-02-05  9:16   ` Johannes Schindelin
@ 2019-02-05 18:02     ` Junio C Hamano
  0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2019-02-05 18:02 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: William Hubbs, git, chutzpah

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +	objects.
>> +	If you need the `author` or `committer` to be different, the
>> +	`author.name`, `author.email`, `committer.name` or
>> +	`committer.email` variables can be set.
>> +	Also, all of these can be overridden by the `GIT_AUTHOR_NAME`,
>> +	`GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME`,
>> +	`GIT_COMMITTER_EMAIL` and `EMAIL` environment variables.
>> +	See linkgit:git-commit-tree[1] for more information.
>
> Thank you, this looks good to me.

Thanks, both.  Queued.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v6 0/2] New {author,committer}.{name,email} config
  2019-02-04 18:48 [PATCH v5 0/1] config: allow giving separate author and committer William Hubbs
  2019-02-04 18:48 ` [PATCH v5 1/1] config: allow giving separate author and committer idents William Hubbs
@ 2019-02-05 19:52 ` Ævar Arnfjörð Bjarmason
  2019-02-05 19:52 ` [PATCH v6 1/2] ident: test how GIT_* and user.{name,email} interact Ævar Arnfjörð Bjarmason
  2019-02-05 19:52 ` [PATCH v6 2/2] config: allow giving separate author and committer idents Ævar Arnfjörð Bjarmason
  3 siblings, 0 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-02-05 19:52 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, William Hubbs, chutzpah,
	Ævar Arnfjörð Bjarmason

I had some feedback on v2 that wasn't addressed. See
https://public-inbox.org/git/875zuc49uj.fsf@evledraar.gmail.com/ &
https://public-inbox.org/git/871s4w4khs.fsf@evledraar.gmail.com/

This fixes that, except I couldn't with limited time reproduce the bug
I was talking about in the latter E-Mail in the test suite, maybe we
only infer the E-Mail from the hostname outside of it? So 1/1 is the
patch I was going to fleshen out with that test, but didn't, but I
figured I'd leave it.

Other changes can be seen in the range-diff, briefly:

* We now update the git-commit-tree docs
* Bunch of coding style nits fixed (alignment, past 79 chars)
* Factored a bunch of copy/pasted code into a helper in ident.c
* Fixed a test to skip a whole GIT_*= && export && sane_unset dance,
  and just use test_env instead.
* Test style: ">f" not "> f" in redirection.

William Hubbs (1):
  config: allow giving separate author and committer idents

Ævar Arnfjörð Bjarmason (1):
  ident: test how GIT_* and user.{name,email} interact

 Documentation/config/user.txt     |  23 ++++---
 Documentation/git-commit-tree.txt |   3 +-
 blame.c                           |   3 +-
 builtin/am.c                      |   1 +
 builtin/commit.c                  |   3 +-
 cache.h                           |  13 +++-
 config.c                          |   4 +-
 ident.c                           | 101 +++++++++++++++++++++-------
 log-tree.c                        |   3 +-
 sequencer.c                       |   6 +-
 t/t7517-per-repo-email.sh         | 108 ++++++++++++++++++++++++++++++
 11 files changed, 223 insertions(+), 45 deletions(-)

Range-diff:
-:  ---------- > 1:  ffd41a882a ident: test how GIT_* and user.{name,email} interact
1:  1172f91155 ! 2:  788ec8412d config: allow giving separate author and committer idents
    @@ -21,6 +21,7 @@
         committer identification.
     
         Signed-off-by: William Hubbs <williamh@gentoo.org>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
      --- a/Documentation/config/user.txt
    @@ -54,6 +55,20 @@
      user.useConfigOnly::
      	Instruct Git to avoid trying to guess defaults for `user.email`
     
    + diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
    + --- a/Documentation/git-commit-tree.txt
    + +++ b/Documentation/git-commit-tree.txt
    +@@
    + (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/blame.c b/blame.c
      --- a/blame.c
      +++ b/blame.c
    @@ -75,7 +90,7 @@
      	}
      
      	author = fmt_ident(state->author_name, state->author_email,
    -+		WANT_AUTHOR_IDENT,
    ++			WANT_AUTHOR_IDENT,
      			state->ignore_date ? NULL : state->author_date,
      			IDENT_STRICT);
      
    @@ -89,7 +104,7 @@
      
     -	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));
    ++					      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);
    @@ -113,8 +128,8 @@
     -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);
    ++			     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);
    @@ -129,8 +144,8 @@
      
     -	if (starts_with(var, "user."))
     +	if (starts_with(var, "user.") ||
    -+		starts_with(var, "author.") ||
    -+			starts_with(var, "committer."))
    ++	    starts_with(var, "author.") ||
    ++	    starts_with(var, "committer."))
      		return git_ident_config(var, value, cb);
      
      	if (starts_with(var, "i18n."))
    @@ -154,7 +169,8 @@
      
      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)
    ++		      enum want_ident whose_ident, const char *date_str,
    ++		      int flag)
      {
      	static struct strbuf ident = STRBUF_INIT;
      	int strict = (flag & IDENT_STRICT);
    @@ -232,68 +248,75 @@
      	return ident_is_sufficient(author_ident_explicitly_given);
      }
      
    --int git_ident_config(const char *var, const char *value, void *data)
    -+static int set_ident(const char *var, const char *value)
    - {
    --	if (!strcmp(var, "user.useconfigonly")) {
    --		ident_use_config_only = git_config_bool(var, value);
    -+	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;
    -+	}
    ++static int set_ident_internal(const char *var, const char *value,
    ++			    struct strbuf *sb, const int flag)
    ++{
    ++	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;
    ++}
     +
    -+	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;
    -+	}
    ++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;
    ++}
     +
    -+	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;
    + int git_ident_config(const char *var, const char *value, void *data)
    + {
    + 	if (!strcmp(var, "user.useconfigonly")) {
    +@@
      		return 0;
      	}
      
    -@@
    - 	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;
    +-	}
    +-
    +-	return 0;
    ++	return set_ident(var, value);
      }
      
    -+int git_ident_config(const char *var, const char *value, void *data)
    -+{
    -+	if (!strcmp(var, "user.useconfigonly")) {
    -+		ident_use_config_only = git_config_bool(var, value);
    -+		return 0;
    -+	}
    -+
    -+	return set_ident(var, value);
    -+}
    -+
      static int buf_cmp(const char *a_begin, const char *a_end,
    - 		   const char *b_begin, const char *b_end)
    - {
     
      diff --git a/log-tree.c b/log-tree.c
      --- a/log-tree.c
    @@ -317,7 +340,8 @@
      
      	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_addstr(&out, fmt_ident(name, email, WANT_AUTHOR_IDENT, date,
    ++				      0));
      	strbuf_swap(buf, &out);
      	strbuf_release(&out);
      	free(name);
    @@ -339,15 +363,24 @@
      	test_must_fail git rebase -p master
      '
      
    +-test_expect_success 'fallbacks for GIT_* and user.{name,email}' '
    ++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 \
    +@@
    + 	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
     +'
    @@ -357,10 +390,10 @@
     +	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
     +'
    @@ -370,10 +403,10 @@
     +	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
     +'
    @@ -383,10 +416,10 @@
     +	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
     +'
    @@ -398,17 +431,17 @@
     +	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
     +'
-- 
2.20.1.611.gfbb209baf1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v6 1/2] ident: test how GIT_* and user.{name,email} interact
  2019-02-04 18:48 [PATCH v5 0/1] config: allow giving separate author and committer William Hubbs
  2019-02-04 18:48 ` [PATCH v5 1/1] config: allow giving separate author and committer idents William Hubbs
  2019-02-05 19:52 ` [PATCH v6 0/2] New {author,committer}.{name,email} config Ævar Arnfjörð Bjarmason
@ 2019-02-05 19:52 ` Ævar Arnfjörð Bjarmason
  2019-02-05 19:52 ` [PATCH v6 2/2] config: allow giving separate author and committer idents Ævar Arnfjörð Bjarmason
  3 siblings, 0 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-02-05 19:52 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, William Hubbs, chutzpah,
	Ævar Arnfjörð Bjarmason

There were no explicit tests for the interaction between setting GIT_*
in the environment, and the user.{name,email} config variables. These
tests are basic, but we're about to learn
{author,committer}.{name,email} in addition to user.{name,email}, so
they'll soon become more useful.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7517-per-repo-email.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
index 231b8cc19d..84bd9e89e5 100755
--- a/t/t7517-per-repo-email.sh
+++ b/t/t7517-per-repo-email.sh
@@ -85,4 +85,38 @@ test_expect_success REBASE_P \
 	test_must_fail git rebase -p master
 '
 
+test_expect_success 'fallbacks for GIT_* and user.{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_done
-- 
2.20.1.611.gfbb209baf1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-04 18:48 [PATCH v5 0/1] config: allow giving separate author and committer William Hubbs
                   ` (2 preceding siblings ...)
  2019-02-05 19:52 ` [PATCH v6 1/2] ident: test how GIT_* and user.{name,email} interact Ævar Arnfjörð Bjarmason
@ 2019-02-05 19:52 ` Ævar Arnfjörð Bjarmason
  2019-02-05 20:22   ` Junio C Hamano
  2019-04-15 14:24   ` Derrick Stolee
  3 siblings, 2 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-02-05 19:52 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, William Hubbs, chutzpah,
	Ævar Arnfjörð Bjarmason

From: William Hubbs <williamh@gentoo.org>

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>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/user.txt     |  23 ++++---
 Documentation/git-commit-tree.txt |   3 +-
 blame.c                           |   3 +-
 builtin/am.c                      |   1 +
 builtin/commit.c                  |   3 +-
 cache.h                           |  13 +++-
 config.c                          |   4 +-
 ident.c                           | 101 ++++++++++++++++++++++--------
 log-tree.c                        |   3 +-
 sequencer.c                       |   6 +-
 t/t7517-per-repo-email.sh         |  76 +++++++++++++++++++++-
 11 files changed, 190 insertions(+), 46 deletions(-)

diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index b5b2ba1199..0557cbbceb 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -1,12 +1,19 @@
-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].
-
 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].
+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.
+	If you need the `author` or `committer` to be different, the
+	`author.name`, `author.email`, `committer.name` or
+	`committer.email` variables can be set.
+	Also, all of these can be overridden by the `GIT_AUTHOR_NAME`,
+	`GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME`,
+	`GIT_COMMITTER_EMAIL` and `EMAIL` environment variables.
+	See linkgit:git-commit-tree[1] for more information.
 
 user.useConfigOnly::
 	Instruct Git to avoid trying to guess defaults for `user.email`
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/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..d4a1cbe828 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1594,6 +1594,7 @@ static void do_commit(const struct am_state *state)
 	}
 
 	author = fmt_ident(state->author_name, state->author_email,
+			WANT_AUTHOR_IDENT,
 			state->ignore_date ? NULL : state->author_date,
 			IDENT_STRICT);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 7d2e0b61e5..0af8fc3503 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -608,7 +608,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 009e8b3b15..c2f9d4e680 100644
--- a/cache.h
+++ b/cache.h
@@ -1494,10 +1494,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 flag);
+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..fd36db7790 100644
--- a/config.c
+++ b/config.c
@@ -1445,7 +1445,9 @@ int git_default_config(const char *var, const char *value, void *cb)
 	if (starts_with(var, "core."))
 		return git_default_core_config(var, value, cb);
 
-	if (starts_with(var, "user."))
+	if (starts_with(var, "user.") ||
+	    starts_with(var, "author.") ||
+	    starts_with(var, "committer."))
 		return git_ident_config(var, value, cb);
 
 	if (starts_with(var, "i18n."))
diff --git a/ident.c b/ident.c
index 33bcf40644..7c3be81ee1 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,20 @@ 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 +388,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 +443,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 +472,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 +485,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);
 }
@@ -473,6 +509,41 @@ int author_ident_sufficiently_given(void)
 	return ident_is_sufficient(author_ident_explicitly_given);
 }
 
+static int set_ident_internal(const char *var, const char *value,
+			    struct strbuf *sb, const int flag)
+{
+	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;
+}
+
 int git_ident_config(const char *var, const char *value, void *data)
 {
 	if (!strcmp(var, "user.useconfigonly")) {
@@ -480,29 +551,7 @@ int git_ident_config(const char *var, const char *value, void *data)
 		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;
-	}
-
-	return 0;
+	return set_ident(var, value);
 }
 
 static int buf_cmp(const char *a_begin, const char *a_end,
diff --git a/log-tree.c b/log-tree.c
index 3cb14256ec..1e56df62a7 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 213815dbfc..c031787826 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -837,7 +837,8 @@ 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);
@@ -4094,8 +4095,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)
diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
index 84bd9e89e5..e0182779ed 100755
--- a/t/t7517-per-repo-email.sh
+++ b/t/t7517-per-repo-email.sh
@@ -85,7 +85,7 @@ test_expect_success REBASE_P \
 	test_must_fail git rebase -p master
 '
 
-test_expect_success 'fallbacks for GIT_* and user.{name,email}' '
+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 \
@@ -119,4 +119,78 @@ test_expect_success 'fallbacks for GIT_* and user.{name,email}' '
 	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 &&
+	test_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
+test_expect_success 'author.email overrides user.email' '
+	test_config user.name user &&
+	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 &&
+	test_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
+test_expect_success 'committer.name overrides user.name' '
+	test_config user.name user &&
+	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 &&
+	test_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
+test_expect_success 'committer.email overrides user.email' '
+	test_config user.name user &&
+	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 &&
+	test_cmp expected-author actual-author &&
+	test_cmp expected-committer actual-committer
+'
+
+test_expect_success 'author and committer environment variables override config settings' '
+	test_config user.name user &&
+	test_config user.email user@example.com &&
+	test_config author.name author &&
+	test_config author.email author@example.com &&
+	test_config committer.name committer &&
+	test_config committer.email committer@example.com &&
+
+	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
+'
+
 test_done
-- 
2.20.1.611.gfbb209baf1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-05 19:52 ` [PATCH v6 2/2] config: allow giving separate author and committer idents Ævar Arnfjörð Bjarmason
@ 2019-02-05 20:22   ` Junio C Hamano
  2019-02-05 21:14     ` Ævar Arnfjörð Bjarmason
  2019-04-15 14:24   ` Derrick Stolee
  1 sibling, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2019-02-05 20:22 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, William Hubbs, chutzpah

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

> +static int set_ident_internal(const char *var, const char *value,
> +			    struct strbuf *sb, const int flag)
> +{
> +	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;
> +}

In the v5 patch from William, author_ident_explicitly_given and
committer_ident_explicitly_given were set separately depending on
what variable was given (e.g. user.name marked both, author.name
marked only author but not committer_ident_explicitly_given).  In
the original before the addition of this feature with v6, giving
user.name would have set both, as we can see below.

Is this change intended?  

Or did you find that committer_ident_explicitly_given is no longer
useful and the variable is not used anymore?

>  int git_ident_config(const char *var, const char *value, void *data)
>  {
>  	if (!strcmp(var, "user.useconfigonly")) {
> @@ -480,29 +551,7 @@ int git_ident_config(const char *var, const char *value, void *data)
>  		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;
> -	}
> -
> -	return 0;
> +	return set_ident(var, value);
>  }

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-05 20:22   ` Junio C Hamano
@ 2019-02-05 21:14     ` Ævar Arnfjörð Bjarmason
  2019-02-06  0:04       ` William Hubbs
  2019-02-06  9:28       ` Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-02-05 21:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, William Hubbs, chutzpah


On Tue, Feb 05 2019, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> +static int set_ident_internal(const char *var, const char *value,
>> +			    struct strbuf *sb, const int flag)
>> +{
>> +	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;
>> +}
>
> In the v5 patch from William, author_ident_explicitly_given and
> committer_ident_explicitly_given were set separately depending on
> what variable was given (e.g. user.name marked both, author.name
> marked only author but not committer_ident_explicitly_given).  In
> the original before the addition of this feature with v6, giving
> user.name would have set both, as we can see below.
>
> Is this change intended?
>
> Or did you find that committer_ident_explicitly_given is no longer
> useful and the variable is not used anymore?

No, that's a mistake of mine when porting this over, but also clearly a
blindspot in our tests since they all pass with this.

I haven't dug (don't have time right now) to check what the effect of
that is. William?

>>  int git_ident_config(const char *var, const char *value, void *data)
>>  {
>>  	if (!strcmp(var, "user.useconfigonly")) {
>> @@ -480,29 +551,7 @@ int git_ident_config(const char *var, const char *value, void *data)
>>  		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;
>> -	}
>> -
>> -	return 0;
>> +	return set_ident(var, value);
>>  }

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-05 21:14     ` Ævar Arnfjörð Bjarmason
@ 2019-02-06  0:04       ` William Hubbs
  2019-02-06  0:15         ` William Hubbs
                           ` (2 more replies)
  2019-02-06  9:28       ` Ævar Arnfjörð Bjarmason
  1 sibling, 3 replies; 23+ messages in thread
From: William Hubbs @ 2019-02-06  0:04 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, git, chutzpah, williamh

On Tue, Feb 05, 2019 at 10:14:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Feb 05 2019, Junio C Hamano wrote:
> 
> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> >
> >> +static int set_ident_internal(const char *var, const char *value,
> >> +			    struct strbuf *sb, const int flag)
> >> +{
> >> +	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;
> >> +}
> >
> > In the v5 patch from William, author_ident_explicitly_given and
> > committer_ident_explicitly_given were set separately depending on
> > what variable was given (e.g. user.name marked both, author.name
> > marked only author but not committer_ident_explicitly_given).  In
> > the original before the addition of this feature with v6, giving
> > user.name would have set both, as we can see below.
> >
> > Is this change intended?
> >
> > Or did you find that committer_ident_explicitly_given is no longer
> > useful and the variable is not used anymore?
> 
> No, that's a mistake of mine when porting this over, but also clearly a
> blindspot in our tests since they all pass with this.
> 
> I haven't dug (don't have time right now) to check what the effect of
> that is. William?

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

Also, according to Junio's report, my patch is already merged to next,
so if you give me your patch based on that, I'll take a look, but it may
be a day or so.

William

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06  0:04       ` William Hubbs
@ 2019-02-06  0:15         ` William Hubbs
  2019-02-06  1:05           ` William Hubbs
  2019-02-06  5:03         ` Junio C Hamano
  2019-02-06  8:58         ` Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 23+ messages in thread
From: William Hubbs @ 2019-02-06  0:15 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, git, chutzpah, williamh

On Tue, Feb 05, 2019 at 06:04:13PM -0600, William Hubbs wrote:
> On Tue, Feb 05, 2019 at 10:14:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > 
> > On Tue, Feb 05 2019, Junio C Hamano wrote:
> > 
> > > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> > >
> > >> +static int set_ident_internal(const char *var, const char *value,
> > >> +			    struct strbuf *sb, const int flag)
> > >> +{
> > >> +	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;
> > >> +}
> > >
> > > In the v5 patch from William, author_ident_explicitly_given and
> > > committer_ident_explicitly_given were set separately depending on
> > > what variable was given (e.g. user.name marked both, author.name
> > > marked only author but not committer_ident_explicitly_given).  In
> > > the original before the addition of this feature with v6, giving
> > > user.name would have set both, as we can see below.
> > >
> > > Is this change intended?
> > >
> > > Or did you find that committer_ident_explicitly_given is no longer
> > > useful and the variable is not used anymore?
> > 
> > No, that's a mistake of mine when porting this over, but also clearly a
> > blindspot in our tests since they all pass with this.
> > 
> > I haven't dug (don't have time right now) to check what the effect of
> > that is. William?
> 
> I attempted to save your patches to apply them, but didn't have any luck

More info here. I use mutt and when I tagged the patches and saved them
to a mailbox, they went to a mailldir type folder, and "git am" didn't
seem to like that, but I'll try again.

> Also, according to Junio's report, my patch is already merged to next,
> so if you give me your patch based on that, I'll take a look, but it may
> be a day or so.

Sorry, I misread, my patch is not in next.

But, Junio's discussion above is why I didn't make this change. I don't
know the code base all that well, so I didn't want my first patch to
possibly introduce regressions. :-)

William

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06  0:15         ` William Hubbs
@ 2019-02-06  1:05           ` William Hubbs
  0 siblings, 0 replies; 23+ messages in thread
From: William Hubbs @ 2019-02-06  1:05 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, git, chutzpah, williamh

On Tue, Feb 05, 2019 at 06:15:58PM -0600, William Hubbs wrote:
> On Tue, Feb 05, 2019 at 06:04:13PM -0600, William Hubbs wrote:
> > On Tue, Feb 05, 2019 at 10:14:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > > 
> > > On Tue, Feb 05 2019, Junio C Hamano wrote:
> > > 
> > > > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> > > >
> > > >> +static int set_ident_internal(const char *var, const char *value,
> > > >> +			    struct strbuf *sb, const int flag)
> > > >> +{
> > > >> +	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;
> > > >> +}
> > > >
> > > > In the v5 patch from William, author_ident_explicitly_given and
> > > > committer_ident_explicitly_given were set separately depending on
> > > > what variable was given (e.g. user.name marked both, author.name
> > > > marked only author but not committer_ident_explicitly_given).  In
> > > > the original before the addition of this feature with v6, giving
> > > > user.name would have set both, as we can see below.
> > > >
> > > > Is this change intended?
> > > >
> > > > Or did you find that committer_ident_explicitly_given is no longer
> > > > useful and the variable is not used anymore?
> > > 
> > > No, that's a mistake of mine when porting this over, but also clearly a
> > > blindspot in our tests since they all pass with this.
> > > 
> > > I haven't dug (don't have time right now) to check what the effect of
> > > that is. William?
> > 
> > I attempted to save your patches to apply them, but didn't have any luck
> 
> More info here. I use mutt and when I tagged the patches and saved them
> to a mailbox, they went to a mailldir type folder, and "git am" didn't
> seem to like that, but I'll try again.

I tried one more time, saving the patches to individual files, and they
still would not apply.

Thanks,

William


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06  0:04       ` William Hubbs
  2019-02-06  0:15         ` William Hubbs
@ 2019-02-06  5:03         ` Junio C Hamano
  2019-02-13 16:43           ` William Hubbs
  2019-02-06  8:58         ` Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2019-02-06  5:03 UTC (permalink / raw)
  To: William Hubbs; +Cc: Ævar Arnfjörð Bjarmason, git, chutzpah

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
 '


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06  5:03         ` Junio C Hamano
@ 2019-02-13 16:43           ` William Hubbs
  2019-02-13 22:37             ` Junio C Hamano
  0 siblings, 1 reply; 23+ messages in thread
From: William Hubbs @ 2019-02-13 16:43 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Ævar Arnfjörð Bjarmason, git, chutzpah, williamh

Hi Junio,

I am writing back onn this thread because I'm not quite sure of the
status. v5 of the patch seemed ok, but there were some changes discussed
that would have created a v6. The v6 changes though were never really
clear. I'm not sure whether I am supposed to be doing something more or
whether I'm waiting for you. ;-)

Can you write back and let me know?

Thanks,

William

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-13 16:43           ` William Hubbs
@ 2019-02-13 22:37             ` Junio C Hamano
  2019-02-14 18:17               ` William Hubbs
  0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2019-02-13 22:37 UTC (permalink / raw)
  To: William Hubbs; +Cc: Ævar Arnfjörð Bjarmason, git, chutzpah

William Hubbs <williamh@gentoo.org> writes:

> I am writing back onn this thread because I'm not quite sure of the
> status. v5 of the patch seemed ok, but there were some changes discussed
> that would have created a v6. The v6 changes though were never really
> clear. I'm not sure whether I am supposed to be doing something more or
> whether I'm waiting for you. ;-)
>
> Can you write back and let me know?

In general, unless I ask you to wait, a contributor would almost
never be waiting for me.

I think Ævar's v6 was not up to par, but I thought that v5 from you
(which is in 'next') was good enough to cook in 'next'.  The topic
will not be moving out of 'next' until the final 2.21 is released
anyway, so if anything, I'd say the ball is in his court to update
his version after the release, when your v5 may have a chance to be
kicked out of 'next' and replaced _if_ there is a better version by
then.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-13 22:37             ` Junio C Hamano
@ 2019-02-14 18:17               ` William Hubbs
  0 siblings, 0 replies; 23+ messages in thread
From: William Hubbs @ 2019-02-14 18:17 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Ævar Arnfjörð Bjarmason, git, chutzpah, williamh

On Wed, Feb 13, 2019 at 02:37:48PM -0800, Junio C Hamano wrote:
> William Hubbs <williamh@gentoo.org> writes:
> 
> > I am writing back onn this thread because I'm not quite sure of the
> > status. v5 of the patch seemed ok, but there were some changes discussed
> > that would have created a v6. The v6 changes though were never really
> > clear. I'm not sure whether I am supposed to be doing something more or
> > whether I'm waiting for you. ;-)
> >
> > Can you write back and let me know?
> 
> In general, unless I ask you to wait, a contributor would almost
> never be waiting for me.
> 
> I think Ævar's v6 was not up to par, but I thought that v5 from you
> (which is in 'next') was good enough to cook in 'next'.  The topic
> will not be moving out of 'next' until the final 2.21 is released
> anyway, so if anything, I'd say the ball is in his court to update
> his version after the release, when your v5 may have a chance to be
> kicked out of 'next' and replaced _if_ there is a better version by
> then.

Thanks much for the update. I didn't realize that v5 was in next.

William


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06  0:04       ` William Hubbs
  2019-02-06  0:15         ` William Hubbs
  2019-02-06  5:03         ` Junio C Hamano
@ 2019-02-06  8:58         ` Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-02-06  8:58 UTC (permalink / raw)
  To: William Hubbs; +Cc: Junio C Hamano, git, chutzpah


On Wed, Feb 06 2019, William Hubbs wrote:

> On Tue, Feb 05, 2019 at 10:14:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Feb 05 2019, Junio C Hamano wrote:
>>
>> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>> >
>> >> +static int set_ident_internal(const char *var, const char *value,
>> >> +			    struct strbuf *sb, const int flag)
>> >> +{
>> >> +	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;
>> >> +}
>> >
>> > In the v5 patch from William, author_ident_explicitly_given and
>> > committer_ident_explicitly_given were set separately depending on
>> > what variable was given (e.g. user.name marked both, author.name
>> > marked only author but not committer_ident_explicitly_given).  In
>> > the original before the addition of this feature with v6, giving
>> > user.name would have set both, as we can see below.
>> >
>> > Is this change intended?
>> >
>> > Or did you find that committer_ident_explicitly_given is no longer
>> > useful and the variable is not used anymore?
>>
>> No, that's a mistake of mine when porting this over, but also clearly a
>> blindspot in our tests since they all pass with this.
>>
>> I haven't dug (don't have time right now) to check what the effect of
>> that is. William?
>
> I attempted to save your patches to apply them, but didn't have any luck

On top of current master you can do e.g.:

    vm git (master $=) $ wget -q -O- https://public-inbox.org/git/20190205195212.25550-2-avarab@gmail.com/raw | git am
    Applying: ident: test how GIT_* and user.{name,email} interact
    vm git (master $>) $ wget -q -O- https://public-inbox.org/git/20190205195212.25550-3-avarab@gmail.com/raw | git am
    Applying: config: allow giving separate author and committer idents

I also push these to https://github.com/avar/git.git although not always
in a timely manner...

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-05 21:14     ` Ævar Arnfjörð Bjarmason
  2019-02-06  0:04       ` William Hubbs
@ 2019-02-06  9:28       ` Ævar Arnfjörð Bjarmason
  2019-02-06 18:26         ` Jeff King
  2019-02-06 18:34         ` William Hubbs
  1 sibling, 2 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-02-06  9:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, William Hubbs, chutzpah, Jeff King


On Tue, Feb 05 2019, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Feb 05 2019, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> +static int set_ident_internal(const char *var, const char *value,
>>> +			    struct strbuf *sb, const int flag)
>>> +{
>>> +	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;
>>> +}
>>
>> In the v5 patch from William, author_ident_explicitly_given and
>> committer_ident_explicitly_given were set separately depending on
>> what variable was given (e.g. user.name marked both, author.name
>> marked only author but not committer_ident_explicitly_given).  In
>> the original before the addition of this feature with v6, giving
>> user.name would have set both, as we can see below.
>>
>> Is this change intended?
>>
>> Or did you find that committer_ident_explicitly_given is no longer
>> useful and the variable is not used anymore?
>
> No, that's a mistake of mine when porting this over, but also clearly a
> blindspot in our tests since they all pass with this.
>
> I haven't dug (don't have time right now) to check what the effect of
> that is. William?

I did some further digging. One of the confusing things is that we've
been carrying dead code since 2012 to set this
author_ident_explicitly_given variable. We can just apply this on top of
master:

     cache.h |  1 -
     ident.c | 13 -------------
     2 files changed, 14 deletions(-)

    diff --git a/cache.h b/cache.h
    index 038e3764a9..52308bd5e4 100644
    --- a/cache.h
    +++ b/cache.h
    @@ -1631,3 +1631,2 @@ extern int ignore_untracked_cache_config;
     extern int committer_ident_sufficiently_given(void);
    -extern int author_ident_sufficiently_given(void);

    diff --git a/ident.c b/ident.c
    index 33bcf40644..95fa2370e5 100644
    --- a/ident.c
    +++ b/ident.c
    @@ -22,3 +22,2 @@ static int ident_use_config_only;
     static int committer_ident_explicitly_given;
    -static int author_ident_explicitly_given;
     static int ident_config_given;
    @@ -169,3 +168,2 @@ const char *ident_default_email(void)
     			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
    -			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
     		} else if ((email = query_user_email()) && email[0]) {
    @@ -434,6 +432,2 @@ 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"),
    @@ -470,7 +464,2 @@ int committer_ident_sufficiently_given(void)

    -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)
    @@ -488,3 +477,2 @@ int git_ident_config(const char *var, const char *value, void *data)
     		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
    -		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
     		ident_config_given |= IDENT_NAME_GIVEN;
    @@ -499,3 +487,2 @@ int git_ident_config(const char *var, const char *value, void *data)
     		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
    -		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
     		ident_config_given |= IDENT_MAIL_GIVEN;

A more complete "fix" is to entirely revert Jeff's d6991ceedc ("ident:
keep separate "explicit" flags for author and committer",
2012-11-14). As he noted in 2012
(https://public-inbox.org/git/20121128182534.GA21020@sigill.intra.peff.net/):

    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.

I also found that the bug in my v6 is easily spotted & fixed. We just
grep stderr to see if we emit the "Your name and email were configured
automatically..." message. My patch v6 introduced a bug where we'd start
emitting that.

So it seems to me that the best way forward is to produce a series
where:

 1. We revert Jeff's 2012 patch (or not, Jeff?)

 2. Fix the tests so we test that given a combo of env vars & config
    create the expected commit objects *and* either emit the advice
    about having set stuff implicitly, or not.

    These need a lot of improvement, e.g. all our tests pass if I apply
    this:

        diff --git a/ident.c b/ident.c
        index 33bcf40644..f68e3c32ea 100644
        --- a/ident.c
        +++ b/ident.c
        @@ -167,6 +167,4 @@ const char *ident_default_email(void)
                        if (email && email[0]) {
                                strbuf_addstr(&git_default_email, email);
        -                       committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
        -                       author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
                        } else if ((email = query_user_email()) && email[0]) {
                                strbuf_addstr(&git_default_email, email);

 3. This {author,committer}.{name,email} feature on top of that.

William, is that something you're intererested in carrying forward? I
can also help if you want. Sorry your first contribution to git has
turned into this mess of re-rolls, as often happens we find that when
trying to tweak something that the existing behavior doesn't have any
tests.

I think it's worth spending a bit more time here to prove to ourselves
that the changes aren't e.g. spamming users with the ident advice in
cases where we don't want that, but maybe everyone else is tired of this
and we should just take your v5 and fix the other stuff later. I'll
leave that up to you / Junio to decide.

>>>  int git_ident_config(const char *var, const char *value, void *data)
>>>  {
>>>  	if (!strcmp(var, "user.useconfigonly")) {
>>> @@ -480,29 +551,7 @@ int git_ident_config(const char *var, const char *value, void *data)
>>>  		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;
>>> -	}
>>> -
>>> -	return 0;
>>> +	return set_ident(var, value);
>>>  }

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06  9:28       ` Ævar Arnfjörð Bjarmason
@ 2019-02-06 18:26         ` Jeff King
  2019-02-06 18:41           ` William Hubbs
  2019-02-06 22:43           ` Junio C Hamano
  2019-02-06 18:34         ` William Hubbs
  1 sibling, 2 replies; 23+ messages in thread
From: Jeff King @ 2019-02-06 18:26 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, git, William Hubbs, chutzpah

On Wed, Feb 06, 2019 at 10:28:34AM +0100, Ævar Arnfjörð Bjarmason wrote:

> I did some further digging. One of the confusing things is that we've
> been carrying dead code since 2012 to set this
> author_ident_explicitly_given variable. We can just apply this on top of
> master:
> [...]
>     @@ -434,6 +432,2 @@ 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"),

Yeah, that would be OK with me. It's conceivable somebody ask "was the author
ident sufficiently given", but given that 7 years have passed, it seems
unlikely (and it's easy to resurrect it in the worst case).

But...

> A more complete "fix" is to entirely revert Jeff's d6991ceedc ("ident:
> keep separate "explicit" flags for author and committer",
> 2012-11-14). As he noted in 2012
> (https://public-inbox.org/git/20121128182534.GA21020@sigill.intra.peff.net/):
> 
>     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.

I think that reintroduces some oddness. E.g., if I don't have any ident
information set in config or the environment, and I do:

  GIT_AUTHOR_NAME=me GIT_AUTHOR_EMAIL=me@example.com git commit ...

that shouldn't count as "committer ident sufficiently given", and should
still give a warning. So we wouldn't want to conflate them in a single
flag (which is what d6991ceedc fixed). Curiously, though, I'm not sure
you can trigger the problem through git-commit. It does call
committer_ident_sufficiently_given(), but it never calls
git_author_info(), where we set the flags!

Instead, it does its own parse via determine_author_info(), which does
not bother to set the "explicit" flag at all. I suspect this could be
refactored share more code with git_author_info() (which is what the
plumbing commit-tree uses). But that's all a side note here.

There is one other call to check that the committer ident is
sufficiently given, and that's in sequencer.c, when it prints a picked
commit's info. That _might_ be triggerable (it doesn't call
git_author_info() in that code path, but do_merge() does, so if the two
happen in the same process, you'd not see the "Committer:" info line
when you should).

So the bugs are minor and fairly unlikely. But I do think it's worth
keeping the flags separate (even if we don't bother carrying an "author"
one), just because it's an easy mistake to make.

An alternative view is that anybody who calls git_author_info() to
create a commit _should_ be checking author_ident_sufficiently_given(),
and it's a bug that they're not.

I.e., should we be doing something like this (and probably some other
spots, too):

diff --git a/commit.c b/commit.c
index a5333c7ac6..c99b311a48 100644
--- a/commit.c
+++ b/commit.c
@@ -1419,8 +1419,11 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 	}

 	/* Person/date information */
-	if (!author)
+	if (!author) {
 		author = git_author_info(IDENT_STRICT);
+		if (!author_ident_sufficiently_given())
+			warning("your author ident was auto-detected, etc...");
+	}
 	strbuf_addf(&buffer, "author %s\n", author);
 	strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_STRICT));
 	if (!encoding_is_utf8)

I dunno. It seems pretty low priority, and nobody has even noticed after
all these years. So I'm not sure if it's worth spending too much time on
it.

-Peff

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06 18:26         ` Jeff King
@ 2019-02-06 18:41           ` William Hubbs
  2019-02-06 22:43           ` Junio C Hamano
  1 sibling, 0 replies; 23+ messages in thread
From: William Hubbs @ 2019-02-06 18:41 UTC (permalink / raw)
  To: Jeff King
  Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, git,
	chutzpah, williamh

On Wed, Feb 06, 2019 at 01:26:12PM -0500, Jeff King wrote:
> On Wed, Feb 06, 2019 at 10:28:34AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> > I did some further digging. One of the confusing things is that we've
> > been carrying dead code since 2012 to set this
> > author_ident_explicitly_given variable. We can just apply this on top of
> > master:
> > [...]
> >     @@ -434,6 +432,2 @@ 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"),
> 
> Yeah, that would be OK with me. It's conceivable somebody ask "was the author
> ident sufficiently given", but given that 7 years have passed, it seems
> unlikely (and it's easy to resurrect it in the worst case).
> 
> But...
> 
> > A more complete "fix" is to entirely revert Jeff's d6991ceedc ("ident:
> > keep separate "explicit" flags for author and committer",
> > 2012-11-14). As he noted in 2012
> > (https://public-inbox.org/git/20121128182534.GA21020@sigill.intra.peff.net/):
> > 
> >     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.
> 
> I think that reintroduces some oddness. E.g., if I don't have any ident
> information set in config or the environment, and I do:
> 
>   GIT_AUTHOR_NAME=me GIT_AUTHOR_EMAIL=me@example.com git commit ...
> 
> that shouldn't count as "committer ident sufficiently given", and should
> still give a warning. So we wouldn't want to conflate them in a single
> flag (which is what d6991ceedc fixed). Curiously, though, I'm not sure
> you can trigger the problem through git-commit. It does call
> committer_ident_sufficiently_given(), but it never calls
> git_author_info(), where we set the flags!
> 
> Instead, it does its own parse via determine_author_info(), which does
> not bother to set the "explicit" flag at all. I suspect this could be
> refactored share more code with git_author_info() (which is what the
> plumbing commit-tree uses). But that's all a side note here.
> 
> There is one other call to check that the committer ident is
> sufficiently given, and that's in sequencer.c, when it prints a picked
> commit's info. That _might_ be triggerable (it doesn't call
> git_author_info() in that code path, but do_merge() does, so if the two
> happen in the same process, you'd not see the "Committer:" info line
> when you should).
> 
> So the bugs are minor and fairly unlikely. But I do think it's worth
> keeping the flags separate (even if we don't bother carrying an "author"
> one), just because it's an easy mistake to make.
> 
> An alternative view is that anybody who calls git_author_info() to
> create a commit _should_ be checking author_ident_sufficiently_given(),
> and it's a bug that they're not.
> 
> I.e., should we be doing something like this (and probably some other
> spots, too):
> 
> diff --git a/commit.c b/commit.c
> index a5333c7ac6..c99b311a48 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1419,8 +1419,11 @@ int commit_tree_extended(const char *msg, size_t msg_len,
>  	}
>  
>  	/* Person/date information */
> -	if (!author)
> +	if (!author) {
>  		author = git_author_info(IDENT_STRICT);
> +		if (!author_ident_sufficiently_given())
> +			warning("your author ident was auto-detected, etc...");
> +	}
>  	strbuf_addf(&buffer, "author %s\n", author);
>  	strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_STRICT));
>  	if (!encoding_is_utf8)
> 
> I dunno. It seems pretty low priority, and nobody has even noticed after
> all these years. So I'm not sure if it's worth spending too much time on
> it.

Given this info (which came in while I was writing my last email), I
would rather see my v5 patch get in then we fix everything else later.

Junio, what do you think?

Thanks,

William


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06 18:26         ` Jeff King
  2019-02-06 18:41           ` William Hubbs
@ 2019-02-06 22:43           ` Junio C Hamano
  1 sibling, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2019-02-06 22:43 UTC (permalink / raw)
  To: Jeff King
  Cc: Ævar Arnfjörð Bjarmason, git, William Hubbs,
	chutzpah

Jeff King <peff@peff.net> writes:

> An alternative view is that anybody who calls git_author_info() to
> create a commit _should_ be checking author_ident_sufficiently_given(),
> and it's a bug that they're not.
>
> I.e., should we be doing something like this (and probably some other
> spots, too):
>
> diff --git a/commit.c b/commit.c
> index a5333c7ac6..c99b311a48 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1419,8 +1419,11 @@ int commit_tree_extended(const char *msg, size_t msg_len,
>  	}
>  
>  	/* Person/date information */
> -	if (!author)
> +	if (!author) {
>  		author = git_author_info(IDENT_STRICT);
> +		if (!author_ident_sufficiently_given())
> +			warning("your author ident was auto-detected, etc...");
> +	}
>  	strbuf_addf(&buffer, "author %s\n", author);
>  	strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_STRICT));
>  	if (!encoding_is_utf8)
>
> I dunno. It seems pretty low priority, and nobody has even noticed after
> all these years. So I'm not sure if it's worth spending too much time on
> it.

That's quite tempting.  But I agree that this is something we can
leave for a later clean-up, as the topic to add the config variables
is pretty much orthogonal to it, and we are not making things that
much worse than the status quo.

Thanks.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-06  9:28       ` Ævar Arnfjörð Bjarmason
  2019-02-06 18:26         ` Jeff King
@ 2019-02-06 18:34         ` William Hubbs
  1 sibling, 0 replies; 23+ messages in thread
From: William Hubbs @ 2019-02-06 18:34 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, git, chutzpah, Jeff King, williamh

On Wed, Feb 06, 2019 at 10:28:34AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> William, is that something you're intererested in carrying forward? I
> can also help if you want. Sorry your first contribution to git has
> turned into this mess of re-rolls, as often happens we find that when
> trying to tweak something that the existing behavior doesn't have any
> tests.
 
 I'm open to it, but since we aren't sure about reverting the patch I
 guess we should wait for Jeff to reply?

> I think it's worth spending a bit more time here to prove to ourselves
> that the changes aren't e.g. spamming users with the ident advice in
> cases where we don't want that, but maybe everyone else is tired of this
> and we should just take your v5 and fix the other stuff later. I'll
> leave that up to you / Junio to decide.

Junio,

I guess it's up to you, should we take my v5 patch then do the other
fixes later?

William


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
  2019-02-05 19:52 ` [PATCH v6 2/2] config: allow giving separate author and committer idents Ævar Arnfjörð Bjarmason
  2019-02-05 20:22   ` Junio C Hamano
@ 2019-04-15 14:24   ` Derrick Stolee
  1 sibling, 0 replies; 23+ messages in thread
From: Derrick Stolee @ 2019-04-15 14:24 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, William Hubbs, chutzpah

On 2/5/2019 2:52 PM, Ævar Arnfjörð Bjarmason wrote:
> From: William Hubbs <williamh@gentoo.org>
> -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);
>  }

William and Ævar,

The "WANT_AUTHOR_IDENT" block of this switch statement does not
appear to be hit by any tests, despite the tests included in this
patch. My guess is that it is ignored because we have the following
code in builtin/commit.c:

static void determine_author_info(struct strbuf *author_ident)
{
	char *name, *email, *date;
	struct ident_split author;

	name = xstrdup_or_null(getenv("GIT_AUTHOR_NAME"));
	email = xstrdup_or_null(getenv("GIT_AUTHOR_EMAIL"));
	date = xstrdup_or_null(getenv("GIT_AUTHOR_DATE"));
...


This is likely overriding the need to use fmt_name. Should we
de-duplicate this use of the environment variable by using your
new method at this spot in builtin/commit.c?

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2019-04-15 14:24 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-04 18:48 [PATCH v5 0/1] config: allow giving separate author and committer William Hubbs
2019-02-04 18:48 ` [PATCH v5 1/1] config: allow giving separate author and committer idents William Hubbs
2019-02-05  9:16   ` Johannes Schindelin
2019-02-05 18:02     ` Junio C Hamano
2019-02-05 19:52 ` [PATCH v6 0/2] New {author,committer}.{name,email} config Ævar Arnfjörð Bjarmason
2019-02-05 19:52 ` [PATCH v6 1/2] ident: test how GIT_* and user.{name,email} interact Ævar Arnfjörð Bjarmason
2019-02-05 19:52 ` [PATCH v6 2/2] config: allow giving separate author and committer idents Ævar Arnfjörð Bjarmason
2019-02-05 20:22   ` Junio C Hamano
2019-02-05 21:14     ` Ævar Arnfjörð Bjarmason
2019-02-06  0:04       ` William Hubbs
2019-02-06  0:15         ` William Hubbs
2019-02-06  1:05           ` William Hubbs
2019-02-06  5:03         ` Junio C Hamano
2019-02-13 16:43           ` William Hubbs
2019-02-13 22:37             ` Junio C Hamano
2019-02-14 18:17               ` William Hubbs
2019-02-06  8:58         ` Ævar Arnfjörð Bjarmason
2019-02-06  9:28       ` Ævar Arnfjörð Bjarmason
2019-02-06 18:26         ` Jeff King
2019-02-06 18:41           ` William Hubbs
2019-02-06 22:43           ` Junio C Hamano
2019-02-06 18:34         ` William Hubbs
2019-04-15 14:24   ` Derrick Stolee

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).