[PATCH 0/1] add author and committer configuration settings

[PATCH 0/1] add author and committer configuration settings

[William Hubbs]

Hi all,

this is my first patch for git, so please be gentle. ;-)

I am in a situation where I need to use different authorship information
for some repositories I commit to.

Git already supports setting different authorship and committer
information with environment variables, but this setting is global so if
you want to change it per repository you need to use a separate tool.

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

Thanks much for your reviews, and I would like to publically thank dscho
from #git-devel for assisting me in preparing this patch.

Also, since I use a screen reader, it would be very helpful if you put
your comments in your replies as close to the affected code as possible,
preferably directly below it.

William Hubbs (1):
  Add author and committer configuration settings

 Documentation/config/user.txt | 20 +++++++++++
 builtin/commit.c              |  2 +-
 cache.h                       |  5 ++-
 config.c                      |  6 ++++
 ident.c                       | 68 ++++++++++++++++++++++++++++++++---
 log-tree.c                    |  3 +-
 sequencer.c                   |  3 +-
 7 files changed, 97 insertions(+), 10 deletions(-)

-- 
2.19.2

[PATCH 1/1] Add author and committer configuration settings

[William Hubbs]

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

Signed-off-by: William Hubbs <williamh@gentoo.org>
---
 Documentation/config/user.txt | 20 +++++++++++
 builtin/commit.c              |  2 +-
 cache.h                       |  5 ++-
 config.c                      |  6 ++++
 ident.c                       | 68 ++++++++++++++++++++++++++++++++---
 log-tree.c                    |  3 +-
 sequencer.c                   |  3 +-
 7 files changed, 97 insertions(+), 10 deletions(-)

diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index b5b2ba1199..6ba7002252 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -1,3 +1,23 @@
+author.email::
+Your email address to be recorded on the author line of any newly
+created commits.
+If this is not set, we use user.email.
+
+author.name::
+Your full name to be recorded on the author line of any newly
+created commits.
+If this is not set, we use user.name.
+
+committer.email::
+Your email address to be recorded on the committer line of any newly
+created commits.
+If this is not set, we use user.email.
+
+committer.name::
+Your full name to be recorded on the committer line of any newly
+created commits.
+If this is not set, we use user.name.
+
 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
diff --git a/builtin/commit.c b/builtin/commit.c
index c021b119bb..49a97adeb8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -607,7 +607,7 @@ 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, date, IDENT_STRICT|IDENT_AUTHOR));
 	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 ca36b44ee0..0ee87f22a9 100644
--- a/cache.h
+++ b/cache.h
@@ -1479,10 +1479,13 @@ int date_overflows(timestamp_t date);
 #define IDENT_STRICT	       1
 #define IDENT_NO_DATE	       2
 #define IDENT_NO_NAME	       4
+#define IDENT_AUTHOR          8
+#define IDENT_COMMITTER       16
+
 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_committer_name(void);
 extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
diff --git a/config.c b/config.c
index ff521eb27a..4bd5920dea 100644
--- a/config.c
+++ b/config.c
@@ -1484,6 +1484,12 @@ int git_default_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (starts_with(var, "author."))
+		return git_ident_config(var, value, cb);
+
+	if (starts_with(var, "committer."))
+		return git_ident_config(var, value, cb);
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/ident.c b/ident.c
index 33bcf40644..3da96ebbef 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;
 
@@ -361,7 +365,15 @@ const char *fmt_ident(const char *name, const char *email,
 	int strict = (flag & IDENT_STRICT);
 	int want_date = !(flag & IDENT_NO_DATE);
 	int want_name = !(flag & IDENT_NO_NAME);
+	int want_author = (flag & IDENT_AUTHOR);
+	int want_committer = (flag & IDENT_COMMITTER);
 
+	if (!email) {
+		if (want_author && git_author_email.len)
+			email = git_author_email.buf;
+		else if (want_committer && 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 +389,12 @@ const char *fmt_ident(const char *name, const char *email,
 
 	if (want_name) {
 		int using_default = 0;
+		if (!name) {
+			if (want_author && git_author_name.len)
+				name = git_author_name.buf;
+			else if (want_committer && 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,11 @@ 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_committer_name(void)
 {
-	return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE);
+	char *name = getenv("GIT_COMMITTER_NAME");
+	char *email = getenv("GIT_COMMITTER_EMAIL");
+	return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE|IDENT_COMMITTER);
 }
 
 const char *git_author_info(int flag)
@@ -439,7 +459,7 @@ const char *git_author_info(int flag)
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 			 getenv("GIT_AUTHOR_EMAIL"),
 			 getenv("GIT_AUTHOR_DATE"),
-			 flag);
+			 flag|IDENT_AUTHOR);
 }
 
 const char *git_committer_info(int flag)
@@ -451,7 +471,7 @@ const char *git_committer_info(int flag)
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
 			 getenv("GIT_COMMITTER_DATE"),
-			 flag);
+			 flag|IDENT_COMMITTER);
 }
 
 static int ident_is_sufficient(int user_ident_explicitly_given)
@@ -480,6 +500,46 @@ int git_ident_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 
+	if (!strcmp(var, "author.name")) {
+		if (!value)
+			return config_error_nonbool(var);
+		strbuf_reset(&git_author_name);
+		strbuf_addstr(&git_author_name, value);
+		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		ident_config_given |= IDENT_NAME_GIVEN;
+		return 0;
+	}
+
+	if (!strcmp(var, "author.email")) {
+		if (!value)
+			return config_error_nonbool(var);
+		strbuf_reset(&git_author_email);
+		strbuf_addstr(&git_author_email, value);
+		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		ident_config_given |= IDENT_MAIL_GIVEN;
+		return 0;
+	}
+
+	if (!strcmp(var, "committer.name")) {
+		if (!value)
+			return config_error_nonbool(var);
+		strbuf_reset(&git_committer_name);
+		strbuf_addstr(&git_committer_name, value);
+		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		ident_config_given |= IDENT_NAME_GIVEN;
+		return 0;
+	}
+
+	if (!strcmp(var, "committer.email")) {
+		if (!value)
+			return config_error_nonbool(var);
+		strbuf_reset(&git_committer_email);
+		strbuf_addstr(&git_committer_email, value);
+		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		ident_config_given |= IDENT_MAIL_GIVEN;
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/log-tree.c b/log-tree.c
index 10680c139e..6760a2e9c4 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_committer_name());
 	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 e1a4dd15f1..f357defda5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4036,8 +4036,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_committer_name());
 	strbuf_addch(&sob, '\n');
 
 	if (!ignore_footer)
-- 
2.19.2

Re: [PATCH 1/1] Add author and committer configuration settings

[John Passaro]

On Wed, Dec 19, 2018 at 2:19 PM William Hubbs <williamh@gentoo.org> wrote:
>
>     The author.email, author.name, committer.email and committer.name
>     settings are analogous to the GIT_AUTHOR_* and GIT_COMMITTER_*
>     environment variables, but for the git config system. This allows them
>     to be set separately for each repository.
>

Great! I didn't realize this wasn't already supported...

However your patch does seem to be missing tests.
t/t7517-per-repo-email.sh would appear to be a logical place to add
them.

> Signed-off-by: William Hubbs <williamh@gentoo.org>
> ---
>  Documentation/config/user.txt | 20 +++++++++++
>  builtin/commit.c              |  2 +-
>  cache.h                       |  5 ++-
>  config.c                      |  6 ++++
>  ident.c                       | 68 ++++++++++++++++++++++++++++++++---
>  log-tree.c                    |  3 +-
>  sequencer.c                   |  3 +-
>  7 files changed, 97 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> index b5b2ba1199..6ba7002252 100644
> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -1,3 +1,23 @@
> +author.email::
> +Your email address to be recorded on the author line of any newly
> +created commits.
> +If this is not set, we use user.email.
> +
> +author.name::
> +Your full name to be recorded on the author line of any newly
> +created commits.
> +If this is not set, we use user.name.
> +
> +committer.email::
> +Your email address to be recorded on the committer line of any newly
> +created commits.
> +If this is not set, we use user.email.
> +
> +committer.name::
> +Your full name to be recorded on the committer line of any newly
> +created commits.
> +If this is not set, we use user.name.
> +
>  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

I think it would be wise to mention the new config items under
user.email and user.name as well.

Re: [PATCH 0/1] add author and committer configuration settings

[Jonathan Nieder]

Hi,

William Hubbs wrote:

> this is my first patch for git, so please be gentle. ;-)

Thanks for contributing!

> I am in a situation where I need to use different authorship information
> for some repositories I commit to.
>
> Git already supports setting different authorship and committer
> information with environment variables, but this setting is global so if
> you want to change it per repository you need to use a separate tool.
>
> This patch adds support to git config for author.email, author.name,
> committer.email and committer.name  settings so this information
> can be set per repository. It applies to current master.

The above information (except for "It applies to current master") is
very useful to have when looking back at the change in history.  When
sending the next version of this patch in response to others'
comments, can you replace the commit message with something like it?

In other words, it is very useful for the commit message to include
this kind of information about the "why" behind a change, beyond the
"what" that the patch itself already provides.

> Thanks much for your reviews, and I would like to publically thank dscho
> from #git-devel for assisting me in preparing this patch.
>
> Also, since I use a screen reader, it would be very helpful if you put
> your comments in your replies as close to the affected code as possible,
> preferably directly below it.

Fortunately, this is already common practice here, but the reminder is
very welcome.

By the way, if you have other feedback about Git accessibility through
a screen reader (both the project and the tool), I would be very
interested to hear.  Presumably in a new thread. :)

Thanks and hope that helps,
Jonathan

Re: [PATCH 1/1] Add author and committer configuration settings

[Johannes Schindelin]

Hi William,

thank you for putting this together.

On Wed, 19 Dec 2018, William Hubbs wrote:

> -extern const char *fmt_name(const char *name, const char *email);
> +extern const char *fmt_committer_name(void);

If it would not be too much trouble for you, could I ask you to split this
change out into a separate commit (which would be the first of now two
patches)? It could have a commit message like this:

	ident: rename fmt_name() to fmt_committer_name()

	Ever since 4c28e4ada03f (commit: die before asking to edit the log
	message, 2010-12-20), all remaining callers of that function want
	to format the committer name. To simplify the code, therefore, we
	rename the function and move the getenv() call into it.

Ciao,
Dscho

Re: [PATCH 1/1] Add author and committer configuration settings

[Junio C Hamano]

William Hubbs <williamh@gentoo.org> writes:

> Subject: Re: [PATCH 1/1]     Add author and committer configuration settings

Perhaps something like this

	Subject: config: allow giving separate author and committer idents

would fit better in "git shortlog --no-merges" output.

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

Don't indent the whole proposed log message.

> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> index b5b2ba1199..6ba7002252 100644
> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -1,3 +1,23 @@
> +author.email::
> +Your email address to be recorded on the author line of any newly
> +created commits.
> +If this is not set, we use user.email.

"author line" is a bit too technical and appropriate only to those
who are familiar with "git cat-file commit" output.  How about
phrasing it a bit differently, like

	author.email::
		The email-address used for the author of newly
		created commits.  Defaults to the value of the
		`GIT_AUTHOR_EMAIL` environment variable, or if it is
		not set, the `user.email` configuration variable.

Likewise for the other three variables.

>  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

As you can see, the enumeration list in this file is formatted by
indenting the definition body.  

> diff --git a/builtin/commit.c b/builtin/commit.c
> index c021b119bb..49a97adeb8 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -607,7 +607,7 @@ 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, date, IDENT_STRICT|IDENT_AUTHOR));

That's now a bit overly long line.

>  	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 ca36b44ee0..0ee87f22a9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1479,10 +1479,13 @@ int date_overflows(timestamp_t date);
>  #define IDENT_STRICT	       1
>  #define IDENT_NO_DATE	       2
>  #define IDENT_NO_NAME	       4
> +#define IDENT_AUTHOR          8
> +#define IDENT_COMMITTER       16

>  extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);

It is wrong to pass "do we want the author, or the committer, name?"
information in the same flag parameter to fmt_ident(), and it is
wrong to introduce IDENT_AUTHOR/COMMITTER bits as if they belong to
the existing four.  For one thing, unlike these other bits, these
two are not independent bits.  It would be an error for a caller to
pass neither bits, or both bits at the same time.

One way to do it better may be to pass it as another parameter, e.g.

	enum want_ident {
		WANT_AUTHOR_IDENT,
		WANT_COMMITTER_IDENT
	};
	const char *fmt_ident(const char *name, const char *email,
				enum want_ident whose_ident,
				const char *date_str, int flags);

In the remainder of the review, I'd give update suggestions based on
this function signature.

> diff --git a/ident.c b/ident.c
> index 33bcf40644..3da96ebbef 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;
>  
> @@ -361,7 +365,15 @@ const char *fmt_ident(const char *name, const char *email,
>  	int strict = (flag & IDENT_STRICT);
>  	int want_date = !(flag & IDENT_NO_DATE);
>  	int want_name = !(flag & IDENT_NO_NAME);
> +	int want_author = (flag & IDENT_AUTHOR);
> +	int want_committer = (flag & IDENT_COMMITTER);
>  
> +	if (!email) {
> +		if (want_author && git_author_email.len)
> +			email = git_author_email.buf;
> +		else if (want_committer && 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 +389,12 @@ const char *fmt_ident(const char *name, const char *email,
>  
>  	if (want_name) {
>  		int using_default = 0;
> +		if (!name) {
> +			if (want_author && git_author_name.len)
> +				name = git_author_name.buf;
> +			else if (want_committer && git_committer_name.len)
> +				name = git_committer_name.buf;
> +		}
>  		if (!name) {
>  			if (strict && ident_use_config_only
>  			    && !(ident_config_given & IDENT_NAME_GIVEN)) {

The reviewer's interest here is to see how "author.name trumps
user.name" precedence is implemented; by mucking with "name" before
the code that deals with the ident_default_name(), which yields
git_default_name taken from user.name, the code gives precedence to
these newly introduced variables.

Which makes sense.

> @@ -425,9 +443,11 @@ 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_committer_name(void)
>  {
> -	return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE);
> +	char *name = getenv("GIT_COMMITTER_NAME");
> +	char *email = getenv("GIT_COMMITTER_EMAIL");
> +	return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE|IDENT_COMMITTER);
>  }

OK, we are lucky that no existing caller use fmt_name() with author
information, I guess?  The resulting codebase does invite a question
"why don't we have fmt_author_name() at all?", which is somewhat
disturbing.

As we are going to change this function *and* all of its callers
anyway, perhaps we can generalize it with minimum effort, like so:

	const char *fmt_name(enum want_ident whose_ident)
	{
		switch (whose_ident) {
		case WANT_AUTHOR_IDENT:
			name = getenv("GIT_AUTHOR_NAME");
			email = getenv("GIT_COMMITTER_NAME");
			break;
		case WANT_COMMITTER_IDENT:
			...
		}
		return fmt_ident(name, email, whose_ident,
				  NULL, IDENT_STRICT | IDENT_NO_DATE);
	}

Thanks.