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

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

[Jonathan Nieder]

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(-)

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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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

[Jonathan Nieder]

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(-)

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

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

[Johannes Sixt]

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

-- Hannes

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

[Jonathan Nieder]

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(-)

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

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

[Ian Jackson]

Johannes Sixt writes ("Re: [PATCH v2] ident: check /etc/mailname if email is unknown"):
> 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.

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

[Junio C Hamano]

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(-)

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;

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

[Junio C Hamano]

Ian Jackson <ijackson@chiark.greenend.org.uk> writes:

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

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

[Jonathan Nieder]

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:

> --- 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(-)

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

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

[Junio C Hamano]

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.

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

[Jonathan Nieder]

Junio C Hamano wrote:
> 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.

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