threads / rfc / 28555

RFC patchident: check /etc/mailname if email is unknown

Subject: [PATCH/RFC] ident: check /etc/mailname if email is unknown

## tl;dr

11 messages between Oct 3, 2011 and Oct 6, 2011. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Jonathan Nieder· Oct 3, 2011, 04:57 UTC · lore
From: Gerrit Pape <pape@smarden.org>

Before falling back to gethostname(), check /etc/mailname if GIT_AUTHOR_EMAIL is not set in the environment or through config files. Only fall back if /etc/mailname cannot be opened or read.

The /etc/mailname convention comes from Debian policy section 11.6 ("mail transport, delivery and user agents"), though it seems more widely useful. The lack of this support was noticed by various people in different ways:

 - Ian observed that git was choosing the address
   'ian@anarres.relativity.greenend.org.uk' rather than
   'ian@davenant.greenend.org.uk' as it should have done.
 - Jonathan noticed that operations like "git commit" were needlessly
   slow when the using a resolver that was slow to handle reverse DNS
   lookups.

On the other hand, this means that if /etc/mailname is set up and the [user] name and email configuration aren't, the committer email will not provide a charming reminder of which machine commits were made on any more. I think that cost is worth it.

Requested-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

Debian has been using something like this patch for years now (see <http://bugs.debian.org/448655>) but I thought it might be useful to others, too. A similar patch visited the list three years ago: <http://thread.gmane.org/gmane.comp.version-control.git/51800>.

Thoughts of all kinds welcome, of course.

Thanks, Jonathan

 Documentation/git-commit-tree.txt |    8 +++++++-
 ident.c                           |   23 +++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletions(-)
Show changes to 2 files +30 −1

Documentation/git-commit-tree.txt, ident.c

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index 0fdb82ee..02133d5f 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -68,7 +68,9 @@ if set:
 
 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
-present, system user name and fully qualified hostname.
+present, system user name and the hostname used for outgoing mail (taken
+from `/etc/mailname` and falling back to the fully qualified hostname when
+that file does not exist).
 
 A commit comment is read from stdin. If a changelog
 entry is not provided via "<" redirection, 'git commit-tree' will just wait
@@ -90,6 +92,10 @@ Discussion
 
 include::i18n.txt[]
 
+FILES
+-----
+/etc/mailname
+
 SEE ALSO
 --------
 linkgit:git-write-tree[1]
diff --git a/ident.c b/ident.c
index 35a6f264..87f0a37e 100644
--- a/ident.c
+++ b/ident.c
@@ -52,6 +52,8 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz)
 
 static void copy_email(const struct passwd *pw)
 {
+	FILE *mailname;
+
 	/*
 	 * Make up a fake email address
 	 * (name + '@' + hostname [+ '.' + domainname])
@@ -61,6 +63,27 @@ static void copy_email(const struct passwd *pw)
 		die("Your sysadmin must hate you!");
 	memcpy(git_default_email, pw->pw_name, len);
 	git_default_email[len++] = '@';
+
+	/*
+	 * The domain part comes from /etc/mailname if it is readable,
+	 * or the current hostname and domain name otherwise.
+	 */
+	mailname = fopen("/etc/mailname", "r");
+	if (!mailname) {
+		if (errno != ENOENT)
+			warning("cannot open /etc/mailname: %s",
+				strerror(errno));
+	} else if (fgets(git_default_email + len,
+			 sizeof(git_default_email) - len, mailname)) {
+		/* success! */
+		fclose(mailname);
+		return;
+	} else {
+		if (ferror(mailname))
+			warning("cannot read /etc/mailname: %s",
+				strerror(errno));
+		fclose(mailname);
+	}
 	gethostname(git_default_email + len, sizeof(git_default_email) - len);
 	if (!strchr(git_default_email+len, '.')) {
 		struct hostent *he = gethostbyname(git_default_email + len);
-- 
1.7.7.rc1
Junio C Hamano· Oct 3, 2011, 05:30 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC] ident: check /etc/mailname if email is unknown

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 37 quoted lines
> @@ -52,6 +52,8 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz)
>  
>  static void copy_email(const struct passwd *pw)
>  {
> +	FILE *mailname;
> +
>  	/*
>  	 * Make up a fake email address
>  	 * (name + '@' + hostname [+ '.' + domainname])
> @@ -61,6 +63,27 @@ static void copy_email(const struct passwd *pw)
>  		die("Your sysadmin must hate you!");
>  	memcpy(git_default_email, pw->pw_name, len);
>  	git_default_email[len++] = '@';
> +
> +	/*
> +	 * The domain part comes from /etc/mailname if it is readable,
> +	 * or the current hostname and domain name otherwise.
> +	 */
> +	mailname = fopen("/etc/mailname", "r");
> +	if (!mailname) {
> +		if (errno != ENOENT)
> +			warning("cannot open /etc/mailname: %s",
> +				strerror(errno));
> +	} else if (fgets(git_default_email + len,
> +			 sizeof(git_default_email) - len, mailname)) {
> +		/* success! */
> +		fclose(mailname);
> +		return;
> +	} else {
> +		if (ferror(mailname))
> +			warning("cannot read /etc/mailname: %s",
> +				strerror(errno));
> +		fclose(mailname);
> +	}
>  	gethostname(git_default_email + len, sizeof(git_default_email) - len);
>  	if (!strchr(git_default_email+len, '.')) {
>  		struct hostent *he = gethostbyname(git_default_email + len);

I do not think this would hurt, even though I see /etc/mailname on only one of my boxes (i.e. Debian). For maintainability for the future, however, I would prefer to see the above hunk separated into a helper function to keep addition to copy_email() to the minimum, e.g.

	memcpy(git_default_email, pw->pw_name, len);
        git_default_email[len++] = '@';

+ if (add_mailname_host(git_default_email, len, sizeof(git_default_email))) + return; /* read from "/etc/mailname" (Debian) */

	gethostname(git_default_email + len, sizeof(git_default_email) - len);
        ...

So that people who care about other distros can more easily add a single implementation to a similar location without making copy_email() too long to lose clarity. The fallback default logic that does gethostname() might also want to become a separate helper function as well.

Jonathan Nieder· Oct 3, 2011, 06:16 UTC · re: Junio C Hamano · lore

[PATCH v2] ident: check /etc/mailname if email is unknown

Before falling back to gethostname(), check /etc/mailname if GIT_AUTHOR_EMAIL is not set in the environment or through config files. Only fall back if /etc/mailname cannot be opened or read.

The /etc/mailname convention comes from Debian policy section 11.6 ("mail transport, delivery and user agents"), though maybe it could be useful sometimes on other machines, too. The lack of this support was noticed by various people in different ways:

 - Ian observed that git was choosing the address
   'ian@anarres.relativity.greenend.org.uk' rather than
   'ian@davenant.greenend.org.uk' as it should have done.
 - Jonathan noticed that operations like "git commit" were needlessly
   slow when using a resolver that was slow to handle reverse DNS
   lookups.

Alas, after this patch, if /etc/mailname is set up and the [user] name and email configuration aren't, the committer email will not provide a charming reminder of which machine commits were made on any more. But I think it's worth it.

Mechanics: the functionality of reading mailname goes in its own
function, so people who care about other distros can easily add an
implementation to a similar location without making copy_email() too
long and losing clarity.  While at it, we split out the fallback
default logic that does gethostname(), too (rearranging it a little
and adding a check for errors from gethostname while at it).
Based on a patch by Gerrit Pape <pape@smarden.org>.
Requested-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano wrote:
> I do not think this would hurt, even though I see /etc/mailname on only
> one of my boxes (i.e. Debian). For maintainability for the future,
> however, I would prefer to see the above hunk separated into a helper
> function to keep addition to copy_email() to the minimum, e.g.
Makes sense.  How about this?
 Documentation/git-commit-tree.txt |    8 ++++-
 ident.c                           |   66 +++++++++++++++++++++++++++++-------
 2 files changed, 60 insertions(+), 14 deletions(-)
Show changes to 2 files +60 −14

Documentation/git-commit-tree.txt, ident.c

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index 0fdb82ee..02133d5f 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -68,7 +68,9 @@ if set:
 
 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
-present, system user name and fully qualified hostname.
+present, system user name and the hostname used for outgoing mail (taken
+from `/etc/mailname` and falling back to the fully qualified hostname when
+that file does not exist).
 
 A commit comment is read from stdin. If a changelog
 entry is not provided via "<" redirection, 'git commit-tree' will just wait
@@ -90,6 +92,10 @@ Discussion
 
 include::i18n.txt[]
 
+FILES
+-----
+/etc/mailname
+
 SEE ALSO
 --------
 linkgit:git-write-tree[1]
diff --git a/ident.c b/ident.c
index 35a6f264..edb43144 100644
--- a/ident.c
+++ b/ident.c
@@ -50,6 +50,54 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz)
 
 }
 
+static int add_mailname_host(char *buf, size_t len)
+{
+	FILE *mailname;
+
+	mailname = fopen("/etc/mailname", "r");
+	if (!mailname) {
+		if (errno != ENOENT)
+			warning("cannot open /etc/mailname: %s",
+				strerror(errno));
+		return -1;
+	}
+	if (!fgets(buf, len, mailname)) {
+		if (ferror(mailname))
+			warning("cannot read /etc/mailname: %s",
+				strerror(errno));
+		fclose(mailname);
+		return -1;
+	}
+	/* success! */
+	fclose(mailname);
+	return 0;
+}
+
+static void add_domainname(char *buf, size_t len)
+{
+	struct hostent *he;
+	size_t namelen;
+	const char *domainname;
+
+	if (gethostname(buf, len)) {
+		warning("cannot get host name: %s", strerror(errno));
+		strlcpy(buf, "(none)", len);
+		return;
+	}
+	namelen = strlen(buf);
+	if (memchr(buf, '.', namelen))
+		return;
+
+	he = gethostbyname(buf);
+	buf[namelen++] = '.';
+	buf += namelen;
+	len -= namelen;
+	if (he && (domainname = strchr(he->h_name, '.')))
+		strlcpy(buf, domainname + 1, len);
+	else
+		strlcpy(buf, "(none)", len);
+}
+
 static void copy_email(const struct passwd *pw)
 {
 	/*
@@ -61,20 +109,12 @@ static void copy_email(const struct passwd *pw)
 		die("Your sysadmin must hate you!");
 	memcpy(git_default_email, pw->pw_name, len);
 	git_default_email[len++] = '@';
-	gethostname(git_default_email + len, sizeof(git_default_email) - len);
-	if (!strchr(git_default_email+len, '.')) {
-		struct hostent *he = gethostbyname(git_default_email + len);
-		char *domainname;
 
-		len = strlen(git_default_email);
-		git_default_email[len++] = '.';
-		if (he && (domainname = strchr(he->h_name, '.')))
-			strlcpy(git_default_email + len, domainname + 1,
-				sizeof(git_default_email) - len);
-		else
-			strlcpy(git_default_email + len, "(none)",
-				sizeof(git_default_email) - len);
-	}
+	if (!add_mailname_host(git_default_email + len,
+				sizeof(git_default_email) - len))
+		return;	/* read from "/etc/mailname" (Debian) */
+	add_domainname(git_default_email + len,
+			sizeof(git_default_email) - len);
 }
 
 static void setup_ident(void)
-- 
1.7.7.rc1
Johannes Sixt· Oct 3, 2011, 07:09 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] ident: check /etc/mailname if email is unknown

Am 10/3/2011 8:16, schrieb Jonathan Nieder:
Show 9 quoted lines
> +static int add_mailname_host(char *buf, size_t len)
> +{
> +	FILE *mailname;
> +
> +	mailname = fopen("/etc/mailname", "r");
> +	if (!mailname) {
> +		if (errno != ENOENT)
> +			warning("cannot open /etc/mailname: %s",
> +				strerror(errno));
This warns on EACCES. Is that OK? (Just asking, I have no opinion.)
-- Hannes
Jonathan Nieder· Oct 3, 2011, 07:44 UTC · re: Johannes Sixt · lore

Re: [PATCH v2] ident: check /etc/mailname if email is unknown

Johannes Sixt wrote:
> This warns on EACCES. Is that OK? (Just asking, I have no opinion.)

Good catch. I was worried for a moment: could some command call copy_email() in a loop, producing an annoyingly redundant stream of warnings?

Luckily setup_ident() checks if git_default_email has been set to save the trouble of computing it again. So the behavior is to warn exactly once when using a command that uses an ident string, which is still more often than ideal. It would be better to only warn if the permissions are creating an actual problem, so the user can (1) complain to the sysadmin and then (2) set an email address in ~/.gitconfig and move on.

We _could_ add parameters to setup_ident() to only warn if /etc/mailname was going to be used to produce the committer ident (or whichever ident is checked first). That would be confusing if GIT_COMMITTER_EMAIL is set and GIT_AUTHOR_EMAIL is not.

In the long term it would be nice to find a way to warn when the mailname we tried to retrieve was actually going to be used, but short of that, the least confusing behavior is to just not warn at all.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 ident.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)
Show changes to ident.c +1 −9
diff --git a/ident.c b/ident.c
index edb43144..6f5c885d 100644
--- a/ident.c
+++ b/ident.c
@@ -55,16 +55,9 @@ static int add_mailname_host(char *buf, size_t len)
 	FILE *mailname;
 
 	mailname = fopen("/etc/mailname", "r");
-	if (!mailname) {
-		if (errno != ENOENT)
-			warning("cannot open /etc/mailname: %s",
-				strerror(errno));
+	if (!mailname)
 		return -1;
-	}
 	if (!fgets(buf, len, mailname)) {
-		if (ferror(mailname))
-			warning("cannot read /etc/mailname: %s",
-				strerror(errno));
 		fclose(mailname);
 		return -1;
 	}
@@ -80,7 +73,6 @@ static void add_domainname(char *buf, size_t len)
 	const char *domainname;
 
 	if (gethostname(buf, len)) {
-		warning("cannot get host name: %s", strerror(errno));
 		strlcpy(buf, "(none)", len);
 		return;
 	}
-- 
1.7.7.rc1
Junio C Hamano· Oct 3, 2011, 19:13 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] ident: check /etc/mailname if email is unknown

Jonathan Nieder <jrnieder@gmail.com> writes:
> In the long term it would be nice to find a way to warn when the
> mailname we tried to retrieve was actually going to be used, but short
> of that, the least confusing behavior is to just not warn at all.

I would think that if we widely advertise that we read from /etc/mailname if/when it exists, a user who wonders why it is not used would appreciate it if Git tells them why when it cannot use it.

But I agree that has to happen _after_ we flip the logic so that setup_ident() does not trigger when the user gave us the necessary information.

I wonder if that "flipping the logic" would be as simple as something like this. It probably is about time for us to stop using the static array that is hardwired MAX_GITNAME bytes long and start using strbuf, so I didn't bother with strlcpy().

 ident.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Show changes to ident.c +4 −0
diff --git a/ident.c b/ident.c
index edb4314..9cc55f7 100644
--- a/ident.c
+++ b/ident.c
@@ -239,6 +239,10 @@ const char *fmt_ident(const char *name, const char *email,
 	int warn_on_no_name = (flag & IDENT_WARN_ON_NO_NAME);
 	int name_addr_only = (flag & IDENT_NO_DATE);
 
+	if (name && !git_default_name[0])
+		strcpy(git_default_name, name);
+	if (email && !git_default_email[0])
+		strcpy(git_default_email, email);
 	setup_ident();
 	if (!name)
 		name = git_default_name;
Jonathan Nieder· Oct 6, 2011, 17:17 UTC · re: Junio C Hamano · lore

[PATCH/RFC jn/ident-from-etc-mailname] ident: do not retrieve default ident when unnecessary

Avoid a getpwuid() call (which contacts the network if the password database is not local), read of /etc/mailname, gethostname() call, and reverse DNS lookup if the user has already chosen a name and email through configuration, the environment, or the command line.

This should slightly speed up commands like "git commit". More importantly, it improves error reporting when computation of the default ident string does not go smoothly. For example, after detecting a problem (e.g., "warning: cannot open /etc/mailname: Permission denied") in retrieving the default committer identity:

	touch /etc/mailname;	# as root
	chmod -r /etc/mailname;	# as root
	git commit -m 'test commit'

you can squelch the warning while waiting for your sysadmin to fix the permissions problem.

	echo '[user] email = me@example.com' >>~/.gitconfig
Inspired-by: Johannes Sixt <j6t@kdgb.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
Show 10 quoted lines
> --- a/ident.c
> +++ b/ident.c
> @@ -239,6 +239,10 @@ const char *fmt_ident(const char *name, const char *email,
>  	int warn_on_no_name = (flag & IDENT_WARN_ON_NO_NAME);
>  	int name_addr_only = (flag & IDENT_NO_DATE);
>  
> +	if (name && !git_default_name[0])
> +		strcpy(git_default_name, name);
> +	if (email && !git_default_email[0])
> +		strcpy(git_default_email, email);

That poisons the "git_default_foo" variables for the next fmt_ident call. But something similar should work.

 ident.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
Show changes to ident.c +8 −8
diff --git a/ident.c b/ident.c
index edb43144..f619619b 100644
--- a/ident.c
+++ b/ident.c
@@ -117,19 +117,21 @@ static void copy_email(const struct passwd *pw)
 			sizeof(git_default_email) - len);
 }
 
-static void setup_ident(void)
+static void setup_ident(const char **name, const char **emailp)
 {
 	struct passwd *pw = NULL;
 
 	/* Get the name ("gecos") */
-	if (!git_default_name[0]) {
+	if (!*name && !git_default_name[0]) {
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
 		copy_gecos(pw, git_default_name, sizeof(git_default_name));
 	}
+	if (!*name)
+		*name = git_default_name;
 
-	if (!git_default_email[0]) {
+	if (!*emailp && !git_default_email[0]) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
@@ -144,6 +146,8 @@ static void setup_ident(void)
 			copy_email(pw);
 		}
 	}
+	if (!*emailp)
+		*emailp = git_default_email;
 
 	/* And set the default date */
 	if (!git_default_date[0])
@@ -239,11 +243,7 @@ const char *fmt_ident(const char *name, const char *email,
 	int warn_on_no_name = (flag & IDENT_WARN_ON_NO_NAME);
 	int name_addr_only = (flag & IDENT_NO_DATE);
 
-	setup_ident();
-	if (!name)
-		name = git_default_name;
-	if (!email)
-		email = git_default_email;
+	setup_ident(&name, &email);
 
 	if (!*name) {
 		struct passwd *pw;
-- 
1.7.7.rc1
Junio C Hamano· Oct 6, 2011, 18:19 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC jn/ident-from-etc-mailname] ident: do not retrieve default ident when unnecessary

Jonathan Nieder <jrnieder@gmail.com> writes:
> Avoid a getpwuid() call (which contacts the network if the password
> database is not local), read of /etc/mailname, gethostname() call, and
> reverse DNS lookup if the user has already chosen a name and email
> through configuration, the environment, or the command line.
Oh boy that is a hard to parse paragraph that took me three reads.

In any case, both the motivation of the patch and the change itself make sense to me. Thanks.

Jonathan Nieder· Oct 6, 2011, 18:48 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC jn/ident-from-etc-mailname] ident: do not retrieve default ident when unnecessary

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>> Avoid a getpwuid() call (which contacts the network if the password
>> database is not local), read of /etc/mailname, gethostname() call, and
>> reverse DNS lookup if the user has already chosen a name and email
>> through configuration, the environment, or the command line.
>
> Oh boy that is a hard to parse paragraph that took me three reads.
Here's a possible replacement:
 Avoid looking up the current user's password database entry (which
 might be on another machine) and the current machine's domain name for
 outgoing mail (which can involve a reverse DNS lookup) when the
 environment, configuration, or command line specifies a name and email
 that would override them.

Or it might make sense to drop that paragraph altogether, since the subject line already says as much.

Thanks for looking it over.

Sleepily, Jonathan

Ian Jackson· Oct 3, 2011, 11:32 UTC · re: Johannes Sixt · lore

Re: [PATCH v2] ident: check /etc/mailname if email is unknown

Johannes Sixt writes ("Re: [PATCH v2] ident: check /etc/mailname if email is unknown"):
Show 12 quoted lines
> Am 10/3/2011 8:16, schrieb Jonathan Nieder:
> > +static int add_mailname_host(char *buf, size_t len)
> > +{
> > +	FILE *mailname;
> > +
> > +	mailname = fopen("/etc/mailname", "r");
> > +	if (!mailname) {
> > +		if (errno != ENOENT)
> > +			warning("cannot open /etc/mailname: %s",
> > +				strerror(errno));
> 
> This warns on EACCES. Is that OK? (Just asking, I have no opinion.)

I think that's correct. Personally I'm a bit of an error handling fascist and I would have it crash on EACCES but that's probably a bit harsh.

Certainly this file ought to be generally readable, if it exists.
Ian.
Junio C Hamano· Oct 3, 2011, 19:15 UTC · re: Ian Jackson · lore

Re: [PATCH v2] ident: check /etc/mailname if email is unknown

Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
Show 10 quoted lines
>> > +	if (!mailname) {
>> > +		if (errno != ENOENT)
>> > +			warning("cannot open /etc/mailname: %s",
>> > +				strerror(errno));
>> 
>> This warns on EACCES. Is that OK? (Just asking, I have no opinion.)
>
> I think that's correct.  Personally I'm a bit of an error handling
> fascist and I would have it crash on EACCES but that's probably a bit
> harsh.

It is not just harsh but is outright wrong if the platform is not Debian and the platform happens to use the file for other purposes that does not require normal users to be able to read the file.

It _might_ make sense to do
        #ifndef DEBIAN
        #define add_mailname_host(x,y) (-1)
        #else
        static int add_mailname_host(char *buf, size_t len)
        {
                ...
        }
        #endif

← back to recent threads