[PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond <angusgh@gmail.com>

[PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond <angusgh@gmail.com>

[Angus Hammond]

---
 ident.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ident.c b/ident.c
index 87c697c..51a7a73 100644
--- a/ident.c
+++ b/ident.c
@@ -46,7 +46,7 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz)
 	if (len < sz)
 		name[len] = 0;
 	else
-		die("Your parents must have hated you!");
+		die("Your GECOS field is too long.");
 
 }
 
@@ -106,7 +106,7 @@ static void copy_email(const struct passwd *pw)
 	 */
 	size_t len = strlen(pw->pw_name);
 	if (len > sizeof(git_default_email)/2)
-		die("Your sysadmin must hate you!");
+		die("Your name field in is too long.");
 	memcpy(git_default_email, pw->pw_name, len);
 	git_default_email[len++] = '@';
 
@@ -125,7 +125,7 @@ static void setup_ident(const char **name, const char **emailp)
 	if (!*name && !git_default_name[0]) {
 		pw = getpwuid(getuid());
 		if (!pw)
-			die("You don't exist. Go away!");
+			die("Could not read your GECOS field.");
 		copy_gecos(pw, git_default_name, sizeof(git_default_name));
 	}
 	if (!*name)
@@ -142,7 +142,7 @@ static void setup_ident(const char **name, const char **emailp)
 			if (!pw)
 				pw = getpwuid(getuid());
 			if (!pw)
-				die("You don't exist. Go away!");
+				die("Could not read your GECOS field.");
 			copy_email(pw);
 		}
 	}
@@ -325,7 +325,7 @@ const char *fmt_ident(const char *name, const char *email,
 			die("empty ident %s <%s> not allowed", name, email);
 		pw = getpwuid(getuid());
 		if (!pw)
-			die("You don't exist. Go away!");
+			die("Could not read your GECOS field.");
 		strlcpy(git_default_name, pw->pw_name,
 			sizeof(git_default_name));
 		name = git_default_name;
-- 
1.7.9.5

[PATCH 2/2] Remove diagnostics section from commit-tree and var man pages New error messages shouldn't need explaining like the old ones did so just delete the diagnostics section of the man pages. Signed-off-by: Angus Hammond <angusgh@gmail.com>

[Angus Hammond]

---
 Documentation/git-commit-tree.txt |    9 ---------
 Documentation/git-var.txt         |    9 ---------
 2 files changed, 18 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index cfb9906..eb8ee99 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -88,15 +88,6 @@ for one to be entered and terminated with ^D.
 
 include::date-formats.txt[]
 
-Diagnostics
------------
-You don't exist. Go away!::
-    The passwd(5) gecos field couldn't be read
-Your parents must have hated you!::
-    The passwd(5) gecos field is longer than a giant static buffer.
-Your sysadmin must hate you!::
-    The passwd(5) name field is longer than a giant static buffer.
-
 Discussion
 ----------
 
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 988a323..67edf58 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -59,15 +59,6 @@ ifdef::git-default-pager[]
     The build you are using chose '{git-default-pager}' as the default.
 endif::git-default-pager[]
 
-Diagnostics
------------
-You don't exist. Go away!::
-    The passwd(5) gecos field couldn't be read
-Your parents must have hated you!::
-    The passwd(5) gecos field is longer than a giant static buffer.
-Your sysadmin must hate you!::
-    The passwd(5) name field is longer than a giant static buffer.
-
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
-- 
1.7.9.5

Re: [PATCH 2/2] Remove diagnostics section from commit-tree and var man pages New error messages shouldn't need explaining like the old ones did so just delete the diagnostics section of the man pages. Signed-off-by: Angus Hammond <angusgh@gmail.com>

[Angus Hammond]

I have no idea how I managed to send this to myself and see it as one,
properly formatted email despite it being 2 formatted diabolically with
entire commits messages in the subjects. Sorry about that.
These were meant to offer an alternative solution to the current issue over
unusual error messages from commit-tree by bypassing the whole unix humour
issue. They look it'll still be possible to apply them even if they are
badly sent. If not and people think they're worth while I'd be happy (try
and) send them again without this mess.
Thanks
Angus

Re: [PATCH 1/2] Change error messages in ident.c...

[Jeff King]

On Thu, May 10, 2012 at 08:06:09PM +0100, Angus Hammond wrote:

> Subject: Re: [PATCH 1/2] Change error messages in ident.c Make error messages
>  caused by failed reads of the /etc/passwd file easier to understand.
>  Signed-off-by: Angus Hammond <angusgh@gmail.com>

Holy line-breaks, Batman!

As amusing as I find the existing messages, this is probably a good
direction (although I find it unlikely that most people would see the
messages under normal use).

I am also tempted to suggest that we simply replace the static buffers
with dynamic strbufs. I guess that may open up new vectors for an
attacker to convince git to allocate arbitrary amounts of memory, but
that is already pretty easy to do, so I doubt it's a big deal.

> @@ -46,7 +46,7 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz)
>  	if (len < sz)
>  		name[len] = 0;
>  	else
> -		die("Your parents must have hated you!");
> +		die("Your GECOS field is too long.");

I know that "GECOS" is the standard name for the field, but I wonder if
it is a bit unnecessarily jargon-y. Wouldn't something like:

  die("unable to get real name from system password file: name too long");

be a little more friendly? It tells what operation we were actually
performing, and it doesn't use any jargon.

> @@ -106,7 +106,7 @@ static void copy_email(const struct passwd *pw)
>  	 */
>  	size_t len = strlen(pw->pw_name);
>  	if (len > sizeof(git_default_email)/2)
> -		die("Your sysadmin must hate you!");
> +		die("Your name field in is too long.");

s/in is/is/. Also, similar complaints to above (if you see this message
unexpectedly, you might ask "which name field? One inside a commit
object?").

> [...]

And similar comments for the rest of the messages.

-Peff

Re: [PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond <angusgh@gmail.com>

[Junio C Hamano]

They are one of the oldest and humorous messages we have in the system,
and more importantly, users will see them only once on a badly configured
system.  If there is no real-life reason (e.g. "if we do not change this
message, Nuclear reactors will start misbehaving"), I would rather keep
them as they are for hysterical raisins.

But that is just my preference to keep Linus's twisted sense of humor.

Re: [PATCH 1/2] Change error messages in ident.c...

[Jeff King]

On Thu, May 10, 2012 at 03:23:39PM -0400, Jeff King wrote:

> I am also tempted to suggest that we simply replace the static buffers
> with dynamic strbufs. I guess that may open up new vectors for an
> attacker to convince git to allocate arbitrary amounts of memory, but
> that is already pretty easy to do, so I doubt it's a big deal.

For reference, that patch would look like something like this:

---
 builtin/fmt-merge-msg.c | 14 ++++----
 cache.h                 |  5 ++-
 config.c                |  4 +--
 environment.c           |  4 +--
 http-push.c             |  2 +-
 ident.c                 | 94 ++++++++++++++++++-------------------------------
 6 files changed, 50 insertions(+), 73 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index a517f17..bb716c8 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -230,7 +230,8 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 static void record_person(int which, struct string_list *people,
 			  struct commit *commit)
 {
-	char name_buf[MAX_GITNAME], *name, *name_end;
+	struct strbuf name_buf = STRBUF_INIT;
+	char *name, *name_end;
 	struct string_list_item *elem;
 	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
 
@@ -243,17 +244,18 @@ static void record_person(int which, struct string_list *people,
 		name_end--;
 	while (isspace(*name_end) && name <= name_end)
 		name_end--;
-	if (name_end < name || name + MAX_GITNAME <= name_end)
+	if (name_end < name)
 		return;
-	memcpy(name_buf, name, name_end - name + 1);
-	name_buf[name_end - name + 1] = '\0';
+	strbuf_add(&name_buf, name, name_end - name + 1);
 
-	elem = string_list_lookup(people, name_buf);
+	elem = string_list_lookup(people, name_buf.buf);
 	if (!elem) {
-		elem = string_list_insert(people, name_buf);
+		elem = string_list_insert(people, name_buf.buf);
 		elem->util = (void *)0;
 	}
 	elem->util = (void*)(util_as_integral(elem) + 1);
+
+	strbuf_release(&name_buf);
 }
 
 static int cmp_string_list_util_as_integral(const void *a_, const void *b_)
diff --git a/cache.h b/cache.h
index e14ffcd..0c1a332 100644
--- a/cache.h
+++ b/cache.h
@@ -1138,9 +1138,8 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-#define MAX_GITNAME (1000)
-extern char git_default_email[MAX_GITNAME];
-extern char git_default_name[MAX_GITNAME];
+extern struct strbuf git_default_email;
+extern struct strbuf git_default_name;
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
diff --git a/config.c b/config.c
index eeee986..69cb08c 100644
--- a/config.c
+++ b/config.c
@@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_name, value, sizeof(git_default_name));
+		strbuf_addstr(&git_default_name, value);
 		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
@@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)
 	if (!strcmp(var, "user.email")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_email, value, sizeof(git_default_email));
+		strbuf_addstr(&git_default_email, value);
 		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
diff --git a/environment.c b/environment.c
index d7e6c65..f4e3b53 100644
--- a/environment.c
+++ b/environment.c
@@ -11,8 +11,8 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 
-char git_default_email[MAX_GITNAME];
-char git_default_name[MAX_GITNAME];
+struct strbuf git_default_email = STRBUF_INIT;
+struct strbuf git_default_name = STRBUF_INIT;
 int user_ident_explicitly_given;
 int trust_executable_bit = 1;
 int trust_ctime = 1;
diff --git a/http-push.c b/http-push.c
index 1df7ab5..2362ffd 100644
--- a/http-push.c
+++ b/http-push.c
@@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		ep = strchr(ep + 1, '/');
 	}
 
-	escaped = xml_entities(git_default_email);
+	escaped = xml_entities(git_default_email.buf);
 	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
 	free(escaped);
 
diff --git a/ident.c b/ident.c
index 87c697c..c7bdb3f 100644
--- a/ident.c
+++ b/ident.c
@@ -15,42 +15,27 @@ static char git_default_date[50];
 #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
 #endif
 
-static void copy_gecos(const struct passwd *w, char *name, size_t sz)
+static void copy_gecos(const struct passwd *w, struct strbuf *name)
 {
-	char *src, *dst;
-	size_t len, nlen;
-
-	nlen = strlen(w->pw_name);
+	char *src;
 
 	/* Traditionally GECOS field had office phone numbers etc, separated
 	 * with commas.  Also & stands for capitalized form of the login name.
 	 */
 
-	for (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {
+	for (src = get_gecos(w); *src && *src != ','; src++) {
 		int ch = *src;
-		if (ch != '&') {
-			*dst++ = ch;
-			if (ch == 0 || ch == ',')
-				break;
-			len++;
-			continue;
-		}
-		if (len + nlen < sz) {
+		if (ch != '&')
+			strbuf_addch(name, ch);
+		else {
 			/* Sorry, Mr. McDonald... */
-			*dst++ = toupper(*w->pw_name);
-			memcpy(dst, w->pw_name + 1, nlen - 1);
-			dst += nlen - 1;
-			len += nlen;
+			strbuf_addch(name, toupper(*w->pw_name));
+			strbuf_addstr(name, w->pw_name + 1);
 		}
 	}
-	if (len < sz)
-		name[len] = 0;
-	else
-		die("Your parents must have hated you!");
-
 }
 
-static int add_mailname_host(char *buf, size_t len)
+static int add_mailname_host(struct strbuf *buf)
 {
 	FILE *mailname;
 
@@ -61,7 +46,7 @@ static int add_mailname_host(char *buf, size_t len)
 				strerror(errno));
 		return -1;
 	}
-	if (!fgets(buf, len, mailname)) {
+	if (strbuf_getline(buf, mailname, '\n') == EOF) {
 		if (ferror(mailname))
 			warning("cannot read /etc/mailname: %s",
 				strerror(errno));
@@ -73,48 +58,41 @@ static int add_mailname_host(char *buf, size_t len)
 	return 0;
 }
 
-static void add_domainname(char *buf, size_t len)
+static void add_domainname(struct strbuf *out)
 {
+	char buf[1024];
 	struct hostent *he;
-	size_t namelen;
 	const char *domainname;
 
-	if (gethostname(buf, len)) {
+	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 		return;
 	}
-	namelen = strlen(buf);
-	if (memchr(buf, '.', namelen))
+	strbuf_addstr(out, buf);
+	if (strchr(buf, '.'))
 		return;
 
 	he = gethostbyname(buf);
-	buf[namelen++] = '.';
-	buf += namelen;
-	len -= namelen;
+	strbuf_addch(out, '.');
 	if (he && (domainname = strchr(he->h_name, '.')))
-		strlcpy(buf, domainname + 1, len);
+		strbuf_addstr(out, domainname + 1);
 	else
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 }
 
-static void copy_email(const struct passwd *pw)
+static void copy_email(const struct passwd *pw, struct strbuf *email)
 {
 	/*
 	 * Make up a fake email address
 	 * (name + '@' + hostname [+ '.' + domainname])
 	 */
-	size_t len = strlen(pw->pw_name);
-	if (len > sizeof(git_default_email)/2)
-		die("Your sysadmin must hate you!");
-	memcpy(git_default_email, pw->pw_name, len);
-	git_default_email[len++] = '@';
-
-	if (!add_mailname_host(git_default_email + len,
-				sizeof(git_default_email) - len))
+	strbuf_addstr(email, pw->pw_name);
+	strbuf_addch(email, '@');
+
+	if (!add_mailname_host(email))
 		return;	/* read from "/etc/mailname" (Debian) */
-	add_domainname(git_default_email + len,
-			sizeof(git_default_email) - len);
+	add_domainname(email);
 }
 
 static void setup_ident(const char **name, const char **emailp)
@@ -122,32 +100,31 @@ static void setup_ident(const char **name, const char **emailp)
 	struct passwd *pw = NULL;
 
 	/* Get the name ("gecos") */
-	if (!*name && !git_default_name[0]) {
+	if (!*name && !git_default_name.len) {
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		copy_gecos(pw, git_default_name, sizeof(git_default_name));
+		copy_gecos(pw, &git_default_name);
 	}
 	if (!*name)
-		*name = git_default_name;
+		*name = git_default_name.buf;
 
-	if (!*emailp && !git_default_email[0]) {
+	if (!*emailp && !git_default_email.len) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
-			strlcpy(git_default_email, email,
-				sizeof(git_default_email));
+			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else {
 			if (!pw)
 				pw = getpwuid(getuid());
 			if (!pw)
 				die("You don't exist. Go away!");
-			copy_email(pw);
+			copy_email(pw, &git_default_email);
 		}
 	}
 	if (!*emailp)
-		*emailp = git_default_email;
+		*emailp = git_default_email.buf;
 
 	/* And set the default date */
 	if (!git_default_date[0])
@@ -317,7 +294,7 @@ const char *fmt_ident(const char *name, const char *email,
 		struct passwd *pw;
 
 		if ((warn_on_no_name || error_on_no_name) &&
-		    name == git_default_name && env_hint) {
+		    name == git_default_name.buf && env_hint) {
 			fputs(env_hint, stderr);
 			env_hint = NULL; /* warn only once */
 		}
@@ -326,9 +303,8 @@ const char *fmt_ident(const char *name, const char *email,
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		strlcpy(git_default_name, pw->pw_name,
-			sizeof(git_default_name));
-		name = git_default_name;
+		strbuf_addstr(&git_default_name, pw->pw_name);
+		name = git_default_name.buf;
 	}
 
 	strcpy(date, git_default_date);

Re: [PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond <angusgh@gmail.com>

[Angus Hammond]

On 10 May 2012 20:43, Junio C Hamano <gitster@pobox.com> wrote:
> They are one of the oldest and humorous messages we have in the system,
> and more importantly, users will see them only once on a badly configured
> system.  If there is no real-life reason (e.g. "if we do not change this
> message, Nuclear reactors will start misbehaving"), I would rather keep
> them as they are for hysterical raisins.
I'm not too worried either way, just tried to knock the patch out
quickly because it came up and this seemed like the logical solution.
In all honesty though, whilst I don't have a problem with unix humour
being in git, I do have a bit of a problem with it being in error
messages since when these are displayed it means that a users system
is preventing them from using git for whatever reason, and at those
times there's a good chance you're worried about fixing that problem,
not laughing at a joke made by Linus several years ago.

Just my 2 cents. It's probably not worth too much bother since it'll
only ever show up very rarely.
Thanks
Angus

Re: [PATCH 1/2] Change error messages in ident.c...

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am also tempted to suggest that we simply replace the static buffers
> with dynamic strbufs.

Yeah, I think that is a proper approach for this issue, as it will make
two of these messages unnecessary (or all?  I couldn't think of a way
to deal with missing getpwent case myself, though).

Re: [PATCH 1/2] Change error messages in ident.c...

[Jeff King]

On Thu, May 10, 2012 at 01:04:13PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I am also tempted to suggest that we simply replace the static buffers
> > with dynamic strbufs.
> 
> Yeah, I think that is a proper approach for this issue, as it will make
> two of these messages unnecessary (or all?  I couldn't think of a way
> to deal with missing getpwent case myself, though).

It doesn't get rid of the "you don't exist" message, and I think just
dying there makes sense.  But that is actually the one that I consider
the most likely to happen in practice, and should probably have a more
useful error message.

-Peff

Re: [PATCH 1/2] Change error messages in ident.c...

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It doesn't get rid of the "you don't exist" message, and I think just
> dying there makes sense.  But that is actually the one that I consider
> the most likely to happen in practice, and should probably have a more
> useful error message.

Yeah, I do not think anybody minds losing that phrasing from that message
(the "parents" and "sysadmin" were the humorous ones), and we certainly
can phrase it differently, e.g.

    Your system didn't tell me your real name; hint: git help config
    and look for user.name

or something.

Re: [PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond <angusgh@gmail.com>

[Nguyen Thai Ngoc Duy]

On Fri, May 11, 2012 at 2:06 AM, Angus Hammond <angusgh@gmail.com> wrote:
> ---
>  ident.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

While you are touching this, perhaps you can also turn all die(xxx) in
this file to die(_(xxx)), same for warning()? You touch 5 out of 11
already. And it helps make sure all the new strings are in the same
humor level (aka none). _() allows the messages to be translated in
another language, by the way.

Also this on top so we get nice advice

diff --git a/ident.c b/ident.c
index 87c697c..b5a631f 100644
--- a/ident.c
+++ b/ident.c
@@ -289,7 +289,7 @@ person_only:
 }

 static const char *env_hint =
-"\n"
+N_("\n"
 "*** Please tell me who you are.\n"
 "\n"
 "Run\n"
@@ -299,7 +299,7 @@ static const char *env_hint =
 "\n"
 "to set your account\'s default identity.\n"
 "Omit --global to set the identity only in this repository.\n"
-"\n";
+"\n");

 const char *fmt_ident(const char *name, const char *email,
 		      const char *date_str, int flag)
@@ -318,7 +318,7 @@ const char *fmt_ident(const char *name, const char *email,

 		if ((warn_on_no_name || error_on_no_name) &&
 		    name == git_default_name && env_hint) {
-			fputs(env_hint, stderr);
+			fputs(_(env_hint), stderr);
 			env_hint = NULL; /* warn only once */
 		}
 		if (error_on_no_name)
-- 
Duy

Re: [PATCH 1/2] Change error messages in ident.c...

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, May 10, 2012 at 03:23:39PM -0400, Jeff King wrote:
>
>> I am also tempted to suggest that we simply replace the static buffers
>> with dynamic strbufs. I guess that may open up new vectors for an
>> attacker to convince git to allocate arbitrary amounts of memory, but
>> that is already pretty easy to do, so I doubt it's a big deal.
>
> For reference, that patch would look like something like this:

Looks quite straight-forward and readable, I would say.  Not only you gave
us a legitimate excuse to get rid of the humourous messages, you lifted
most of the artificial limitations ('domainname' limit is still there but
that is not anything new) and use of strlcpy(), the last of which is a
huge win from my point of view ;-)

>
> ---
>  builtin/fmt-merge-msg.c | 14 ++++----
>  cache.h                 |  5 ++-
>  config.c                |  4 +--
>  environment.c           |  4 +--
>  http-push.c             |  2 +-
>  ident.c                 | 94 ++++++++++++++++++-------------------------------
>  6 files changed, 50 insertions(+), 73 deletions(-)
>
> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index a517f17..bb716c8 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -230,7 +230,8 @@ static void add_branch_desc(struct strbuf *out, const char *name)
>  static void record_person(int which, struct string_list *people,
>  			  struct commit *commit)
>  {
> -	char name_buf[MAX_GITNAME], *name, *name_end;
> +	struct strbuf name_buf = STRBUF_INIT;
> +	char *name, *name_end;
>  	struct string_list_item *elem;
>  	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
>  
> @@ -243,17 +244,18 @@ static void record_person(int which, struct string_list *people,
>  		name_end--;
>  	while (isspace(*name_end) && name <= name_end)
>  		name_end--;
> -	if (name_end < name || name + MAX_GITNAME <= name_end)
> +	if (name_end < name)
>  		return;
> -	memcpy(name_buf, name, name_end - name + 1);
> -	name_buf[name_end - name + 1] = '\0';
> +	strbuf_add(&name_buf, name, name_end - name + 1);
>  
> -	elem = string_list_lookup(people, name_buf);
> +	elem = string_list_lookup(people, name_buf.buf);
>  	if (!elem) {
> -		elem = string_list_insert(people, name_buf);
> +		elem = string_list_insert(people, name_buf.buf);
>  		elem->util = (void *)0;
>  	}
>  	elem->util = (void*)(util_as_integral(elem) + 1);
> +
> +	strbuf_release(&name_buf);
>  }
>  
>  static int cmp_string_list_util_as_integral(const void *a_, const void *b_)
> diff --git a/cache.h b/cache.h
> index e14ffcd..0c1a332 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1138,9 +1138,8 @@ struct config_include_data {
>  #define CONFIG_INCLUDE_INIT { 0 }
>  extern int git_config_include(const char *name, const char *value, void *data);
>  
> -#define MAX_GITNAME (1000)
> -extern char git_default_email[MAX_GITNAME];
> -extern char git_default_name[MAX_GITNAME];
> +extern struct strbuf git_default_email;
> +extern struct strbuf git_default_name;
>  #define IDENT_NAME_GIVEN 01
>  #define IDENT_MAIL_GIVEN 02
>  #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
> diff --git a/config.c b/config.c
> index eeee986..69cb08c 100644
> --- a/config.c
> +++ b/config.c
> @@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)
>  	if (!strcmp(var, "user.name")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> -		strlcpy(git_default_name, value, sizeof(git_default_name));
> +		strbuf_addstr(&git_default_name, value);
>  		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
>  		return 0;
>  	}
> @@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)
>  	if (!strcmp(var, "user.email")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> -		strlcpy(git_default_email, value, sizeof(git_default_email));
> +		strbuf_addstr(&git_default_email, value);
>  		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
>  		return 0;
>  	}
> diff --git a/environment.c b/environment.c
> index d7e6c65..f4e3b53 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -11,8 +11,8 @@
>  #include "refs.h"
>  #include "fmt-merge-msg.h"
>  
> -char git_default_email[MAX_GITNAME];
> -char git_default_name[MAX_GITNAME];
> +struct strbuf git_default_email = STRBUF_INIT;
> +struct strbuf git_default_name = STRBUF_INIT;
>  int user_ident_explicitly_given;
>  int trust_executable_bit = 1;
>  int trust_ctime = 1;
> diff --git a/http-push.c b/http-push.c
> index 1df7ab5..2362ffd 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
>  		ep = strchr(ep + 1, '/');
>  	}
>  
> -	escaped = xml_entities(git_default_email);
> +	escaped = xml_entities(git_default_email.buf);
>  	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
>  	free(escaped);
>  
> diff --git a/ident.c b/ident.c
> index 87c697c..c7bdb3f 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -15,42 +15,27 @@ static char git_default_date[50];
>  #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
>  #endif
>  
> -static void copy_gecos(const struct passwd *w, char *name, size_t sz)
> +static void copy_gecos(const struct passwd *w, struct strbuf *name)
>  {
> -	char *src, *dst;
> -	size_t len, nlen;
> -
> -	nlen = strlen(w->pw_name);
> +	char *src;
>  
>  	/* Traditionally GECOS field had office phone numbers etc, separated
>  	 * with commas.  Also & stands for capitalized form of the login name.
>  	 */
>  
> -	for (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {
> +	for (src = get_gecos(w); *src && *src != ','; src++) {
>  		int ch = *src;
> -		if (ch != '&') {
> -			*dst++ = ch;
> -			if (ch == 0 || ch == ',')
> -				break;
> -			len++;
> -			continue;
> -		}
> -		if (len + nlen < sz) {
> +		if (ch != '&')
> +			strbuf_addch(name, ch);
> +		else {
>  			/* Sorry, Mr. McDonald... */
> -			*dst++ = toupper(*w->pw_name);
> -			memcpy(dst, w->pw_name + 1, nlen - 1);
> -			dst += nlen - 1;
> -			len += nlen;
> +			strbuf_addch(name, toupper(*w->pw_name));
> +			strbuf_addstr(name, w->pw_name + 1);
>  		}
>  	}
> -	if (len < sz)
> -		name[len] = 0;
> -	else
> -		die("Your parents must have hated you!");
> -
>  }
>  
> -static int add_mailname_host(char *buf, size_t len)
> +static int add_mailname_host(struct strbuf *buf)
>  {
>  	FILE *mailname;
>  
> @@ -61,7 +46,7 @@ static int add_mailname_host(char *buf, size_t len)
>  				strerror(errno));
>  		return -1;
>  	}
> -	if (!fgets(buf, len, mailname)) {
> +	if (strbuf_getline(buf, mailname, '\n') == EOF) {
>  		if (ferror(mailname))
>  			warning("cannot read /etc/mailname: %s",
>  				strerror(errno));
> @@ -73,48 +58,41 @@ static int add_mailname_host(char *buf, size_t len)
>  	return 0;
>  }
>  
> -static void add_domainname(char *buf, size_t len)
> +static void add_domainname(struct strbuf *out)
>  {
> +	char buf[1024];
>  	struct hostent *he;
> -	size_t namelen;
>  	const char *domainname;
>  
> -	if (gethostname(buf, len)) {
> +	if (gethostname(buf, sizeof(buf))) {
>  		warning("cannot get host name: %s", strerror(errno));
> -		strlcpy(buf, "(none)", len);
> +		strbuf_addstr(out, "(none)");
>  		return;
>  	}
> -	namelen = strlen(buf);
> -	if (memchr(buf, '.', namelen))
> +	strbuf_addstr(out, buf);
> +	if (strchr(buf, '.'))
>  		return;
>  
>  	he = gethostbyname(buf);
> -	buf[namelen++] = '.';
> -	buf += namelen;
> -	len -= namelen;
> +	strbuf_addch(out, '.');
>  	if (he && (domainname = strchr(he->h_name, '.')))
> -		strlcpy(buf, domainname + 1, len);
> +		strbuf_addstr(out, domainname + 1);
>  	else
> -		strlcpy(buf, "(none)", len);
> +		strbuf_addstr(out, "(none)");
>  }
>  
> -static void copy_email(const struct passwd *pw)
> +static void copy_email(const struct passwd *pw, struct strbuf *email)
>  {
>  	/*
>  	 * Make up a fake email address
>  	 * (name + '@' + hostname [+ '.' + domainname])
>  	 */
> -	size_t len = strlen(pw->pw_name);
> -	if (len > sizeof(git_default_email)/2)
> -		die("Your sysadmin must hate you!");
> -	memcpy(git_default_email, pw->pw_name, len);
> -	git_default_email[len++] = '@';
> -
> -	if (!add_mailname_host(git_default_email + len,
> -				sizeof(git_default_email) - len))
> +	strbuf_addstr(email, pw->pw_name);
> +	strbuf_addch(email, '@');
> +
> +	if (!add_mailname_host(email))
>  		return;	/* read from "/etc/mailname" (Debian) */
> -	add_domainname(git_default_email + len,
> -			sizeof(git_default_email) - len);
> +	add_domainname(email);
>  }
>  
>  static void setup_ident(const char **name, const char **emailp)
> @@ -122,32 +100,31 @@ static void setup_ident(const char **name, const char **emailp)
>  	struct passwd *pw = NULL;
>  
>  	/* Get the name ("gecos") */
> -	if (!*name && !git_default_name[0]) {
> +	if (!*name && !git_default_name.len) {
>  		pw = getpwuid(getuid());
>  		if (!pw)
>  			die("You don't exist. Go away!");
> -		copy_gecos(pw, git_default_name, sizeof(git_default_name));
> +		copy_gecos(pw, &git_default_name);
>  	}
>  	if (!*name)
> -		*name = git_default_name;
> +		*name = git_default_name.buf;
>  
> -	if (!*emailp && !git_default_email[0]) {
> +	if (!*emailp && !git_default_email.len) {
>  		const char *email = getenv("EMAIL");
>  
>  		if (email && email[0]) {
> -			strlcpy(git_default_email, email,
> -				sizeof(git_default_email));
> +			strbuf_addstr(&git_default_email, email);
>  			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
>  		} else {
>  			if (!pw)
>  				pw = getpwuid(getuid());
>  			if (!pw)
>  				die("You don't exist. Go away!");
> -			copy_email(pw);
> +			copy_email(pw, &git_default_email);
>  		}
>  	}
>  	if (!*emailp)
> -		*emailp = git_default_email;
> +		*emailp = git_default_email.buf;
>  
>  	/* And set the default date */
>  	if (!git_default_date[0])
> @@ -317,7 +294,7 @@ const char *fmt_ident(const char *name, const char *email,
>  		struct passwd *pw;
>  
>  		if ((warn_on_no_name || error_on_no_name) &&
> -		    name == git_default_name && env_hint) {
> +		    name == git_default_name.buf && env_hint) {
>  			fputs(env_hint, stderr);
>  			env_hint = NULL; /* warn only once */
>  		}
> @@ -326,9 +303,8 @@ const char *fmt_ident(const char *name, const char *email,
>  		pw = getpwuid(getuid());
>  		if (!pw)
>  			die("You don't exist. Go away!");
> -		strlcpy(git_default_name, pw->pw_name,
> -			sizeof(git_default_name));
> -		name = git_default_name;
> +		strbuf_addstr(&git_default_name, pw->pw_name);
> +		name = git_default_name.buf;
>  	}
>  
>  	strcpy(date, git_default_date);

Re: [PATCH 1/2] Change error messages in ident.c...

[Jeff King]

On Fri, May 11, 2012 at 03:53:43PM -0700, Junio C Hamano wrote:

> >> I am also tempted to suggest that we simply replace the static buffers
> >> with dynamic strbufs. I guess that may open up new vectors for an
> >> attacker to convince git to allocate arbitrary amounts of memory, but
> >> that is already pretty easy to do, so I doubt it's a big deal.
> >
> > For reference, that patch would look like something like this:
> 
> Looks quite straight-forward and readable, I would say.  Not only you gave
> us a legitimate excuse to get rid of the humourous messages, you lifted
> most of the artificial limitations ('domainname' limit is still there but
> that is not anything new) and use of strlcpy(), the last of which is a
> huge win from my point of view ;-)

Thanks. I'll re-roll with a commit message, and a follow-on patch to fix
the "you don't exist" message. But probably tomorrow, as I am just
finishing gitting for the day.

-Peff

[PATCH 1/2] drop length limitations on gecos-derived names and emails

[Jeff King]

When we pull the user's name from the GECOS field of the
passwd file (or generate an email address based on their
username and hostname), we put the result into a
static buffer. While it's extremely unlikely that anybody
ever hit these limits (after all, in such a case their
parents must have hated them), we still had to deal with the
error cases in our code.

Converting these static buffers to strbufs lets us simplify
the code and drop some error messages from the documentation
that have confused some users.

Note that there is still one length limitation: the
gethostname interface requires us to provide a static
buffer, so we arbitrarily choose 1024 bytes for the
hostname.

Signed-off-by: Jeff King <peff@peff.net>
---
I noticed in add_domainname that we look up the host via gethostname,
and then if it is not fully qualified, call gethostbyname and steal the
domain portion of the result, tacking it onto the hostname we got.

That seems oddly complex to me, and like it could result in a bogus
hostname if the unqualified name does not match the first part of the
returned qualified name. E.g., if the /etc/hosts file contains something
like:

  192.168.1.1 foo.example.com bar.example.com bar

(and your hostname is "bar"). I doubt it matters much in practice, and
it is outside the scope of this patch, so I left it for now.

 Documentation/git-commit-tree.txt |  4 --
 Documentation/git-var.txt         |  4 --
 builtin/fmt-merge-msg.c           | 14 +++---
 cache.h                           |  5 +--
 config.c                          |  4 +-
 environment.c                     |  4 +-
 http-push.c                       |  2 +-
 ident.c                           | 94 +++++++++++++++------------------------
 8 files changed, 50 insertions(+), 81 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index cfb9906..eb12b2d 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -92,10 +92,6 @@ Diagnostics
 -----------
 You don't exist. Go away!::
     The passwd(5) gecos field couldn't be read
-Your parents must have hated you!::
-    The passwd(5) gecos field is longer than a giant static buffer.
-Your sysadmin must hate you!::
-    The passwd(5) name field is longer than a giant static buffer.
 
 Discussion
 ----------
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 988a323..3f703e3 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -63,10 +63,6 @@ Diagnostics
 -----------
 You don't exist. Go away!::
     The passwd(5) gecos field couldn't be read
-Your parents must have hated you!::
-    The passwd(5) gecos field is longer than a giant static buffer.
-Your sysadmin must hate you!::
-    The passwd(5) name field is longer than a giant static buffer.
 
 SEE ALSO
 --------
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index a517f17..bb716c8 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -230,7 +230,8 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 static void record_person(int which, struct string_list *people,
 			  struct commit *commit)
 {
-	char name_buf[MAX_GITNAME], *name, *name_end;
+	struct strbuf name_buf = STRBUF_INIT;
+	char *name, *name_end;
 	struct string_list_item *elem;
 	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
 
@@ -243,17 +244,18 @@ static void record_person(int which, struct string_list *people,
 		name_end--;
 	while (isspace(*name_end) && name <= name_end)
 		name_end--;
-	if (name_end < name || name + MAX_GITNAME <= name_end)
+	if (name_end < name)
 		return;
-	memcpy(name_buf, name, name_end - name + 1);
-	name_buf[name_end - name + 1] = '\0';
+	strbuf_add(&name_buf, name, name_end - name + 1);
 
-	elem = string_list_lookup(people, name_buf);
+	elem = string_list_lookup(people, name_buf.buf);
 	if (!elem) {
-		elem = string_list_insert(people, name_buf);
+		elem = string_list_insert(people, name_buf.buf);
 		elem->util = (void *)0;
 	}
 	elem->util = (void*)(util_as_integral(elem) + 1);
+
+	strbuf_release(&name_buf);
 }
 
 static int cmp_string_list_util_as_integral(const void *a_, const void *b_)
diff --git a/cache.h b/cache.h
index e14ffcd..0c1a332 100644
--- a/cache.h
+++ b/cache.h
@@ -1138,9 +1138,8 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-#define MAX_GITNAME (1000)
-extern char git_default_email[MAX_GITNAME];
-extern char git_default_name[MAX_GITNAME];
+extern struct strbuf git_default_email;
+extern struct strbuf git_default_name;
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
diff --git a/config.c b/config.c
index eeee986..69cb08c 100644
--- a/config.c
+++ b/config.c
@@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_name, value, sizeof(git_default_name));
+		strbuf_addstr(&git_default_name, value);
 		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
@@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)
 	if (!strcmp(var, "user.email")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_email, value, sizeof(git_default_email));
+		strbuf_addstr(&git_default_email, value);
 		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
diff --git a/environment.c b/environment.c
index d7e6c65..f4e3b53 100644
--- a/environment.c
+++ b/environment.c
@@ -11,8 +11,8 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 
-char git_default_email[MAX_GITNAME];
-char git_default_name[MAX_GITNAME];
+struct strbuf git_default_email = STRBUF_INIT;
+struct strbuf git_default_name = STRBUF_INIT;
 int user_ident_explicitly_given;
 int trust_executable_bit = 1;
 int trust_ctime = 1;
diff --git a/http-push.c b/http-push.c
index 1df7ab5..2362ffd 100644
--- a/http-push.c
+++ b/http-push.c
@@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		ep = strchr(ep + 1, '/');
 	}
 
-	escaped = xml_entities(git_default_email);
+	escaped = xml_entities(git_default_email.buf);
 	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
 	free(escaped);
 
diff --git a/ident.c b/ident.c
index 87c697c..c7bdb3f 100644
--- a/ident.c
+++ b/ident.c
@@ -15,42 +15,27 @@ static char git_default_date[50];
 #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
 #endif
 
-static void copy_gecos(const struct passwd *w, char *name, size_t sz)
+static void copy_gecos(const struct passwd *w, struct strbuf *name)
 {
-	char *src, *dst;
-	size_t len, nlen;
-
-	nlen = strlen(w->pw_name);
+	char *src;
 
 	/* Traditionally GECOS field had office phone numbers etc, separated
 	 * with commas.  Also & stands for capitalized form of the login name.
 	 */
 
-	for (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {
+	for (src = get_gecos(w); *src && *src != ','; src++) {
 		int ch = *src;
-		if (ch != '&') {
-			*dst++ = ch;
-			if (ch == 0 || ch == ',')
-				break;
-			len++;
-			continue;
-		}
-		if (len + nlen < sz) {
+		if (ch != '&')
+			strbuf_addch(name, ch);
+		else {
 			/* Sorry, Mr. McDonald... */
-			*dst++ = toupper(*w->pw_name);
-			memcpy(dst, w->pw_name + 1, nlen - 1);
-			dst += nlen - 1;
-			len += nlen;
+			strbuf_addch(name, toupper(*w->pw_name));
+			strbuf_addstr(name, w->pw_name + 1);
 		}
 	}
-	if (len < sz)
-		name[len] = 0;
-	else
-		die("Your parents must have hated you!");
-
 }
 
-static int add_mailname_host(char *buf, size_t len)
+static int add_mailname_host(struct strbuf *buf)
 {
 	FILE *mailname;
 
@@ -61,7 +46,7 @@ static int add_mailname_host(char *buf, size_t len)
 				strerror(errno));
 		return -1;
 	}
-	if (!fgets(buf, len, mailname)) {
+	if (strbuf_getline(buf, mailname, '\n') == EOF) {
 		if (ferror(mailname))
 			warning("cannot read /etc/mailname: %s",
 				strerror(errno));
@@ -73,48 +58,41 @@ static int add_mailname_host(char *buf, size_t len)
 	return 0;
 }
 
-static void add_domainname(char *buf, size_t len)
+static void add_domainname(struct strbuf *out)
 {
+	char buf[1024];
 	struct hostent *he;
-	size_t namelen;
 	const char *domainname;
 
-	if (gethostname(buf, len)) {
+	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 		return;
 	}
-	namelen = strlen(buf);
-	if (memchr(buf, '.', namelen))
+	strbuf_addstr(out, buf);
+	if (strchr(buf, '.'))
 		return;
 
 	he = gethostbyname(buf);
-	buf[namelen++] = '.';
-	buf += namelen;
-	len -= namelen;
+	strbuf_addch(out, '.');
 	if (he && (domainname = strchr(he->h_name, '.')))
-		strlcpy(buf, domainname + 1, len);
+		strbuf_addstr(out, domainname + 1);
 	else
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 }
 
-static void copy_email(const struct passwd *pw)
+static void copy_email(const struct passwd *pw, struct strbuf *email)
 {
 	/*
 	 * Make up a fake email address
 	 * (name + '@' + hostname [+ '.' + domainname])
 	 */
-	size_t len = strlen(pw->pw_name);
-	if (len > sizeof(git_default_email)/2)
-		die("Your sysadmin must hate you!");
-	memcpy(git_default_email, pw->pw_name, len);
-	git_default_email[len++] = '@';
-
-	if (!add_mailname_host(git_default_email + len,
-				sizeof(git_default_email) - len))
+	strbuf_addstr(email, pw->pw_name);
+	strbuf_addch(email, '@');
+
+	if (!add_mailname_host(email))
 		return;	/* read from "/etc/mailname" (Debian) */
-	add_domainname(git_default_email + len,
-			sizeof(git_default_email) - len);
+	add_domainname(email);
 }
 
 static void setup_ident(const char **name, const char **emailp)
@@ -122,32 +100,31 @@ static void setup_ident(const char **name, const char **emailp)
 	struct passwd *pw = NULL;
 
 	/* Get the name ("gecos") */
-	if (!*name && !git_default_name[0]) {
+	if (!*name && !git_default_name.len) {
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		copy_gecos(pw, git_default_name, sizeof(git_default_name));
+		copy_gecos(pw, &git_default_name);
 	}
 	if (!*name)
-		*name = git_default_name;
+		*name = git_default_name.buf;
 
-	if (!*emailp && !git_default_email[0]) {
+	if (!*emailp && !git_default_email.len) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
-			strlcpy(git_default_email, email,
-				sizeof(git_default_email));
+			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else {
 			if (!pw)
 				pw = getpwuid(getuid());
 			if (!pw)
 				die("You don't exist. Go away!");
-			copy_email(pw);
+			copy_email(pw, &git_default_email);
 		}
 	}
 	if (!*emailp)
-		*emailp = git_default_email;
+		*emailp = git_default_email.buf;
 
 	/* And set the default date */
 	if (!git_default_date[0])
@@ -317,7 +294,7 @@ const char *fmt_ident(const char *name, const char *email,
 		struct passwd *pw;
 
 		if ((warn_on_no_name || error_on_no_name) &&
-		    name == git_default_name && env_hint) {
+		    name == git_default_name.buf && env_hint) {
 			fputs(env_hint, stderr);
 			env_hint = NULL; /* warn only once */
 		}
@@ -326,9 +303,8 @@ const char *fmt_ident(const char *name, const char *email,
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		strlcpy(git_default_name, pw->pw_name,
-			sizeof(git_default_name));
-		name = git_default_name;
+		strbuf_addstr(&git_default_name, pw->pw_name);
+		name = git_default_name.buf;
 	}
 
 	strcpy(date, git_default_date);
-- 
1.7.10.2.8.g1101eed

[PATCH 2/2] ident: report passwd errors with a more friendly message

[Jeff King]

When getpwuid fails, we give a cute but cryptic message.
While it makes sense if you know that getpwuid or identity
functions are being called, this code is triggered behind
the scenes by quite a few git commands these days (e.g.,
receive-pack on a remote server might use it for a reflog;
the current message is hard to distinguish from an
authentication error).  Let's switch to something that gives
a little more context.

While we're at it, we can factor out all of the
cut-and-pastes of the "you don't exist" message into a
wrapper function. Rather than provide xgetpwuid, let's make
it even more specific to just getting the passwd entry for
the current uid. That's the only way we use getpwuid anyway,
and it lets us make an even more specific error message.

The current message also fails to mention errno. While the
usual cause for getpwuid failing is that the user does not
exist, mentioning errno makes it easier to diagnose these
problems.  Note that POSIX specifies that errno remain
untouched if the passwd entry does not exist (but will be
set on actual errors), whereas some systems will return
ENOENT or similar for a missing entry. We handle both cases
in our wrapper.

Signed-off-by: Jeff King <peff@peff.net>
---
You earlier suggested to show a hint to set "user.name". That might be
complicated by the fact that this message can come from a remote server.
Or maybe since that is by far the minority case, we should disregard it
and show the hint. I left it out of this patch, as it can be trivially
added on top due to the refactoring.

I also noticed that the version of getpwuid in compat/mingw.c completely
disregards its uid argument. This isn't a problem in the current
codebase, since we always feed getuid(). But since the new wrapper is
explicitly about getting our _own_ pw entry, it might make more sense to
convert our getpwuid() replacement into an xgetpwuid_self() replacement,
which is slightly more accurate. I'll leave that cleanup to Johannes if
he cares to do it.

 Documentation/git-commit-tree.txt |  5 -----
 Documentation/git-var.txt         |  5 -----
 git-compat-util.h                 |  3 +++
 ident.c                           | 12 +++---------
 wrapper.c                         | 12 ++++++++++++
 5 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index eb12b2d..eb8ee99 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -88,11 +88,6 @@ for one to be entered and terminated with ^D.
 
 include::date-formats.txt[]
 
-Diagnostics
------------
-You don't exist. Go away!::
-    The passwd(5) gecos field couldn't be read
-
 Discussion
 ----------
 
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 3f703e3..67edf58 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -59,11 +59,6 @@ ifdef::git-default-pager[]
     The build you are using chose '{git-default-pager}' as the default.
 endif::git-default-pager[]
 
-Diagnostics
------------
-You don't exist. Go away!::
-    The passwd(5) gecos field couldn't be read
-
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/git-compat-util.h b/git-compat-util.h
index ed11ad8..5bd9ad7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -595,4 +595,7 @@ int rmdir_or_warn(const char *path);
  */
 int remove_or_warn(unsigned int mode, const char *path);
 
+/* Get the passwd entry for the UID of the current process. */
+struct passwd *xgetpwuid_self(void);
+
 #endif
diff --git a/ident.c b/ident.c
index c7bdb3f..72944ba 100644
--- a/ident.c
+++ b/ident.c
@@ -101,9 +101,7 @@ static void setup_ident(const char **name, const char **emailp)
 
 	/* Get the name ("gecos") */
 	if (!*name && !git_default_name.len) {
-		pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
+		pw = xgetpwuid_self();
 		copy_gecos(pw, &git_default_name);
 	}
 	if (!*name)
@@ -117,9 +115,7 @@ static void setup_ident(const char **name, const char **emailp)
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else {
 			if (!pw)
-				pw = getpwuid(getuid());
-			if (!pw)
-				die("You don't exist. Go away!");
+				pw = xgetpwuid_self();
 			copy_email(pw, &git_default_email);
 		}
 	}
@@ -300,9 +296,7 @@ const char *fmt_ident(const char *name, const char *email,
 		}
 		if (error_on_no_name)
 			die("empty ident %s <%s> not allowed", name, email);
-		pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
+		pw = xgetpwuid_self();
 		strbuf_addstr(&git_default_name, pw->pw_name);
 		name = git_default_name.buf;
 	}
diff --git a/wrapper.c b/wrapper.c
index 6ccd059..b5e33e4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -402,3 +402,15 @@ int remove_or_warn(unsigned int mode, const char *file)
 {
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
+
+struct passwd *xgetpwuid_self(void)
+{
+	struct passwd *pw;
+
+	errno = 0;
+	pw = getpwuid(getuid());
+	if (!pw)
+		die(_("unable to look up current user in the passwd file: %s"),
+		    errno ? strerror(errno) : _("no such user"));
+	return pw;
+}
-- 
1.7.10.2.8.g1101eed

Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

[Jeff King]

On Mon, May 14, 2012 at 12:28:24PM -0400, Jeff King wrote:

> I noticed in add_domainname that we look up the host via gethostname,
> and then if it is not fully qualified, call gethostbyname and steal the
> domain portion of the result, tacking it onto the hostname we got.
> 
> That seems oddly complex to me, and like it could result in a bogus
> hostname if the unqualified name does not match the first part of the
> returned qualified name. E.g., if the /etc/hosts file contains something
> like:
> 
>   192.168.1.1 foo.example.com bar.example.com bar
> 
> (and your hostname is "bar"). I doubt it matters much in practice, and
> it is outside the scope of this patch, so I left it for now.

It looks like a bug in adc3dbc (Use sensible domain name (the DNS one)
when guessing ident information, 2005-10-21). Before that we used
getdomainname, where that procedure made more sense.

The patch below fixes it. I doubt it matters much in practice, but I
think the resulting code is way less confusing to read.

-- >8 --
Subject: [PATCH] ident: use full dns names to generate email addresses

When we construct an email address from the username and
hostname, we generate the host part of the email with this
procedure:

  1. add the result of gethostname

  2. if it has a dot, ok, it's fully qualified

  3. if not, then look up the unqualified hostname via
     gethostbyname; take the domain name of the result and
     append it to the hostname

Step 3 can actually produce a bogus result, as the name
returned by gethostbyname may not be related to the hostname
we fed it (e.g., consider a machine "foo" with names
"foo.one.example.com" and "bar.two.example.com"; we may have
the latter returned and generate the bogus name
"foo.two.example.com").

This patch simply uses the full hostname returned by
gethostbyname. In the common case that the first part is the
same as the unqualified hostname, the behavior is identical.
And in the case that it is not the same, we are much more
likely to be generating a valid name.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/ident.c b/ident.c
index 72944ba..e552e7f 100644
--- a/ident.c
+++ b/ident.c
@@ -62,23 +62,18 @@ static void add_domainname(struct strbuf *out)
 {
 	char buf[1024];
 	struct hostent *he;
-	const char *domainname;
 
 	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
 		strbuf_addstr(out, "(none)");
 		return;
 	}
-	strbuf_addstr(out, buf);
 	if (strchr(buf, '.'))
-		return;
-
-	he = gethostbyname(buf);
-	strbuf_addch(out, '.');
-	if (he && (domainname = strchr(he->h_name, '.')))
-		strbuf_addstr(out, domainname + 1);
+		strbuf_addstr(out, buf);
+	else if ((he = gethostbyname(buf)) && strchr(he->h_name, '.'))
+		strbuf_addstr(out, he->h_name);
 	else
-		strbuf_addstr(out, "(none)");
+		strbuf_addf(out, "%s.(none)", buf);
 }
 
 static void copy_email(const struct passwd *pw, struct strbuf *email)
-- 
1.7.10.2.8.g1101eed

Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

[Jeff King]

On Mon, May 14, 2012 at 12:28:24PM -0400, Jeff King wrote:

> When we pull the user's name from the GECOS field of the
> passwd file (or generate an email address based on their
> username and hostname), we put the result into a
> static buffer. While it's extremely unlikely that anybody
> ever hit these limits (after all, in such a case their
> parents must have hated them), we still had to deal with the
> error cases in our code.
> 
> Converting these static buffers to strbufs lets us simplify
> the code and drop some error messages from the documentation
> that have confused some users.
> 
> Note that there is still one length limitation: the
> gethostname interface requires us to provide a static
> buffer, so we arbitrarily choose 1024 bytes for the
> hostname.
> 
> Signed-off-by: Jeff King <peff@peff.net>

Ick, there is something very wrong with this patch. While testing a
completely unrelated bug, I noticed that it set my name to "Jeff
KingJeff KingJeff King". Which, while a wonderful ego massage, is
probably excessive.

I'm sure the problem is the switch to strbuf's appending semantics
rather than strlcpy's overwriting semantics. I thought we were careful
not to bother re-run the gecos code if we had already gotten a name, but
obviously that is not the case in some code paths. I'll investigate.

-Peff

Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

[Jeff King]

On Mon, May 14, 2012 at 05:02:25PM -0400, Jeff King wrote:

> Ick, there is something very wrong with this patch. While testing a
> completely unrelated bug, I noticed that it set my name to "Jeff
> KingJeff KingJeff King". Which, while a wonderful ego massage, is
> probably excessive.
> 
> I'm sure the problem is the switch to strbuf's appending semantics
> rather than strlcpy's overwriting semantics. I thought we were careful
> not to bother re-run the gecos code if we had already gotten a name, but
> obviously that is not the case in some code paths. I'll investigate.

Ah, I see. The problem is here:

> --- a/config.c
> +++ b/config.c
> @@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)
>  	if (!strcmp(var, "user.name")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> -		strlcpy(git_default_name, value, sizeof(git_default_name));
> +		strbuf_addstr(&git_default_name, value);
>  		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
>  		return 0;
>  	}
> @@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)
>  	if (!strcmp(var, "user.email")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> -		strlcpy(git_default_email, value, sizeof(git_default_email));
> +		strbuf_addstr(&git_default_email, value);
>  		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;

where we are not careful. The fix is trivial. However, while examining
fmt_ident, I notice there is another potential spot there that needs
further investigation (I think it may actually be unreachable code, but
I need to look closer).

I'll re-roll the series with the fixes after investigating fmt_ident.

-Peff

Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

[Jeff King]

On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:

> where we are not careful. The fix is trivial. However, while examining
> fmt_ident, I notice there is another potential spot there that needs
> further investigation (I think it may actually be unreachable code, but
> I need to look closer).
> 
> I'll re-roll the series with the fixes after investigating fmt_ident.

Hmm. This code from fmt_ident is very odd:

> const char *fmt_ident(const char *name, const char *email,
> 		      const char *date_str, int flag)
> {
> [...]
> 	setup_ident(&name, &email);
> 
> 	if (!*name) {
> 		struct passwd *pw;
> 
> 		if ((warn_on_no_name || error_on_no_name) &&
> 		    name == git_default_name && env_hint) {
> 			fputs(env_hint, stderr);
> 			env_hint = NULL; /* warn only once */
> 		}
> 		if (error_on_no_name)
> 			die("empty ident %s <%s> not allowed", name, email);
> 		pw = getpwuid(getuid());
> 		if (!pw)
> 			die("You don't exist. Go away!");
> 		strlcpy(git_default_name, pw->pw_name,
> 			sizeof(git_default_name));
> 		name = git_default_name;
> 	}

We call setup_ident with our name pointer, which usually comes from
getenv("GIT_*_NAME"), although could also come from something like "git
commit -c $commit". We feed that to setup_ident. If name is NULL, then
setup_ident will use git_default_name (filling it in from gecos or
config). If it's not NULL, then we use it literally. And then we check
_that_ result to see if it's empty. If it is, we either die or warn,
depending on the flags. In the latter case, we fallback to using the
username as the name.

And that's what confuses me. Depending on what was passed in, we may
have checked that GIT_COMMITTER_NAME is an empty string, or we may have
checked that the config or gecos field yielded an empty string. In the
latter case, it makes sense to fall back to the username. But in the
former case, it doesn't; we should fall back to the config name or the
gecos name. And worse, we've polluted git_default_name for the rest of
the program run.

Instead of falling back to getpwuid(), should it fall back to:

   /* If this wasn't our default name already, then fall back to that. */
   if (name != git_default_name) {
           name = NULL;
           setup_ident(&name, &email);
   }

   /* If we _still_ don't have a non-empty name, then fall back to
    * username. */
   if (!*name) {
          pw = getpwuid(getuid());
          if (!pw)
                  die("You don't exist. Go away!");
          strlcpy(git_default_name, pw->pw_name, sizeof(git_default_name));
          nae = git_default_name;
   }

Of course we've still polluted this crappy fake name into
git_default_name, so that later calls with error_on_no_name will see it
and not error. I think so far it hasn't mattered because the only user
of this "warn" code is format-patch, which otherwise does not care about
ident (and doesn't even end up using the name at all!). And I doubt this
code path gets triggered much anyway; do people really run
"GIT_COMMITTER_NAME= git format-patch"?

I can just leave it as it's not really hurting anybody, I think. But I
was refactoring in the area and it just seemed flaky and questionable. I
wonder if we can simply get rid of the IDENT_WARN_ON_NO_NAME code path
entirely. The use here is grabbing the email address to use as part of a
message id. Could we just call setup_ident and then read from
git_default_email directly? There's no need to respect
GIT_COMMITTER_EMAIL here at all.

-Peff

Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

[Jeff King]

On Mon, May 14, 2012 at 09:54:37PM -0400, Jeff King wrote:

> Of course we've still polluted this crappy fake name into
> git_default_name, so that later calls with error_on_no_name will see it
> and not error. I think so far it hasn't mattered because the only user
> of this "warn" code is format-patch, which otherwise does not care about
> ident (and doesn't even end up using the name at all!). And I doubt this
> code path gets triggered much anyway; do people really run
> "GIT_COMMITTER_NAME= git format-patch"?
> 
> I can just leave it as it's not really hurting anybody, I think. But I
> was refactoring in the area and it just seemed flaky and questionable. I
> wonder if we can simply get rid of the IDENT_WARN_ON_NO_NAME code path
> entirely. The use here is grabbing the email address to use as part of a
> message id. Could we just call setup_ident and then read from
> git_default_email directly? There's no need to respect
> GIT_COMMITTER_EMAIL here at all.

Hmm, I was mistaken. This code path also gets followed whenever
IDENT_ERROR_ON_NO_NAME is not set (regardless of IDENT_WARN_ON_NO_NAME).
So other programs may accidentally get this pollution of
git_default_name and show a username when we _could_ have shown the name
from config. I can see the pollution in a debugger in "git commit", but
I don't think you can actually trigger a commit with it, because later
calls to fmt_ident use ERROR_ON_NO_NAME.

I really wonder if we can just get rid of all of the calls which do not
use ERROR_ON_NO_NAME. As far as I can tell, they are all part of
programs which later end up using ERROR_ON_NO_NAME anyway.

-Peff

Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:
>
> We call setup_ident with our name pointer, which usually comes from
> getenv("GIT_*_NAME"), although could also come from something like "git
> commit -c $commit". We feed that to setup_ident. If name is NULL, then
> setup_ident will use git_default_name (filling it in from gecos or
> config). If it's not NULL, then we use it literally. And then we check
> _that_ result to see if it's empty. If it is, we either die or warn,
> depending on the flags. In the latter case, we fallback to using the
> username as the name.
>
> And that's what confuses me. Depending on what was passed in, we may
> have checked that GIT_COMMITTER_NAME is an empty string, or we may have
> checked that the config or gecos field yielded an empty string. 

Sounds quite sensible to me, though.

> In the
> latter case, it makes sense to fall back to the username.

I agree that we should use something like "Sorry, Mr. McDonald" codepath
when the GECOS field returns an empty string---after all that is what we
do when we are built with NO_GECOS_IN_PWENT.

> But in the
> former case, it doesn't; we should fall back to the config name or the
> gecos name.

If the user said GIT_COMMITTER_NAME is empty with "GIT_COMMITTER_NAME=",
that is different from saying with "unset GIT_COMMITTER_NAME" that the
user does not want the environment to take effect, no?  So I do not think
falling back to configured or gecos in the former case is the right thing
to do, even though that would mean explicitly giving an empty string in
that configuration variable is asking only for an error without any
recourse, which is not useful at all.

Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

[Jeff King]

On Tue, May 15, 2012 at 08:03:38AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:
> >
> > We call setup_ident with our name pointer, which usually comes from
> > getenv("GIT_*_NAME"), although could also come from something like "git
> > commit -c $commit". We feed that to setup_ident. If name is NULL, then
> > setup_ident will use git_default_name (filling it in from gecos or
> > config). If it's not NULL, then we use it literally. And then we check
> > _that_ result to see if it's empty. If it is, we either die or warn,
> > depending on the flags. In the latter case, we fallback to using the
> > username as the name.
> >
> > And that's what confuses me. Depending on what was passed in, we may
> > have checked that GIT_COMMITTER_NAME is an empty string, or we may have
> > checked that the config or gecos field yielded an empty string. 
> 
> Sounds quite sensible to me, though.

Yes, I think it is OK to check what was given to us (or our fallback).
But using that check to decide which next step to take doesn't seem
right.

> > In the
> > latter case, it makes sense to fall back to the username.
> 
> I agree that we should use something like "Sorry, Mr. McDonald" codepath
> when the GECOS field returns an empty string---after all that is what we
> do when we are built with NO_GECOS_IN_PWENT.

Right, and that is more or less what we do (just without the
capitalization).

> > But in the
> > former case, it doesn't; we should fall back to the config name or the
> > gecos name.
> 
> If the user said GIT_COMMITTER_NAME is empty with "GIT_COMMITTER_NAME=",
> that is different from saying with "unset GIT_COMMITTER_NAME" that the
> user does not want the environment to take effect, no?

I agree two the cases are different. And for the most part, you are
insane to pass an empty GIT_COMMITTER_NAME. But if you do, why would the
right behavior be to fall back to sticking the username into the name
field, and not the gecos name?

Part of me is wondering why we should fall back at all in that case. If
a caller does not pass ERROR_ON_NO_NAME, then they don't really care
what the name is, do they? The current callers that do not pass it are:

  - blame.c:fake_working_tree_commit, which is passing in a fake name
    buffer anyway (so will never trigger this code path)

  - log.c:gen_message_id, which only cares about the email
    portion anyway

  - fmt-merge-msg.c:credit_people; this caller compares the name field
    to what's in the commits, checking for differences. So it could just
    as easily be "(none)" or some other token

  - commit.c:prepare_to_commit; this compares and shows author and
    commiter ids, and does not care about a blank name for the committer
    (but does for the author). The commit can't go through anyway with a
    blank committer name, so should it not just use ERROR_ON_NO_NAME?

  - log.c:make_cover_letter; this uses the committer information to make
    a fake commit that we ultimately use just to get the "%f" pretty
    userformat from it. In other words, we don't care about the
    committer at all, and this is really just working around an
    absolutely horrific interface.

  - refs.c:log_ref_write; finally, a caller who actually cares about the
    name, but doesn't want to die if we don't have a good name. We are
    happy enough with the username, though if somebody passes
    GIT_COMMITTER_NAME=, wouldn't it be OK to fail?

So it seems to me like a much simpler set of rules would be:

  1. When reading gecos, always fall back to the username if the gecos
     field is unavailable or blank.

  2. Always die when the name field is blank. That means we will die
     when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty
     config name), which makes a lot more sense to me than falling back;
     those are bogus requests, not system config problems.  And we won't
     ever have a blank gecos name, because we'll always fall back on the
     username.

Again, I'm sorry to belabor this, and we can just drop it; I don't think
there's currently a bug. It's just that I'm cleaning up in the area, and
the current behavior seems overly complex; in particular, I'm worried
that writing the username into the git_default_name field (overwriting
the _real_ name the user gave us!) is a maintenance time-bomb that will
bite us later.

If I'm not being clear, I can express it in the form of patches, which
might be more obvious.

-Peff

Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So it seems to me like a much simpler set of rules would be:
>
>   1. When reading gecos, always fall back to the username if the gecos
>      field is unavailable or blank.
>
>   2. Always die when the name field is blank. That means we will die
>      when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty
>      config name), which makes a lot more sense to me than falling back;
>      those are bogus requests, not system config problems.  And we won't
>      ever have a blank gecos name, because we'll always fall back on the
>      username.

That certainly sounds very simple to explain and understand, and I do not
offhand think of anything *sane* that would break ;-)

Thanks.

[PATCH 0/13] ident cleanups and bugfixes

[Jeff King]

On Tue, May 15, 2012 at 11:10:36AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So it seems to me like a much simpler set of rules would be:
> >
> >   1. When reading gecos, always fall back to the username if the gecos
> >      field is unavailable or blank.
> >
> >   2. Always die when the name field is blank. That means we will die
> >      when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty
> >      config name), which makes a lot more sense to me than falling back;
> >      those are bogus requests, not system config problems.  And we won't
> >      ever have a blank gecos name, because we'll always fall back on the
> >      username.
> 
> That certainly sounds very simple to explain and understand, and I do not
> offhand think of anything *sane* that would break ;-)

Actually, it does end up breaking. The "error_on_no_name" check wants to
see the value _before_ the fallback, because it is not just about "do
not hand out an ident with an empty name", but rather about "do not hand
out this crappy fallback name when the gecos field is empty".  So we
cannot do an "always fallback" behavior without breaking that check.

I ended up avoiding the issue in a different way, as you'll see in patch
8 below. Here's the series:

  [01/13]: ident: split setup_ident into separate functions
  [02/13]: http-push: do not access git_default_email directly
  [03/13]: fmt-merge-msg: don't use static buffer in record_person
  [04/13]: move identity config parsing to ident.c
  [05/13]: move git_default_* variables to ident.c
  [06/13]: format-patch: use default email for generating message ids
  [07/13]: fmt_ident: drop IDENT_WARN_ON_NO_NAME code
  [08/13]: ident: don't write fallback username into git_default_name
  [09/13]: drop length limitations on gecos-derived names and emails
  [10/13]: ident: report passwd errors with a more friendly message
  [11/13]: ident: use full dns names to generate email addresses
  [12/13]: ident: use a dynamic strbuf in fmt_ident
  [13/13]: format-patch: refactor get_patch_filename

Patches 2, 8, and 11 fix actual bugs. The rest of it is refactoring and
cleanup; here's the diffstat:

 Documentation/git-commit-tree.txt |    9 --
 Documentation/git-var.txt         |    9 --
 builtin/fmt-merge-msg.c           |    8 +-
 builtin/log.c                     |   45 ++------
 cache.h                           |   12 +-
 config.c                          |   24 +----
 environment.c                     |    3 -
 git-compat-util.h                 |    3 +
 http-push.c                       |    2 +-
 ident.c                           |  213 ++++++++++++++++---------------------
 log-tree.c                        |   19 ++--
 log-tree.h                        |    4 +-
 wrapper.c                         |   12 ++
 13 files changed, 142 insertions(+), 221 deletions(-)

-Peff

[PATCH 01/13] ident: split setup_ident into separate functions

[Jeff King]

This function sets up the default name, email, and date, and
is not publicly available. Let's split it into three public
functions so that callers can get just the parts they need.

While we're at it, let's change the interface to simple
accessors. The original function was called only by fmt_ident,
and contained logic for "if we already have some other
value, don't load the default" which properly belongs in
fmt_ident.

Signed-off-by: Jeff King <peff@peff.net>
---
The patch is a pain to read because it's splitting an existing function
in 3; just reading the result is much easier.

 cache.h |  3 +++
 ident.c | 35 +++++++++++++++++++----------------
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/cache.h b/cache.h
index e14ffcd..0c095d4 100644
--- a/cache.h
+++ b/cache.h
@@ -894,6 +894,9 @@ 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 *ident_default_name(void);
+extern const char *ident_default_email(void);
+extern const char *ident_default_date(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 
diff --git a/ident.c b/ident.c
index 87c697c..0f7dcae 100644
--- a/ident.c
+++ b/ident.c
@@ -117,21 +117,20 @@ static void copy_email(const struct passwd *pw)
 			sizeof(git_default_email) - len);
 }
 
-static void setup_ident(const char **name, const char **emailp)
+const char *ident_default_name(void)
 {
-	struct passwd *pw = NULL;
-
-	/* Get the name ("gecos") */
-	if (!*name && !git_default_name[0]) {
-		pw = getpwuid(getuid());
+	if (!git_default_name[0]) {
+		struct passwd *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;
+	return git_default_name;
+}
 
-	if (!*emailp && !git_default_email[0]) {
+const char *ident_default_email(void)
+{
+	if (!git_default_email[0]) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
@@ -139,19 +138,20 @@ static void setup_ident(const char **name, const char **emailp)
 				sizeof(git_default_email));
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else {
-			if (!pw)
-				pw = getpwuid(getuid());
+			struct passwd *pw = getpwuid(getuid());
 			if (!pw)
 				die("You don't exist. Go away!");
 			copy_email(pw);
 		}
 	}
-	if (!*emailp)
-		*emailp = git_default_email;
+	return git_default_email;
+}
 
-	/* And set the default date */
+const char *ident_default_date(void)
+{
 	if (!git_default_date[0])
 		datestamp(git_default_date, sizeof(git_default_date));
+	return git_default_date;
 }
 
 static int add_raw(char *buf, size_t size, int offset, const char *str)
@@ -311,7 +311,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);
 
-	setup_ident(&name, &email);
+	if (!name)
+		name = ident_default_name();
+	if (!email)
+		email = ident_default_email();
 
 	if (!*name) {
 		struct passwd *pw;
@@ -331,7 +334,7 @@ const char *fmt_ident(const char *name, const char *email,
 		name = git_default_name;
 	}
 
-	strcpy(date, git_default_date);
+	strcpy(date, ident_default_date());
 	if (!name_addr_only && date_str && date_str[0]) {
 		if (parse_date(date_str, date, sizeof(date)) < 0)
 			die("invalid date format: %s", date_str);
-- 
1.7.10.1.16.g53a707b

[PATCH 02/13] http-push: do not access git_default_email directly

[Jeff King]

By calling the ident_default_email accessor, we can be sure
that the default value is actually filled-in.

Signed-off-by: Jeff King <peff@peff.net>
---
I believe this is a real, triggerable bug if you don't have your
user.email config set, but I didn't actually test it (and I have no idea
if DAV would actually care about an empty email here anyway).

 http-push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index 1df7ab5..a832ca7 100644
--- a/http-push.c
+++ b/http-push.c
@@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		ep = strchr(ep + 1, '/');
 	}
 
-	escaped = xml_entities(git_default_email);
+	escaped = xml_entities(ident_default_email());
 	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
 	free(escaped);
 
-- 
1.7.10.1.16.g53a707b

[PATCH 03/13] fmt-merge-msg: don't use static buffer in record_person

[Jeff King]

The record_person function just parses out the "name" field
of the person line in a commit and adds it to a string_list.
The only reason we need an extra buffer is that the
string_list functions require a NUL-terminated string.

Instead of the static buffer, we can just allocate a
temporary NUL-terminated copy. In addition to removing a
useless limit, this removes the only user of MAX_GITNAME
outside of ident.c.

Signed-off-by: Jeff King <peff@peff.net>
---
This actually introduces an extra malloc() per commit that we analyze. I
doubt we care, but if we do, the right solution IMHO is to introduce
string-list functions which can lookup or add a (buf, len) pair and
avoid the copy altogether.

 builtin/fmt-merge-msg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index a517f17..4ee6a88 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -230,7 +230,7 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 static void record_person(int which, struct string_list *people,
 			  struct commit *commit)
 {
-	char name_buf[MAX_GITNAME], *name, *name_end;
+	char *name_buf, *name, *name_end;
 	struct string_list_item *elem;
 	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
 
@@ -243,10 +243,9 @@ static void record_person(int which, struct string_list *people,
 		name_end--;
 	while (isspace(*name_end) && name <= name_end)
 		name_end--;
-	if (name_end < name || name + MAX_GITNAME <= name_end)
+	if (name_end < name)
 		return;
-	memcpy(name_buf, name, name_end - name + 1);
-	name_buf[name_end - name + 1] = '\0';
+	name_buf = xmemdupz(name, name_end - name + 1);
 
 	elem = string_list_lookup(people, name_buf);
 	if (!elem) {
@@ -254,6 +253,7 @@ static void record_person(int which, struct string_list *people,
 		elem->util = (void *)0;
 	}
 	elem->util = (void*)(util_as_integral(elem) + 1);
+	free(name_buf);
 }
 
 static int cmp_string_list_util_as_integral(const void *a_, const void *b_)
-- 
1.7.10.1.16.g53a707b

[PATCH 04/13] move identity config parsing to ident.c

[Jeff King]

There's no reason for this to be in config, except that once
upon a time all of the config parsing was there. It makes
more sense to keep the ident code together.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h  |  1 +
 config.c | 24 +-----------------------
 ident.c  | 21 +++++++++++++++++++++
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/cache.h b/cache.h
index 0c095d4..86224c8 100644
--- a/cache.h
+++ b/cache.h
@@ -899,6 +899,7 @@ extern const char *ident_default_email(void);
 extern const char *ident_default_date(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
+extern int git_ident_config(const char *, const char *, void *);
 
 struct ident_split {
 	const char *name_begin;
diff --git a/config.c b/config.c
index eeee986..71ef171 100644
--- a/config.c
+++ b/config.c
@@ -762,28 +762,6 @@ static int git_default_core_config(const char *var, const char *value)
 	return 0;
 }
 
-static int git_default_user_config(const char *var, const char *value)
-{
-	if (!strcmp(var, "user.name")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strlcpy(git_default_name, value, sizeof(git_default_name));
-		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
-		return 0;
-	}
-
-	if (!strcmp(var, "user.email")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strlcpy(git_default_email, value, sizeof(git_default_email));
-		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		return 0;
-	}
-
-	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
-}
-
 static int git_default_i18n_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "i18n.commitencoding"))
@@ -870,7 +848,7 @@ int git_default_config(const char *var, const char *value, void *dummy)
 		return git_default_core_config(var, value);
 
 	if (!prefixcmp(var, "user."))
-		return git_default_user_config(var, value);
+		return git_ident_config(var, value, dummy);
 
 	if (!prefixcmp(var, "i18n."))
 		return git_default_i18n_config(var, value);
diff --git a/ident.c b/ident.c
index 0f7dcae..bb1158f 100644
--- a/ident.c
+++ b/ident.c
@@ -388,3 +388,24 @@ int user_ident_sufficiently_given(void)
 	return (user_ident_explicitly_given == IDENT_ALL_GIVEN);
 #endif
 }
+
+int git_ident_config(const char *var, const char *value, void *data)
+{
+	if (!strcmp(var, "user.name")) {
+		if (!value)
+			return config_error_nonbool(var);
+		strlcpy(git_default_name, value, sizeof(git_default_name));
+		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		return 0;
+	}
+
+	if (!strcmp(var, "user.email")) {
+		if (!value)
+			return config_error_nonbool(var);
+		strlcpy(git_default_email, value, sizeof(git_default_email));
+		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		return 0;
+	}
+
+	return 0;
+}
-- 
1.7.10.1.16.g53a707b

[PATCH 05/13] move git_default_* variables to ident.c

[Jeff King]

There's no reason anybody outside of ident.c should access
these directly (they should use the new accessors which make
sure the variables are initialized), so we can make them
file-scope statics.

While we're at it, move user_ident_explicitly_given into
ident.c; while still globally visible, it makes more sense
to reside with the ident code.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h       | 3 ---
 environment.c | 3 ---
 ident.c       | 4 ++++
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 86224c8..f63b71f 100644
--- a/cache.h
+++ b/cache.h
@@ -1142,9 +1142,6 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-#define MAX_GITNAME (1000)
-extern char git_default_email[MAX_GITNAME];
-extern char git_default_name[MAX_GITNAME];
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
diff --git a/environment.c b/environment.c
index d7e6c65..669e498 100644
--- a/environment.c
+++ b/environment.c
@@ -11,9 +11,6 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 
-char git_default_email[MAX_GITNAME];
-char git_default_name[MAX_GITNAME];
-int user_ident_explicitly_given;
 int trust_executable_bit = 1;
 int trust_ctime = 1;
 int has_symlinks = 1;
diff --git a/ident.c b/ident.c
index bb1158f..af92b2c 100644
--- a/ident.c
+++ b/ident.c
@@ -7,7 +7,11 @@
  */
 #include "cache.h"
 
+#define MAX_GITNAME (1000)
+static char git_default_name[MAX_GITNAME];
+static char git_default_email[MAX_GITNAME];
 static char git_default_date[50];
+int user_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
-- 
1.7.10.1.16.g53a707b

[PATCH 06/13] format-patch: use default email for generating message ids

[Jeff King]

We try to generate a sane message id for cover letters and
threading by appending some changing bits to the front of
the user's email address. The current code parses the email
out of the results of git_committer_info, but we can do this
much more easily by just calling ident_default_email
ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
Technically this is a regression if you really wanted:

  GIT_COMMITTER_EMAIL=some.addr@example.com \
  git format-patch --thread=deep

to make your environment variable part of the message-ids. I don't think
it matters, but I can adjust it if we care.

 builtin/log.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 690caa7..656bddf 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -737,15 +737,9 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha
 
 static void gen_message_id(struct rev_info *info, char *base)
 {
-	const char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME);
-	const char *email_start = strrchr(committer, '<');
-	const char *email_end = strrchr(committer, '>');
 	struct strbuf buf = STRBUF_INIT;
-	if (!email_start || !email_end || email_start > email_end - 1)
-		die(_("Could not extract email from committer identity."));
-	strbuf_addf(&buf, "%s.%lu.git.%.*s", base,
-		    (unsigned long) time(NULL),
-		    (int)(email_end - email_start - 1), email_start + 1);
+	strbuf_addf(&buf, "%s.%lu.git.%s", base,
+		    (unsigned long) time(NULL), ident_default_email());
 	info->message_id = strbuf_detach(&buf, NULL);
 }
 
-- 
1.7.10.1.16.g53a707b

[PATCH 07/13] fmt_ident: drop IDENT_WARN_ON_NO_NAME code

[Jeff King]

There are no more callers who want this, so we can drop it.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |  5 ++---
 ident.c | 11 ++++-------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index f63b71f..65cbab5 100644
--- a/cache.h
+++ b/cache.h
@@ -887,9 +887,8 @@ unsigned long approxidate_careful(const char *, int *);
 unsigned long approxidate_relative(const char *date, const struct timeval *now);
 enum date_mode parse_date_format(const char *format);
 
-#define IDENT_WARN_ON_NO_NAME  1
-#define IDENT_ERROR_ON_NO_NAME 2
-#define IDENT_NO_DATE	       4
+#define IDENT_ERROR_ON_NO_NAME 1
+#define IDENT_NO_DATE	       2
 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);
diff --git a/ident.c b/ident.c
index af92b2c..d2fa271 100644
--- a/ident.c
+++ b/ident.c
@@ -312,7 +312,6 @@ const char *fmt_ident(const char *name, const char *email,
 	char date[50];
 	int i;
 	int error_on_no_name = (flag & IDENT_ERROR_ON_NO_NAME);
-	int warn_on_no_name = (flag & IDENT_WARN_ON_NO_NAME);
 	int name_addr_only = (flag & IDENT_NO_DATE);
 
 	if (!name)
@@ -323,13 +322,11 @@ const char *fmt_ident(const char *name, const char *email,
 	if (!*name) {
 		struct passwd *pw;
 
-		if ((warn_on_no_name || error_on_no_name) &&
-		    name == git_default_name && env_hint) {
-			fputs(env_hint, stderr);
-			env_hint = NULL; /* warn only once */
-		}
-		if (error_on_no_name)
+		if (error_on_no_name) {
+			if (name == git_default_name)
+				fputs(env_hint, stderr);
 			die("empty ident %s <%s> not allowed", name, email);
+		}
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-- 
1.7.10.1.16.g53a707b

[PATCH 08/13] ident: don't write fallback username into git_default_name

[Jeff King]

The fmt_ident function gets a flag that tells us whether to
die if the name field is blank. If it is blank and we don't
die, then we fall back to the username from the passwd file.

The current code writes the value into git_default_name.
However, that's not necessarily correct, as the empty value
might have come from git_default_name, or it might have been
passed in.  This leads to two potential problems:

  1. If we are overriding an empty name in the passed-in
     value, then we may be overwriting a perfectly good name
     (from gitconfig or gecos) in the git_default_name
     buffer. Later calls to fmt_ident will end up using the
     fallback name, even though a better name was available.

  2. If we override an empty gecos name, we end up with the
     fallback name in git_default_name. A later call that
     uses IDENT_ERROR_ON_NO_NAME will see the fallback name
     and think that it is a good name, instead of producing
     an error. In other words, a blank gecos name would
     cause an error with this code:

       git_committer_info(IDENT_ERROR_ON_NO_NAME);

     but not this:

       git_committer_info(0);
       git_committer_info(IDENT_ERROR_ON_NO_NAME);

     because in the latter case, the first call has polluted
     the name buffer.

Instead, let's make the fallback a per-invocation variable.
We can just use the pw->pw_name string directly, since it
only needs to persist through the rest of the function (and
we don't do any other getpwent calls).

Note that while this solves (1) for future invocations of
fmt_indent, the current invocation might use the fallback
when it could in theory load a better value from
git_default_name. However, by not passing
IDENT_ERROR_ON_NO_NAME, the caller is indicating that it
does not care too much about the name, anyway, so we don't
bother; this is primarily about protecting future callers
who do care.

Signed-off-by: Jeff King <peff@peff.net>
---
You can trigger bug (2) by having an empty gecos field, no user.name,
and running a merge. The recent credit_person code path will call
git_committer_info(0), then the commit-generating code path will call
git_committer_info(IDENT_ERROR_ON_NO_NAME), and you will end
up with a commit with your username in the "name" field (even though the
second call would want to error out).

I don't think (1) is triggerable in the current code, mainly because we
typically are feeding getenv("GIT_*_NAME") to fmt_ident, and a blank
there will end up causing an error later on in the program.

By the way, I left it so that:

  GIT_COMMITTER_NAME= git ...

will continue to fallback to the passwd username if a caller doesn't
request to die on a blank name. We could pretty easily die in that case,
but people with flaky scripts might be affected. I figured that callers
who don't give ERROR_ON_NO_NAME don't really care too much what's in the
name anyway, so there's not much point in tightening the rules.

 ident.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ident.c b/ident.c
index d2fa271..f44bcb3 100644
--- a/ident.c
+++ b/ident.c
@@ -330,9 +330,7 @@ const char *fmt_ident(const char *name, const char *email,
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		strlcpy(git_default_name, pw->pw_name,
-			sizeof(git_default_name));
-		name = git_default_name;
+		name = pw->pw_name;
 	}
 
 	strcpy(date, ident_default_date());
-- 
1.7.10.1.16.g53a707b

[PATCH 09/13] drop length limitations on gecos-derived names and emails

[Jeff King]

When we pull the user's name from the GECOS field of the
passwd file (or generate an email address based on their
username and hostname), we put the result into a
static buffer. While it's extremely unlikely that anybody
ever hit these limits (after all, in such a case their
parents must have hated them), we still had to deal with the
error cases in our code.

Converting these static buffers to strbufs lets us simplify
the code and drop some error messages from the documentation
that have confused some users.

The conversion is mostly mechanical: replace string copies
with strbuf equivalents, and access the strbuf.buf directly.
There are a few exceptions:

  - copy_gecos and copy_email are the big winners in code
    reduction (since they no longer have to manage the
    string length manually)

  - git_ident_config wants to replace old versions of
    the default name (e.g., if we read the config multiple
    times), so it must reset+add to the strbuf instead of
    just adding

Note that there is still one length limitation: the
gethostname interface requires us to provide a static
buffer, so we arbitrarily choose 1024 bytes for the
hostname.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-commit-tree.txt |   4 --
 Documentation/git-var.txt         |   4 --
 ident.c                           | 100 +++++++++++++++-----------------------
 3 files changed, 39 insertions(+), 69 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index cfb9906..eb12b2d 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -92,10 +92,6 @@ Diagnostics
 -----------
 You don't exist. Go away!::
     The passwd(5) gecos field couldn't be read
-Your parents must have hated you!::
-    The passwd(5) gecos field is longer than a giant static buffer.
-Your sysadmin must hate you!::
-    The passwd(5) name field is longer than a giant static buffer.
 
 Discussion
 ----------
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 988a323..3f703e3 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -63,10 +63,6 @@ Diagnostics
 -----------
 You don't exist. Go away!::
     The passwd(5) gecos field couldn't be read
-Your parents must have hated you!::
-    The passwd(5) gecos field is longer than a giant static buffer.
-Your sysadmin must hate you!::
-    The passwd(5) name field is longer than a giant static buffer.
 
 SEE ALSO
 --------
diff --git a/ident.c b/ident.c
index f44bcb3..73a06a1 100644
--- a/ident.c
+++ b/ident.c
@@ -7,9 +7,8 @@
  */
 #include "cache.h"
 
-#define MAX_GITNAME (1000)
-static char git_default_name[MAX_GITNAME];
-static char git_default_email[MAX_GITNAME];
+static struct strbuf git_default_name = STRBUF_INIT;
+static struct strbuf git_default_email = STRBUF_INIT;
 static char git_default_date[50];
 int user_ident_explicitly_given;
 
@@ -19,42 +18,27 @@ int user_ident_explicitly_given;
 #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
 #endif
 
-static void copy_gecos(const struct passwd *w, char *name, size_t sz)
+static void copy_gecos(const struct passwd *w, struct strbuf *name)
 {
-	char *src, *dst;
-	size_t len, nlen;
-
-	nlen = strlen(w->pw_name);
+	char *src;
 
 	/* Traditionally GECOS field had office phone numbers etc, separated
 	 * with commas.  Also & stands for capitalized form of the login name.
 	 */
 
-	for (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {
+	for (src = get_gecos(w); *src && *src != ','; src++) {
 		int ch = *src;
-		if (ch != '&') {
-			*dst++ = ch;
-			if (ch == 0 || ch == ',')
-				break;
-			len++;
-			continue;
-		}
-		if (len + nlen < sz) {
+		if (ch != '&')
+			strbuf_addch(name, ch);
+		else {
 			/* Sorry, Mr. McDonald... */
-			*dst++ = toupper(*w->pw_name);
-			memcpy(dst, w->pw_name + 1, nlen - 1);
-			dst += nlen - 1;
-			len += nlen;
+			strbuf_addch(name, toupper(*w->pw_name));
+			strbuf_addstr(name, w->pw_name + 1);
 		}
 	}
-	if (len < sz)
-		name[len] = 0;
-	else
-		die("Your parents must have hated you!");
-
 }
 
-static int add_mailname_host(char *buf, size_t len)
+static int add_mailname_host(struct strbuf *buf)
 {
 	FILE *mailname;
 
@@ -65,7 +49,7 @@ static int add_mailname_host(char *buf, size_t len)
 				strerror(errno));
 		return -1;
 	}
-	if (!fgets(buf, len, mailname)) {
+	if (strbuf_getline(buf, mailname, '\n') == EOF) {
 		if (ferror(mailname))
 			warning("cannot read /etc/mailname: %s",
 				strerror(errno));
@@ -77,78 +61,70 @@ static int add_mailname_host(char *buf, size_t len)
 	return 0;
 }
 
-static void add_domainname(char *buf, size_t len)
+static void add_domainname(struct strbuf *out)
 {
+	char buf[1024];
 	struct hostent *he;
-	size_t namelen;
 	const char *domainname;
 
-	if (gethostname(buf, len)) {
+	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 		return;
 	}
-	namelen = strlen(buf);
-	if (memchr(buf, '.', namelen))
+	strbuf_addstr(out, buf);
+	if (strchr(buf, '.'))
 		return;
 
 	he = gethostbyname(buf);
-	buf[namelen++] = '.';
-	buf += namelen;
-	len -= namelen;
+	strbuf_addch(out, '.');
 	if (he && (domainname = strchr(he->h_name, '.')))
-		strlcpy(buf, domainname + 1, len);
+		strbuf_addstr(out, domainname + 1);
 	else
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 }
 
-static void copy_email(const struct passwd *pw)
+static void copy_email(const struct passwd *pw, struct strbuf *email)
 {
 	/*
 	 * Make up a fake email address
 	 * (name + '@' + hostname [+ '.' + domainname])
 	 */
-	size_t len = strlen(pw->pw_name);
-	if (len > sizeof(git_default_email)/2)
-		die("Your sysadmin must hate you!");
-	memcpy(git_default_email, pw->pw_name, len);
-	git_default_email[len++] = '@';
-
-	if (!add_mailname_host(git_default_email + len,
-				sizeof(git_default_email) - len))
+	strbuf_addstr(email, pw->pw_name);
+	strbuf_addch(email, '@');
+
+	if (!add_mailname_host(email))
 		return;	/* read from "/etc/mailname" (Debian) */
-	add_domainname(git_default_email + len,
-			sizeof(git_default_email) - len);
+	add_domainname(email);
 }
 
 const char *ident_default_name(void)
 {
-	if (!git_default_name[0]) {
+	if (!git_default_name.len) {
 		struct passwd *pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		copy_gecos(pw, git_default_name, sizeof(git_default_name));
+		copy_gecos(pw, &git_default_name);
 	}
-	return git_default_name;
+	return git_default_name.buf;
 }
 
 const char *ident_default_email(void)
 {
-	if (!git_default_email[0]) {
+	if (!git_default_email.len) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
-			strlcpy(git_default_email, email,
-				sizeof(git_default_email));
+			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else {
 			struct passwd *pw = getpwuid(getuid());
 			if (!pw)
 				die("You don't exist. Go away!");
-			copy_email(pw);
+			copy_email(pw, &git_default_email);
 		}
 	}
-	return git_default_email;
+	return git_default_email.buf;
 }
 
 const char *ident_default_date(void)
@@ -323,7 +299,7 @@ const char *fmt_ident(const char *name, const char *email,
 		struct passwd *pw;
 
 		if (error_on_no_name) {
-			if (name == git_default_name)
+			if (name == git_default_name.buf)
 				fputs(env_hint, stderr);
 			die("empty ident %s <%s> not allowed", name, email);
 		}
@@ -393,7 +369,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_name, value, sizeof(git_default_name));
+		strbuf_reset(&git_default_name);
+		strbuf_addstr(&git_default_name, value);
 		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
@@ -401,7 +378,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 	if (!strcmp(var, "user.email")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_email, value, sizeof(git_default_email));
+		strbuf_reset(&git_default_email);
+		strbuf_addstr(&git_default_email, value);
 		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
-- 
1.7.10.1.16.g53a707b

[PATCH 10/13] ident: report passwd errors with a more friendly message

[Jeff King]

When getpwuid fails, we give a cute but cryptic message.
While it makes sense if you know that getpwuid or identity
functions are being called, this code is triggered behind
the scenes by quite a few git commands these days (e.g.,
receive-pack on a remote server might use it for a reflog;
the current message is hard to distinguish from an
authentication error).  Let's switch to something that gives
a little more context.

While we're at it, we can factor out all of the
cut-and-pastes of the "you don't exist" message into a
wrapper function. Rather than provide xgetpwuid, let's make
it even more specific to just getting the passwd entry for
the current uid. That's the only way we use getpwuid anyway,
and it lets us make an even more specific error message.

The current message also fails to mention errno. While the
usual cause for getpwuid failing is that the user does not
exist, mentioning errno makes it easier to diagnose these
problems.  Note that POSIX specifies that errno remain
untouched if the passwd entry does not exist (but will be
set on actual errors), whereas some systems will return
ENOENT or similar for a missing entry. We handle both cases
in our wrapper.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-commit-tree.txt |  5 -----
 Documentation/git-var.txt         |  5 -----
 git-compat-util.h                 |  3 +++
 ident.c                           | 20 +++++---------------
 wrapper.c                         | 12 ++++++++++++
 5 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index eb12b2d..eb8ee99 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -88,11 +88,6 @@ for one to be entered and terminated with ^D.
 
 include::date-formats.txt[]
 
-Diagnostics
------------
-You don't exist. Go away!::
-    The passwd(5) gecos field couldn't be read
-
 Discussion
 ----------
 
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 3f703e3..67edf58 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -59,11 +59,6 @@ ifdef::git-default-pager[]
     The build you are using chose '{git-default-pager}' as the default.
 endif::git-default-pager[]
 
-Diagnostics
------------
-You don't exist. Go away!::
-    The passwd(5) gecos field couldn't be read
-
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/git-compat-util.h b/git-compat-util.h
index ed11ad8..5bd9ad7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -595,4 +595,7 @@ int rmdir_or_warn(const char *path);
  */
 int remove_or_warn(unsigned int mode, const char *path);
 
+/* Get the passwd entry for the UID of the current process. */
+struct passwd *xgetpwuid_self(void);
+
 #endif
diff --git a/ident.c b/ident.c
index 73a06a1..5aec073 100644
--- a/ident.c
+++ b/ident.c
@@ -100,12 +100,8 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
 
 const char *ident_default_name(void)
 {
-	if (!git_default_name.len) {
-		struct passwd *pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
-		copy_gecos(pw, &git_default_name);
-	}
+	if (!git_default_name.len)
+		copy_gecos(xgetpwuid_self(), &git_default_name);
 	return git_default_name.buf;
 }
 
@@ -117,12 +113,8 @@ const char *ident_default_email(void)
 		if (email && email[0]) {
 			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		} else {
-			struct passwd *pw = getpwuid(getuid());
-			if (!pw)
-				die("You don't exist. Go away!");
-			copy_email(pw, &git_default_email);
-		}
+		} else
+			copy_email(xgetpwuid_self(), &git_default_email);
 	}
 	return git_default_email.buf;
 }
@@ -303,9 +295,7 @@ const char *fmt_ident(const char *name, const char *email,
 				fputs(env_hint, stderr);
 			die("empty ident %s <%s> not allowed", name, email);
 		}
-		pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
+		pw = xgetpwuid_self();
 		name = pw->pw_name;
 	}
 
diff --git a/wrapper.c b/wrapper.c
index 6ccd059..b5e33e4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -402,3 +402,15 @@ int remove_or_warn(unsigned int mode, const char *file)
 {
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
+
+struct passwd *xgetpwuid_self(void)
+{
+	struct passwd *pw;
+
+	errno = 0;
+	pw = getpwuid(getuid());
+	if (!pw)
+		die(_("unable to look up current user in the passwd file: %s"),
+		    errno ? strerror(errno) : _("no such user"));
+	return pw;
+}
-- 
1.7.10.1.16.g53a707b

[PATCH 11/13] ident: use full dns names to generate email addresses

[Jeff King]

When we construct an email address from the username and
hostname, we generate the host part of the email with this
procedure:

  1. add the result of gethostname

  2. if it has a dot, ok, it's fully qualified

  3. if not, then look up the unqualified hostname via
     gethostbyname; take the domain name of the result and
     append it to the hostname

Step 3 can actually produce a bogus result, as the name
returned by gethostbyname may not be related to the hostname
we fed it (e.g., consider a machine "foo" with names
"foo.one.example.com" and "bar.two.example.com"; we may have
the latter returned and generate the bogus name
"foo.two.example.com").

This patch simply uses the full hostname returned by
gethostbyname. In the common case that the first part is the
same as the unqualified hostname, the behavior is identical.
And in the case that it is not the same, we are much more
likely to be generating a valid name.

Signed-off-by: Jeff King <peff@peff.net>
---
It takes a pretty odd /etc/hosts setup, but I was able to trigger this
bug. I doubt anyone is affected in practice, but hey, the right code is
5 lines shorter (and easier to understand).

 ident.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/ident.c b/ident.c
index 5aec073..b111e34 100644
--- a/ident.c
+++ b/ident.c
@@ -65,23 +65,18 @@ static void add_domainname(struct strbuf *out)
 {
 	char buf[1024];
 	struct hostent *he;
-	const char *domainname;
 
 	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
 		strbuf_addstr(out, "(none)");
 		return;
 	}
-	strbuf_addstr(out, buf);
 	if (strchr(buf, '.'))
-		return;
-
-	he = gethostbyname(buf);
-	strbuf_addch(out, '.');
-	if (he && (domainname = strchr(he->h_name, '.')))
-		strbuf_addstr(out, domainname + 1);
+		strbuf_addstr(out, buf);
+	else if ((he = gethostbyname(buf)) && strchr(he->h_name, '.'))
+		strbuf_addstr(out, he->h_name);
 	else
-		strbuf_addstr(out, "(none)");
+		strbuf_addf(out, "%s.(none)", buf);
 }
 
 static void copy_email(const struct passwd *pw, struct strbuf *email)
-- 
1.7.10.1.16.g53a707b

[PATCH 12/13] ident: use a dynamic strbuf in fmt_ident

[Jeff King]

Now that we accept arbitrary-sized names and email
addresses, the only remaining limit is in the actual
formatting of the names into a buffer. The current limit is
1000 characters, which is not likely to be reached, but
using a strbuf is one less error condition we have to worry
about.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/ident.c b/ident.c
index b111e34..cefb829 100644
--- a/ident.c
+++ b/ident.c
@@ -121,15 +121,6 @@ const char *ident_default_date(void)
 	return git_default_date;
 }
 
-static int add_raw(char *buf, size_t size, int offset, const char *str)
-{
-	size_t len = strlen(str);
-	if (offset + len > size)
-		return size;
-	memcpy(buf + offset, str, len);
-	return offset + len;
-}
-
 static int crud(unsigned char c)
 {
 	return  c <= 32  ||
@@ -148,7 +139,7 @@ static int crud(unsigned char c)
  * Copy over a string to the destination, but avoid special
  * characters ('\n', '<' and '>') and remove crud at the end
  */
-static int copy(char *buf, size_t size, int offset, const char *src)
+static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src)
 {
 	size_t i, len;
 	unsigned char c;
@@ -172,19 +163,19 @@ static int copy(char *buf, size_t size, int offset, const char *src)
 	/*
 	 * Copy the rest to the buffer, but avoid the special
 	 * characters '\n' '<' and '>' that act as delimiters on
-	 * an identification line
+	 * an identification line. We can only remove crud, never add it,
+	 * so 'len' is our maximum.
 	 */
+	strbuf_grow(sb, len);
 	for (i = 0; i < len; i++) {
 		c = *src++;
 		switch (c) {
 		case '\n': case '<': case '>':
 			continue;
 		}
-		if (offset >= size)
-			return size;
-		buf[offset++] = c;
+		sb->buf[sb->len++] = c;
 	}
-	return offset;
+	sb->buf[sb->len] = '\0';
 }
 
 /*
@@ -271,9 +262,8 @@ static const char *env_hint =
 const char *fmt_ident(const char *name, const char *email,
 		      const char *date_str, int flag)
 {
-	static char buffer[1000];
+	static struct strbuf ident = STRBUF_INIT;
 	char date[50];
-	int i;
 	int error_on_no_name = (flag & IDENT_ERROR_ON_NO_NAME);
 	int name_addr_only = (flag & IDENT_NO_DATE);
 
@@ -300,19 +290,16 @@ const char *fmt_ident(const char *name, const char *email,
 			die("invalid date format: %s", date_str);
 	}
 
-	i = copy(buffer, sizeof(buffer), 0, name);
-	i = add_raw(buffer, sizeof(buffer), i, " <");
-	i = copy(buffer, sizeof(buffer), i, email);
+	strbuf_reset(&ident);
+	strbuf_addstr_without_crud(&ident, name);
+	strbuf_addstr(&ident, " <");
+	strbuf_addstr_without_crud(&ident, email);
+	strbuf_addch(&ident, '>');
 	if (!name_addr_only) {
-		i = add_raw(buffer, sizeof(buffer), i,  "> ");
-		i = copy(buffer, sizeof(buffer), i, date);
-	} else {
-		i = add_raw(buffer, sizeof(buffer), i, ">");
+		strbuf_addch(&ident, ' ');
+		strbuf_addstr_without_crud(&ident, date);
 	}
-	if (i >= sizeof(buffer))
-		die("Impossibly long personal identifier");
-	buffer[i] = 0;
-	return buffer;
+	return ident.buf;
 }
 
 const char *fmt_name(const char *name, const char *email)
-- 
1.7.10.1.16.g53a707b

[PATCH 13/13] format-patch: refactor get_patch_filename

[Jeff King]

The get_patch_filename function expects a commit argument
and uses it to get the sanitized subject line when making a
patch filename. However, we also want to use this same
function for the cover letter, which does not have a commit
object. The current solution is to create a fake commit with
the subject "cover letter". Instead, let's make the
get_patch_filename interface more flexibile, and allow
passing a direct subject.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is a bonus. It doesn't really have anything to do with ident,
except that I came across it while trying to figure out how the
committer info is being used in make_cover_letter.

 builtin/log.c | 35 +++++++----------------------------
 log-tree.c    | 19 +++++++++++--------
 log-tree.h    |  4 ++--
 3 files changed, 20 insertions(+), 38 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 656bddf..8010a40 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -663,7 +663,8 @@ static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
-static int reopen_stdout(struct commit *commit, struct rev_info *rev, int quiet)
+static int reopen_stdout(struct commit *commit, const char *subject,
+			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
 	int suffix_len = strlen(fmt_patch_suffix) + 1;
@@ -677,7 +678,7 @@ static int reopen_stdout(struct commit *commit, struct rev_info *rev, int quiet)
 			strbuf_addch(&filename, '/');
 	}
 
-	get_patch_filename(commit, rev->nr, fmt_patch_suffix, &filename);
+	get_patch_filename(commit, subject, rev->nr, fmt_patch_suffix, &filename);
 
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
@@ -778,7 +779,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	const char *encoding = "UTF-8";
 	struct diff_options opts;
 	int need_8bit_cte = 0;
-	struct commit *commit = NULL;
 	struct pretty_print_context pp = {0};
 
 	if (rev->commit_format != CMIT_FMT_EMAIL)
@@ -786,31 +786,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	committer = git_committer_info(0);
 
-	if (!numbered_files) {
-		/*
-		 * We fake a commit for the cover letter so we get the filename
-		 * desired.
-		 */
-		commit = xcalloc(1, sizeof(*commit));
-		commit->buffer = xmalloc(400);
-		snprintf(commit->buffer, 400,
-			"tree 0000000000000000000000000000000000000000\n"
-			"parent %s\n"
-			"author %s\n"
-			"committer %s\n\n"
-			"cover letter\n",
-			sha1_to_hex(head->object.sha1), committer, committer);
-	}
-
-	if (!use_stdout && reopen_stdout(commit, rev, quiet))
+	if (!use_stdout &&
+	    reopen_stdout(NULL, numbered_files ? NULL : "cover-letter", rev, quiet))
 		return;
 
-	if (commit) {
-
-		free(commit->buffer);
-		free(commit);
-	}
-
 	log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
 				&need_8bit_cte);
 
@@ -1405,8 +1384,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, sha1_to_hex(commit->object.sha1));
 		}
 
-		if (!use_stdout && reopen_stdout(numbered_files ? NULL : commit,
-						 &rev, quiet))
+		if (!use_stdout &&
+		    reopen_stdout(numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("Failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
 		free(commit->buffer);
diff --git a/log-tree.c b/log-tree.c
index 376d973..c894930 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -299,19 +299,22 @@ static unsigned int digits_in_number(unsigned int number)
 	return result;
 }
 
-void get_patch_filename(struct commit *commit, int nr, const char *suffix,
-			struct strbuf *buf)
+void get_patch_filename(struct commit *commit, const char *subject, int nr,
+			const char *suffix, struct strbuf *buf)
 {
 	int suffix_len = strlen(suffix) + 1;
 	int start_len = buf->len;
 
-	strbuf_addf(buf, commit ? "%04d-" : "%d", nr);
-	if (commit) {
+	strbuf_addf(buf, commit || subject ? "%04d-" : "%d", nr);
+	if (commit || subject) {
 		int max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;
 		struct pretty_print_context ctx = {0};
-		ctx.date_mode = DATE_NORMAL;
 
-		format_commit_message(commit, "%f", buf, &ctx);
+		if (subject)
+			strbuf_addstr(buf, subject);
+		else if (commit)
+			format_commit_message(commit, "%f", buf, &ctx);
+
 		if (max_len < buf->len)
 			strbuf_setlen(buf, max_len);
 		strbuf_addstr(buf, suffix);
@@ -384,8 +387,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			 mime_boundary_leader, opt->mime_boundary);
 		extra_headers = subject_buffer;
 
-		get_patch_filename(opt->numbered_files ? NULL : commit, opt->nr,
-				    opt->patch_suffix, &filename);
+		get_patch_filename(opt->numbered_files ? NULL : commit, NULL,
+				   opt->nr, opt->patch_suffix, &filename);
 		snprintf(buffer, sizeof(buffer) - 1,
 			 "\n--%s%s\n"
 			 "Content-Type: text/x-patch;"
diff --git a/log-tree.h b/log-tree.h
index 5c4cf7c..f5ac238 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -21,7 +21,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 void load_ref_decorations(int flags);
 
 #define FORMAT_PATCH_NAME_MAX 64
-void get_patch_filename(struct commit *commit, int nr, const char *suffix,
-			struct strbuf *buf);
+void get_patch_filename(struct commit *commit, const char *subject, int nr,
+			const char *suffix, struct strbuf *buf);
 
 #endif
-- 
1.7.10.1.16.g53a707b

Re: [PATCH 08/13] ident: don't write fallback username into git_default_name

[Junio C Hamano]

This step seems to break t/t4014-format-patch.sh; the last output when the
test is run with "-i -v" indicates that Message-Id: and In-Reply-To: lines
have "<long-hexdigits.long-decimal.git.name@host" without matching closing
">" on it.

Re: [PATCH 06/13] format-patch: use default email for generating message ids

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We try to generate a sane message id for cover letters and
> threading by appending some changing bits to the front of
> the user's email address. The current code parses the email
> out of the results of git_committer_info, but we can do this
> much more easily by just calling ident_default_email
> ourselves.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Technically this is a regression if you really wanted:
>
>   GIT_COMMITTER_EMAIL=some.addr@example.com \
>   git format-patch --thread=deep
>
> to make your environment variable part of the message-ids. I don't think
> it matters, but I can adjust it if we care.

Is it because you no longer explicitly ask for "committer" and get generic
"who am I" bit from the ident infrastructure?

I wouldn't be surprised if some automated "commit email notification
reacting to push" bot in post-receive hook is using the environment
variable to affect the message ID.  I would doubt that would break the
message as long as the message ID generated from this codepath stays
valid, though, so I wouldn't worry about complaints along the lines of
"you started using names different from what you used to use".  As long as
we don't die due to "Hey bot, you do not seem to have a valid e-mail
address!", I don't think we need to worry about it.

Re: [PATCH 05/13] move git_default_* variables to ident.c

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/cache.h b/cache.h
> index 86224c8..f63b71f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1142,9 +1142,6 @@ struct config_include_data {
>  #define CONFIG_INCLUDE_INIT { 0 }
>  extern int git_config_include(const char *name, const char *value, void *data);
>  
> -#define MAX_GITNAME (1000)
> -extern char git_default_email[MAX_GITNAME];
> -extern char git_default_name[MAX_GITNAME];
>  #define IDENT_NAME_GIVEN 01
>  #define IDENT_MAIL_GIVEN 02
>  #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)

During this step, builtin/fmt-merge-msg.c ceases to compile, even though
in the end it is fixed...

Re: [PATCH 05/13] move git_default_* variables to ident.c

[Jeff King]

On Sun, May 20, 2012 at 09:07:42PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/cache.h b/cache.h
> > index 86224c8..f63b71f 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -1142,9 +1142,6 @@ struct config_include_data {
> >  #define CONFIG_INCLUDE_INIT { 0 }
> >  extern int git_config_include(const char *name, const char *value, void *data);
> >  
> > -#define MAX_GITNAME (1000)
> > -extern char git_default_email[MAX_GITNAME];
> > -extern char git_default_name[MAX_GITNAME];
> >  #define IDENT_NAME_GIVEN 01
> >  #define IDENT_MAIL_GIVEN 02
> >  #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
> 
> During this step, builtin/fmt-merge-msg.c ceases to compile, even though
> in the end it is fixed...

Hrm. I ran the series through "make" on each commit before submitting,
and it compiled fine. I just did it again to double-check, and it's
still fine.

What was the error you got? Are you sure you had 03/13 in place
beforehand, which removes the use of MAX_GITNAME from
builtin/fmt-merge-msg.c?

-Peff

Re: [PATCH 08/13] ident: don't write fallback username into git_default_name

[Jeff King]

On Sun, May 20, 2012 at 07:54:24PM -0700, Junio C Hamano wrote:

> This step seems to break t/t4014-format-patch.sh; the last output when the
> test is run with "-i -v" indicates that Message-Id: and In-Reply-To: lines
> have "<long-hexdigits.long-decimal.git.name@host" without matching closing
> ">" on it.

I found the issue. The test failure is actually introduced by patch
06/13 (format-patch: use default email for generating message ids). But
then it later gets fixed in 09/13 (drop length limitations on
gecos-derived names and emails).

The problems turns out to be a latent bug hidden in add_mailname_host.
It uses fgets to read from /etc/mailname, but never trims the trailing
newline. So git_default_email ends up set to something like
"peff@peff.net\n". Most of the time, this bug gets covered up by the
normalization that happens in fmt_ident (where we remove cruft
characters like newline). But in patch 6, we start using it directly
instead of re-parsing the output of fmt_ident, introducing a noticeable
bug.

I suspect there is also a bug in http-push.c, which directly uses
git_default_email as part of the xml in the dav request. I don't know if
it actually matters there or not.  And of course this triggers only on
systems with /etc/mailname, and only when you don't have user.email set.
So it may just be that the combination of that and the fact that hardly
anybody uses dumb http push has caused it to remain hidden.

The bug later ends up fixed in patch 9, because we replace fgets with
strbuf_getline, which trims the newline for us. Placing the patch below
anywhere before patch 6 fixes the issue (and then patch 9 would need to
later remove the code).

It does raise a subtle issue, though: should we be trimming whitespace
and other undesirable characters from git_default_email (or
git_default_name)? We don't currently, and it ends up OK because the
result typically is fed through fmt_ident, which cleans it up. But:

  1. We do look at the git_default_* variables for things like deciding
     whether the name is blank. So if your gecos field was " ", I think
     that would fool git into thinking it had something useful, and skip
     the IDENT_ERROR_ON_NO_NAME check, even though fmt_ident would
     produce an empty name.

  2. We don't always feed it through fmt_ident (the http-push.c callsite
     I mentioned above, and now patch 6 adds another one).

So I think my preference would be:

  - apply the patch below as 5.5/13

  - tweak patch 9 to remove the extra trimming

  - add a patch 14 to call strbuf_trim on the name and email buffers
    after reading them from system files.

-- >8 --
Subject: [PATCH] ident: trim trailing whitespace from /etc/mailname

Otherwise we will typically end up with an extra newline in
our git_default_email. Most of the time this doesn't matter,
as fmt_ident will skip it as cruft, but there is one code
path that accesses it directly (in http-push.c:lock_remote).

Let's trim any trailing space or line termination when we
read from /etc/mailname.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ident.c b/ident.c
index af92b2c..e797e36 100644
--- a/ident.c
+++ b/ident.c
@@ -57,6 +57,7 @@ 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;
+	char *p;
 
 	mailname = fopen("/etc/mailname", "r");
 	if (!mailname) {
@@ -74,6 +75,8 @@ static int add_mailname_host(char *buf, size_t len)
 	}
 	/* success! */
 	fclose(mailname);
+	for (p = buf + strlen(buf) - 1; p >= buf && isspace(*p); p--)
+		*p = '\0';
 	return 0;
 }
 
-- 
1.7.10.1.19.g711d603

Re: [PATCH 06/13] format-patch: use default email for generating message ids

[Jeff King]

On Sun, May 20, 2012 at 07:58:04PM -0700, Junio C Hamano wrote:

> > Technically this is a regression if you really wanted:
> >
> >   GIT_COMMITTER_EMAIL=some.addr@example.com \
> >   git format-patch --thread=deep
> >
> > to make your environment variable part of the message-ids. I don't think
> > it matters, but I can adjust it if we care.
> 
> Is it because you no longer explicitly ask for "committer" and get generic
> "who am I" bit from the ident infrastructure?

Right. Calling git_committer_info will also check
getenv("GIT_COMMITTER_EMAIL"), but we don't bother to do that here. We
could change the code to:

  const char *email = getenv("GIT_COMMITTER_EMAIL");
  if (!email)
          email = ident_default_email();

if it matters. I don't think it's worth building an alternate version of
git_committer_info that doesn't do the "name" half of the info.

> I wouldn't be surprised if some automated "commit email notification
> reacting to push" bot in post-receive hook is using the environment
> variable to affect the message ID.  I would doubt that would break the
> message as long as the message ID generated from this codepath stays
> valid, though, so I wouldn't worry about complaints along the lines of
> "you started using names different from what you used to use".  As long as
> we don't die due to "Hey bot, you do not seem to have a valid e-mail
> address!", I don't think we need to worry about it.

No, we'll never die as a result. But if you have a poorly configured
machine, you might get "user@host.(none)". Or I suppose you could leak
information about the username of the post-receive process. In both
cases, we do this already, so the only change is if you are trying to
override that with GIT_COMMITTER_EMAIL.

So it's a little far-fetched, which is why I didn't bother. But the code
above is quite simple, so maybe it is better to be conservative.

-Peff

Re: [PATCH 05/13] move git_default_* variables to ident.c

[Jeff King]

On Mon, May 21, 2012 at 01:41:18AM -0400, Jeff King wrote:

> > > -#define MAX_GITNAME (1000)
> > > -extern char git_default_email[MAX_GITNAME];
> > > -extern char git_default_name[MAX_GITNAME];
> > >  #define IDENT_NAME_GIVEN 01
> > >  #define IDENT_MAIL_GIVEN 02
> > >  #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
> > 
> > During this step, builtin/fmt-merge-msg.c ceases to compile, even though
> > in the end it is fixed...
> 
> Hrm. I ran the series through "make" on each commit before submitting,
> and it compiled fine. I just did it again to double-check, and it's
> still fine.
> 
> What was the error you got? Are you sure you had 03/13 in place
> beforehand, which removes the use of MAX_GITNAME from
> builtin/fmt-merge-msg.c?

Ah, yeah. I just looked at what you wrote in the "What's Cooking"
message. For some reason you applied patch 03/13 out of order (in slot
5). I'm not sure how you order the messages in a series when you apply
it, but AFAICT, the numbers in the subjects and the date stamps are
in the right order. Operator error? :)

-Peff

Re: [PATCH 08/13] ident: don't write fallback username into git_default_name

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It does raise a subtle issue, though: should we be trimming whitespace
> and other undesirable characters from git_default_email (or
> git_default_name)? We don't currently, and it ends up OK because the
> result typically is fed through fmt_ident, which cleans it up. But:
>
>   1. We do look at the git_default_* variables for things like deciding
>      whether the name is blank. So if your gecos field was " ", I think
>      that would fool git into thinking it had something useful, and skip
>      the IDENT_ERROR_ON_NO_NAME check, even though fmt_ident would
>      produce an empty name.
>
>   2. We don't always feed it through fmt_ident (the http-push.c callsite
>      I mentioned above, and now patch 6 adds another one).
>
> So I think my preference would be:
>
>   - apply the patch below as 5.5/13
>
>   - tweak patch 9 to remove the extra trimming
>
>   - add a patch 14 to call strbuf_trim on the name and email buffers
>     after reading them from system files.

Sounds sensible; thanks.

[PATCHv2 0/15] ident cleanups git_default_name

[Jeff King]

On Mon, May 21, 2012 at 02:11:58AM -0700, Junio C Hamano wrote:

> > So I think my preference would be:
> >
> >   - apply the patch below as 5.5/13
> >
> >   - tweak patch 9 to remove the extra trimming
> >
> >   - add a patch 14 to call strbuf_trim on the name and email buffers
> >     after reading them from system files.
> 
> Sounds sensible; thanks.

Here's the whole series again. Most of it is unchanged, but given the
ordering problem earlier, it seems reasonable to just post it all again.

  [01/15]: ident: split setup_ident into separate functions
  [02/15]: http-push: do not access git_default_email directly
  [03/15]: fmt-merge-msg: don't use static buffer in record_person
  [04/15]: move identity config parsing to ident.c
  [05/15]: move git_default_* variables to ident.c
  [06/15]: ident: trim trailing newline from /etc/mailname

  This one is new and fixes the t4014 failure. The earlier version I
  posted trimmed all trailing whitespace, but there's not much point in
  being thorough, since this code gets removed later, anyway.

  [07/15]: format-patch: use default email for generating message ids
  [08/15]: fmt_ident: drop IDENT_WARN_ON_NO_NAME code
  [09/15]: ident: don't write fallback username into git_default_name
  [10/15]: drop length limitations on gecos-derived names and emails

  This one drops the new code from patch 6, as it is pointless with
  strbufs. I considered reordering the patches to avoid patch 6
  altogether, but there is a circular dependency with getting rid of the
  WARN case in patch 8.

  [11/15]: ident: report passwd errors with a more friendly message
  [12/15]: ident: use full dns names to generate email addresses
  [13/15]: ident: use a dynamic strbuf in fmt_ident
  [14/15]: ident: trim whitespace from default name/email

  This one is new, and actually does a good job of trimming unnecessary
  whitespace (from both the front and back, and from both name and
  email).

  [15/15]: format-patch: refactor get_patch_filename

-Peff

[PATCHv2 01/15] ident: split setup_ident into separate functions

[Jeff King]

This function sets up the default name, email, and date, and
is not publicly available. Let's split it into three public
functions so that callers can get just the parts they need.

While we're at it, let's change the interface to simple
accessors. The original function was called only by fmt_ident,
and contained logic for "if we already have some other
value, don't load the default" which properly belongs in
fmt_ident.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |  3 +++
 ident.c | 35 +++++++++++++++++++----------------
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/cache.h b/cache.h
index e14ffcd..0c095d4 100644
--- a/cache.h
+++ b/cache.h
@@ -894,6 +894,9 @@ 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 *ident_default_name(void);
+extern const char *ident_default_email(void);
+extern const char *ident_default_date(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 
diff --git a/ident.c b/ident.c
index 87c697c..0f7dcae 100644
--- a/ident.c
+++ b/ident.c
@@ -117,21 +117,20 @@ static void copy_email(const struct passwd *pw)
 			sizeof(git_default_email) - len);
 }
 
-static void setup_ident(const char **name, const char **emailp)
+const char *ident_default_name(void)
 {
-	struct passwd *pw = NULL;
-
-	/* Get the name ("gecos") */
-	if (!*name && !git_default_name[0]) {
-		pw = getpwuid(getuid());
+	if (!git_default_name[0]) {
+		struct passwd *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;
+	return git_default_name;
+}
 
-	if (!*emailp && !git_default_email[0]) {
+const char *ident_default_email(void)
+{
+	if (!git_default_email[0]) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
@@ -139,19 +138,20 @@ static void setup_ident(const char **name, const char **emailp)
 				sizeof(git_default_email));
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else {
-			if (!pw)
-				pw = getpwuid(getuid());
+			struct passwd *pw = getpwuid(getuid());
 			if (!pw)
 				die("You don't exist. Go away!");
 			copy_email(pw);
 		}
 	}
-	if (!*emailp)
-		*emailp = git_default_email;
+	return git_default_email;
+}
 
-	/* And set the default date */
+const char *ident_default_date(void)
+{
 	if (!git_default_date[0])
 		datestamp(git_default_date, sizeof(git_default_date));
+	return git_default_date;
 }
 
 static int add_raw(char *buf, size_t size, int offset, const char *str)
@@ -311,7 +311,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);
 
-	setup_ident(&name, &email);
+	if (!name)
+		name = ident_default_name();
+	if (!email)
+		email = ident_default_email();
 
 	if (!*name) {
 		struct passwd *pw;
@@ -331,7 +334,7 @@ const char *fmt_ident(const char *name, const char *email,
 		name = git_default_name;
 	}
 
-	strcpy(date, git_default_date);
+	strcpy(date, ident_default_date());
 	if (!name_addr_only && date_str && date_str[0]) {
 		if (parse_date(date_str, date, sizeof(date)) < 0)
 			die("invalid date format: %s", date_str);
-- 
1.7.10.1.19.g711d603

[PATCHv2 02/15] http-push: do not access git_default_email directly

[Jeff King]

By calling the ident_default_email accessor, we can be sure
that the default value is actually filled-in.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index 1df7ab5..a832ca7 100644
--- a/http-push.c
+++ b/http-push.c
@@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		ep = strchr(ep + 1, '/');
 	}
 
-	escaped = xml_entities(git_default_email);
+	escaped = xml_entities(ident_default_email());
 	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
 	free(escaped);
 
-- 
1.7.10.1.19.g711d603

[PATCHv2 03/15] fmt-merge-msg: don't use static buffer in record_person

[Jeff King]

The record_person function just parses out the "name" field
of the person line in a commit and adds it to a string_list.
The only reason we need an extra buffer is that the
string_list functions require a NUL-terminated string.

Instead of the static buffer, we can just allocate a
temporary NUL-terminated copy. In addition to removing a
useless limit, this removes the only user of MAX_GITNAME
outside of ident.c.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fmt-merge-msg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index a517f17..4ee6a88 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -230,7 +230,7 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 static void record_person(int which, struct string_list *people,
 			  struct commit *commit)
 {
-	char name_buf[MAX_GITNAME], *name, *name_end;
+	char *name_buf, *name, *name_end;
 	struct string_list_item *elem;
 	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
 
@@ -243,10 +243,9 @@ static void record_person(int which, struct string_list *people,
 		name_end--;
 	while (isspace(*name_end) && name <= name_end)
 		name_end--;
-	if (name_end < name || name + MAX_GITNAME <= name_end)
+	if (name_end < name)
 		return;
-	memcpy(name_buf, name, name_end - name + 1);
-	name_buf[name_end - name + 1] = '\0';
+	name_buf = xmemdupz(name, name_end - name + 1);
 
 	elem = string_list_lookup(people, name_buf);
 	if (!elem) {
@@ -254,6 +253,7 @@ static void record_person(int which, struct string_list *people,
 		elem->util = (void *)0;
 	}
 	elem->util = (void*)(util_as_integral(elem) + 1);
+	free(name_buf);
 }
 
 static int cmp_string_list_util_as_integral(const void *a_, const void *b_)
-- 
1.7.10.1.19.g711d603

[PATCHv2 04/15] move identity config parsing to ident.c

[Jeff King]

There's no reason for this to be in config, except that once
upon a time all of the config parsing was there. It makes
more sense to keep the ident code together.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h  |  1 +
 config.c | 24 +-----------------------
 ident.c  | 21 +++++++++++++++++++++
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/cache.h b/cache.h
index 0c095d4..86224c8 100644
--- a/cache.h
+++ b/cache.h
@@ -899,6 +899,7 @@ extern const char *ident_default_email(void);
 extern const char *ident_default_date(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
+extern int git_ident_config(const char *, const char *, void *);
 
 struct ident_split {
 	const char *name_begin;
diff --git a/config.c b/config.c
index eeee986..71ef171 100644
--- a/config.c
+++ b/config.c
@@ -762,28 +762,6 @@ static int git_default_core_config(const char *var, const char *value)
 	return 0;
 }
 
-static int git_default_user_config(const char *var, const char *value)
-{
-	if (!strcmp(var, "user.name")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strlcpy(git_default_name, value, sizeof(git_default_name));
-		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
-		return 0;
-	}
-
-	if (!strcmp(var, "user.email")) {
-		if (!value)
-			return config_error_nonbool(var);
-		strlcpy(git_default_email, value, sizeof(git_default_email));
-		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		return 0;
-	}
-
-	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
-}
-
 static int git_default_i18n_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "i18n.commitencoding"))
@@ -870,7 +848,7 @@ int git_default_config(const char *var, const char *value, void *dummy)
 		return git_default_core_config(var, value);
 
 	if (!prefixcmp(var, "user."))
-		return git_default_user_config(var, value);
+		return git_ident_config(var, value, dummy);
 
 	if (!prefixcmp(var, "i18n."))
 		return git_default_i18n_config(var, value);
diff --git a/ident.c b/ident.c
index 0f7dcae..bb1158f 100644
--- a/ident.c
+++ b/ident.c
@@ -388,3 +388,24 @@ int user_ident_sufficiently_given(void)
 	return (user_ident_explicitly_given == IDENT_ALL_GIVEN);
 #endif
 }
+
+int git_ident_config(const char *var, const char *value, void *data)
+{
+	if (!strcmp(var, "user.name")) {
+		if (!value)
+			return config_error_nonbool(var);
+		strlcpy(git_default_name, value, sizeof(git_default_name));
+		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		return 0;
+	}
+
+	if (!strcmp(var, "user.email")) {
+		if (!value)
+			return config_error_nonbool(var);
+		strlcpy(git_default_email, value, sizeof(git_default_email));
+		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		return 0;
+	}
+
+	return 0;
+}
-- 
1.7.10.1.19.g711d603

[PATCHv2 05/15] move git_default_* variables to ident.c

[Jeff King]

There's no reason anybody outside of ident.c should access
these directly (they should use the new accessors which make
sure the variables are initialized), so we can make them
file-scope statics.

While we're at it, move user_ident_explicitly_given into
ident.c; while still globally visible, it makes more sense
to reside with the ident code.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h       | 3 ---
 environment.c | 3 ---
 ident.c       | 4 ++++
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 86224c8..f63b71f 100644
--- a/cache.h
+++ b/cache.h
@@ -1142,9 +1142,6 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-#define MAX_GITNAME (1000)
-extern char git_default_email[MAX_GITNAME];
-extern char git_default_name[MAX_GITNAME];
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
diff --git a/environment.c b/environment.c
index d7e6c65..669e498 100644
--- a/environment.c
+++ b/environment.c
@@ -11,9 +11,6 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 
-char git_default_email[MAX_GITNAME];
-char git_default_name[MAX_GITNAME];
-int user_ident_explicitly_given;
 int trust_executable_bit = 1;
 int trust_ctime = 1;
 int has_symlinks = 1;
diff --git a/ident.c b/ident.c
index bb1158f..af92b2c 100644
--- a/ident.c
+++ b/ident.c
@@ -7,7 +7,11 @@
  */
 #include "cache.h"
 
+#define MAX_GITNAME (1000)
+static char git_default_name[MAX_GITNAME];
+static char git_default_email[MAX_GITNAME];
 static char git_default_date[50];
+int user_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
-- 
1.7.10.1.19.g711d603

[PATCHv2 06/15] ident: trim trailing newline from /etc/mailname

[Jeff King]

We use fgets to read the /etc/mailname file, which means we
will typically end up with an extra newline in our
git_default_email. Most of the time this doesn't matter, as
fmt_ident will skip it as cruft, but there is one code path
that accesses it directly (in http-push.c:lock_remote).

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ident.c b/ident.c
index af92b2c..acb3a08 100644
--- a/ident.c
+++ b/ident.c
@@ -74,6 +74,10 @@ static int add_mailname_host(char *buf, size_t len)
 	}
 	/* success! */
 	fclose(mailname);
+
+	len = strlen(buf);
+	if (len && buf[len-1] == '\n')
+		buf[len-1] = '\0';
 	return 0;
 }
 
-- 
1.7.10.1.19.g711d603

[PATCHv2 07/15] format-patch: use default email for generating message ids

[Jeff King]

We try to generate a sane message id for cover letters and
threading by appending some changing bits to the front of
the user's email address. The current code parses the email
out of the results of git_committer_info, but we can do this
much more easily by just calling ident_default_email
ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 690caa7..656bddf 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -737,15 +737,9 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha
 
 static void gen_message_id(struct rev_info *info, char *base)
 {
-	const char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME);
-	const char *email_start = strrchr(committer, '<');
-	const char *email_end = strrchr(committer, '>');
 	struct strbuf buf = STRBUF_INIT;
-	if (!email_start || !email_end || email_start > email_end - 1)
-		die(_("Could not extract email from committer identity."));
-	strbuf_addf(&buf, "%s.%lu.git.%.*s", base,
-		    (unsigned long) time(NULL),
-		    (int)(email_end - email_start - 1), email_start + 1);
+	strbuf_addf(&buf, "%s.%lu.git.%s", base,
+		    (unsigned long) time(NULL), ident_default_email());
 	info->message_id = strbuf_detach(&buf, NULL);
 }
 
-- 
1.7.10.1.19.g711d603

[PATCHv2 08/15] fmt_ident: drop IDENT_WARN_ON_NO_NAME code

[Jeff King]

There are no more callers who want this, so we can drop it.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |  5 ++---
 ident.c | 11 ++++-------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index f63b71f..65cbab5 100644
--- a/cache.h
+++ b/cache.h
@@ -887,9 +887,8 @@ unsigned long approxidate_careful(const char *, int *);
 unsigned long approxidate_relative(const char *date, const struct timeval *now);
 enum date_mode parse_date_format(const char *format);
 
-#define IDENT_WARN_ON_NO_NAME  1
-#define IDENT_ERROR_ON_NO_NAME 2
-#define IDENT_NO_DATE	       4
+#define IDENT_ERROR_ON_NO_NAME 1
+#define IDENT_NO_DATE	       2
 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);
diff --git a/ident.c b/ident.c
index acb3a08..3b92c44 100644
--- a/ident.c
+++ b/ident.c
@@ -316,7 +316,6 @@ const char *fmt_ident(const char *name, const char *email,
 	char date[50];
 	int i;
 	int error_on_no_name = (flag & IDENT_ERROR_ON_NO_NAME);
-	int warn_on_no_name = (flag & IDENT_WARN_ON_NO_NAME);
 	int name_addr_only = (flag & IDENT_NO_DATE);
 
 	if (!name)
@@ -327,13 +326,11 @@ const char *fmt_ident(const char *name, const char *email,
 	if (!*name) {
 		struct passwd *pw;
 
-		if ((warn_on_no_name || error_on_no_name) &&
-		    name == git_default_name && env_hint) {
-			fputs(env_hint, stderr);
-			env_hint = NULL; /* warn only once */
-		}
-		if (error_on_no_name)
+		if (error_on_no_name) {
+			if (name == git_default_name)
+				fputs(env_hint, stderr);
 			die("empty ident %s <%s> not allowed", name, email);
+		}
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-- 
1.7.10.1.19.g711d603

[PATCHv2 09/15] ident: don't write fallback username into git_default_name

[Jeff King]

The fmt_ident function gets a flag that tells us whether to
die if the name field is blank. If it is blank and we don't
die, then we fall back to the username from the passwd file.

The current code writes the value into git_default_name.
However, that's not necessarily correct, as the empty value
might have come from git_default_name, or it might have been
passed in.  This leads to two potential problems:

  1. If we are overriding an empty name in the passed-in
     value, then we may be overwriting a perfectly good name
     (from gitconfig or gecos) in the git_default_name
     buffer. Later calls to fmt_ident will end up using the
     fallback name, even though a better name was available.

  2. If we override an empty gecos name, we end up with the
     fallback name in git_default_name. A later call that
     uses IDENT_ERROR_ON_NO_NAME will see the fallback name
     and think that it is a good name, instead of producing
     an error. In other words, a blank gecos name would
     cause an error with this code:

       git_committer_info(IDENT_ERROR_ON_NO_NAME);

     but not this:

       git_committer_info(0);
       git_committer_info(IDENT_ERROR_ON_NO_NAME);

     because in the latter case, the first call has polluted
     the name buffer.

Instead, let's make the fallback a per-invocation variable.
We can just use the pw->pw_name string directly, since it
only needs to persist through the rest of the function (and
we don't do any other getpwent calls).

Note that while this solves (1) for future invocations of
fmt_indent, the current invocation might use the fallback
when it could in theory load a better value from
git_default_name. However, by not passing
IDENT_ERROR_ON_NO_NAME, the caller is indicating that it
does not care too much about the name, anyway, so we don't
bother; this is primarily about protecting future callers
who do care.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ident.c b/ident.c
index 3b92c44..87e3cbe 100644
--- a/ident.c
+++ b/ident.c
@@ -334,9 +334,7 @@ const char *fmt_ident(const char *name, const char *email,
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		strlcpy(git_default_name, pw->pw_name,
-			sizeof(git_default_name));
-		name = git_default_name;
+		name = pw->pw_name;
 	}
 
 	strcpy(date, ident_default_date());
-- 
1.7.10.1.19.g711d603

[PATCHv2 10/15] drop length limitations on gecos-derived names and emails

[Jeff King]

When we pull the user's name from the GECOS field of the
passwd file (or generate an email address based on their
username and hostname), we put the result into a
static buffer. While it's extremely unlikely that anybody
ever hit these limits (after all, in such a case their
parents must have hated them), we still had to deal with the
error cases in our code.

Converting these static buffers to strbufs lets us simplify
the code and drop some error messages from the documentation
that have confused some users.

The conversion is mostly mechanical: replace string copies
with strbuf equivalents, and access the strbuf.buf directly.
There are a few exceptions:

  - copy_gecos and copy_email are the big winners in code
    reduction (since they no longer have to manage the
    string length manually)

  - git_ident_config wants to replace old versions of
    the default name (e.g., if we read the config multiple
    times), so it must reset+add to the strbuf instead of
    just adding

Note that there is still one length limitation: the
gethostname interface requires us to provide a static
buffer, so we arbitrarily choose 1024 bytes for the
hostname.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-commit-tree.txt |   4 --
 Documentation/git-var.txt         |   4 --
 ident.c                           | 104 ++++++++++++++------------------------
 3 files changed, 39 insertions(+), 73 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index cfb9906..eb12b2d 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -92,10 +92,6 @@ Diagnostics
 -----------
 You don't exist. Go away!::
     The passwd(5) gecos field couldn't be read
-Your parents must have hated you!::
-    The passwd(5) gecos field is longer than a giant static buffer.
-Your sysadmin must hate you!::
-    The passwd(5) name field is longer than a giant static buffer.
 
 Discussion
 ----------
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 988a323..3f703e3 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -63,10 +63,6 @@ Diagnostics
 -----------
 You don't exist. Go away!::
     The passwd(5) gecos field couldn't be read
-Your parents must have hated you!::
-    The passwd(5) gecos field is longer than a giant static buffer.
-Your sysadmin must hate you!::
-    The passwd(5) name field is longer than a giant static buffer.
 
 SEE ALSO
 --------
diff --git a/ident.c b/ident.c
index 87e3cbe..73a06a1 100644
--- a/ident.c
+++ b/ident.c
@@ -7,9 +7,8 @@
  */
 #include "cache.h"
 
-#define MAX_GITNAME (1000)
-static char git_default_name[MAX_GITNAME];
-static char git_default_email[MAX_GITNAME];
+static struct strbuf git_default_name = STRBUF_INIT;
+static struct strbuf git_default_email = STRBUF_INIT;
 static char git_default_date[50];
 int user_ident_explicitly_given;
 
@@ -19,42 +18,27 @@ int user_ident_explicitly_given;
 #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
 #endif
 
-static void copy_gecos(const struct passwd *w, char *name, size_t sz)
+static void copy_gecos(const struct passwd *w, struct strbuf *name)
 {
-	char *src, *dst;
-	size_t len, nlen;
-
-	nlen = strlen(w->pw_name);
+	char *src;
 
 	/* Traditionally GECOS field had office phone numbers etc, separated
 	 * with commas.  Also & stands for capitalized form of the login name.
 	 */
 
-	for (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {
+	for (src = get_gecos(w); *src && *src != ','; src++) {
 		int ch = *src;
-		if (ch != '&') {
-			*dst++ = ch;
-			if (ch == 0 || ch == ',')
-				break;
-			len++;
-			continue;
-		}
-		if (len + nlen < sz) {
+		if (ch != '&')
+			strbuf_addch(name, ch);
+		else {
 			/* Sorry, Mr. McDonald... */
-			*dst++ = toupper(*w->pw_name);
-			memcpy(dst, w->pw_name + 1, nlen - 1);
-			dst += nlen - 1;
-			len += nlen;
+			strbuf_addch(name, toupper(*w->pw_name));
+			strbuf_addstr(name, w->pw_name + 1);
 		}
 	}
-	if (len < sz)
-		name[len] = 0;
-	else
-		die("Your parents must have hated you!");
-
 }
 
-static int add_mailname_host(char *buf, size_t len)
+static int add_mailname_host(struct strbuf *buf)
 {
 	FILE *mailname;
 
@@ -65,7 +49,7 @@ static int add_mailname_host(char *buf, size_t len)
 				strerror(errno));
 		return -1;
 	}
-	if (!fgets(buf, len, mailname)) {
+	if (strbuf_getline(buf, mailname, '\n') == EOF) {
 		if (ferror(mailname))
 			warning("cannot read /etc/mailname: %s",
 				strerror(errno));
@@ -74,85 +58,73 @@ static int add_mailname_host(char *buf, size_t len)
 	}
 	/* success! */
 	fclose(mailname);
-
-	len = strlen(buf);
-	if (len && buf[len-1] == '\n')
-		buf[len-1] = '\0';
 	return 0;
 }
 
-static void add_domainname(char *buf, size_t len)
+static void add_domainname(struct strbuf *out)
 {
+	char buf[1024];
 	struct hostent *he;
-	size_t namelen;
 	const char *domainname;
 
-	if (gethostname(buf, len)) {
+	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 		return;
 	}
-	namelen = strlen(buf);
-	if (memchr(buf, '.', namelen))
+	strbuf_addstr(out, buf);
+	if (strchr(buf, '.'))
 		return;
 
 	he = gethostbyname(buf);
-	buf[namelen++] = '.';
-	buf += namelen;
-	len -= namelen;
+	strbuf_addch(out, '.');
 	if (he && (domainname = strchr(he->h_name, '.')))
-		strlcpy(buf, domainname + 1, len);
+		strbuf_addstr(out, domainname + 1);
 	else
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 }
 
-static void copy_email(const struct passwd *pw)
+static void copy_email(const struct passwd *pw, struct strbuf *email)
 {
 	/*
 	 * Make up a fake email address
 	 * (name + '@' + hostname [+ '.' + domainname])
 	 */
-	size_t len = strlen(pw->pw_name);
-	if (len > sizeof(git_default_email)/2)
-		die("Your sysadmin must hate you!");
-	memcpy(git_default_email, pw->pw_name, len);
-	git_default_email[len++] = '@';
-
-	if (!add_mailname_host(git_default_email + len,
-				sizeof(git_default_email) - len))
+	strbuf_addstr(email, pw->pw_name);
+	strbuf_addch(email, '@');
+
+	if (!add_mailname_host(email))
 		return;	/* read from "/etc/mailname" (Debian) */
-	add_domainname(git_default_email + len,
-			sizeof(git_default_email) - len);
+	add_domainname(email);
 }
 
 const char *ident_default_name(void)
 {
-	if (!git_default_name[0]) {
+	if (!git_default_name.len) {
 		struct passwd *pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		copy_gecos(pw, git_default_name, sizeof(git_default_name));
+		copy_gecos(pw, &git_default_name);
 	}
-	return git_default_name;
+	return git_default_name.buf;
 }
 
 const char *ident_default_email(void)
 {
-	if (!git_default_email[0]) {
+	if (!git_default_email.len) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
-			strlcpy(git_default_email, email,
-				sizeof(git_default_email));
+			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else {
 			struct passwd *pw = getpwuid(getuid());
 			if (!pw)
 				die("You don't exist. Go away!");
-			copy_email(pw);
+			copy_email(pw, &git_default_email);
 		}
 	}
-	return git_default_email;
+	return git_default_email.buf;
 }
 
 const char *ident_default_date(void)
@@ -327,7 +299,7 @@ const char *fmt_ident(const char *name, const char *email,
 		struct passwd *pw;
 
 		if (error_on_no_name) {
-			if (name == git_default_name)
+			if (name == git_default_name.buf)
 				fputs(env_hint, stderr);
 			die("empty ident %s <%s> not allowed", name, email);
 		}
@@ -397,7 +369,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_name, value, sizeof(git_default_name));
+		strbuf_reset(&git_default_name);
+		strbuf_addstr(&git_default_name, value);
 		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
@@ -405,7 +378,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 	if (!strcmp(var, "user.email")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_email, value, sizeof(git_default_email));
+		strbuf_reset(&git_default_email);
+		strbuf_addstr(&git_default_email, value);
 		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
-- 
1.7.10.1.19.g711d603

[PATCHv2 11/15] ident: report passwd errors with a more friendly message

[Jeff King]

When getpwuid fails, we give a cute but cryptic message.
While it makes sense if you know that getpwuid or identity
functions are being called, this code is triggered behind
the scenes by quite a few git commands these days (e.g.,
receive-pack on a remote server might use it for a reflog;
the current message is hard to distinguish from an
authentication error).  Let's switch to something that gives
a little more context.

While we're at it, we can factor out all of the
cut-and-pastes of the "you don't exist" message into a
wrapper function. Rather than provide xgetpwuid, let's make
it even more specific to just getting the passwd entry for
the current uid. That's the only way we use getpwuid anyway,
and it lets us make an even more specific error message.

The current message also fails to mention errno. While the
usual cause for getpwuid failing is that the user does not
exist, mentioning errno makes it easier to diagnose these
problems.  Note that POSIX specifies that errno remain
untouched if the passwd entry does not exist (but will be
set on actual errors), whereas some systems will return
ENOENT or similar for a missing entry. We handle both cases
in our wrapper.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-commit-tree.txt |  5 -----
 Documentation/git-var.txt         |  5 -----
 git-compat-util.h                 |  3 +++
 ident.c                           | 20 +++++---------------
 wrapper.c                         | 12 ++++++++++++
 5 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index eb12b2d..eb8ee99 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -88,11 +88,6 @@ for one to be entered and terminated with ^D.
 
 include::date-formats.txt[]
 
-Diagnostics
------------
-You don't exist. Go away!::
-    The passwd(5) gecos field couldn't be read
-
 Discussion
 ----------
 
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 3f703e3..67edf58 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -59,11 +59,6 @@ ifdef::git-default-pager[]
     The build you are using chose '{git-default-pager}' as the default.
 endif::git-default-pager[]
 
-Diagnostics
------------
-You don't exist. Go away!::
-    The passwd(5) gecos field couldn't be read
-
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]
diff --git a/git-compat-util.h b/git-compat-util.h
index ed11ad8..5bd9ad7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -595,4 +595,7 @@ int rmdir_or_warn(const char *path);
  */
 int remove_or_warn(unsigned int mode, const char *path);
 
+/* Get the passwd entry for the UID of the current process. */
+struct passwd *xgetpwuid_self(void);
+
 #endif
diff --git a/ident.c b/ident.c
index 73a06a1..5aec073 100644
--- a/ident.c
+++ b/ident.c
@@ -100,12 +100,8 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
 
 const char *ident_default_name(void)
 {
-	if (!git_default_name.len) {
-		struct passwd *pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
-		copy_gecos(pw, &git_default_name);
-	}
+	if (!git_default_name.len)
+		copy_gecos(xgetpwuid_self(), &git_default_name);
 	return git_default_name.buf;
 }
 
@@ -117,12 +113,8 @@ const char *ident_default_email(void)
 		if (email && email[0]) {
 			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		} else {
-			struct passwd *pw = getpwuid(getuid());
-			if (!pw)
-				die("You don't exist. Go away!");
-			copy_email(pw, &git_default_email);
-		}
+		} else
+			copy_email(xgetpwuid_self(), &git_default_email);
 	}
 	return git_default_email.buf;
 }
@@ -303,9 +295,7 @@ const char *fmt_ident(const char *name, const char *email,
 				fputs(env_hint, stderr);
 			die("empty ident %s <%s> not allowed", name, email);
 		}
-		pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
+		pw = xgetpwuid_self();
 		name = pw->pw_name;
 	}
 
diff --git a/wrapper.c b/wrapper.c
index 6ccd059..b5e33e4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -402,3 +402,15 @@ int remove_or_warn(unsigned int mode, const char *file)
 {
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
+
+struct passwd *xgetpwuid_self(void)
+{
+	struct passwd *pw;
+
+	errno = 0;
+	pw = getpwuid(getuid());
+	if (!pw)
+		die(_("unable to look up current user in the passwd file: %s"),
+		    errno ? strerror(errno) : _("no such user"));
+	return pw;
+}
-- 
1.7.10.1.19.g711d603

[PATCHv2 12/15] ident: use full dns names to generate email addresses

[Jeff King]

When we construct an email address from the username and
hostname, we generate the host part of the email with this
procedure:

  1. add the result of gethostname

  2. if it has a dot, ok, it's fully qualified

  3. if not, then look up the unqualified hostname via
     gethostbyname; take the domain name of the result and
     append it to the hostname

Step 3 can actually produce a bogus result, as the name
returned by gethostbyname may not be related to the hostname
we fed it (e.g., consider a machine "foo" with names
"foo.one.example.com" and "bar.two.example.com"; we may have
the latter returned and generate the bogus name
"foo.two.example.com").

This patch simply uses the full hostname returned by
gethostbyname. In the common case that the first part is the
same as the unqualified hostname, the behavior is identical.
And in the case that it is not the same, we are much more
likely to be generating a valid name.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/ident.c b/ident.c
index 5aec073..b111e34 100644
--- a/ident.c
+++ b/ident.c
@@ -65,23 +65,18 @@ static void add_domainname(struct strbuf *out)
 {
 	char buf[1024];
 	struct hostent *he;
-	const char *domainname;
 
 	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
 		strbuf_addstr(out, "(none)");
 		return;
 	}
-	strbuf_addstr(out, buf);
 	if (strchr(buf, '.'))
-		return;
-
-	he = gethostbyname(buf);
-	strbuf_addch(out, '.');
-	if (he && (domainname = strchr(he->h_name, '.')))
-		strbuf_addstr(out, domainname + 1);
+		strbuf_addstr(out, buf);
+	else if ((he = gethostbyname(buf)) && strchr(he->h_name, '.'))
+		strbuf_addstr(out, he->h_name);
 	else
-		strbuf_addstr(out, "(none)");
+		strbuf_addf(out, "%s.(none)", buf);
 }
 
 static void copy_email(const struct passwd *pw, struct strbuf *email)
-- 
1.7.10.1.19.g711d603

[PATCHv2 13/15] ident: use a dynamic strbuf in fmt_ident

[Jeff King]

Now that we accept arbitrary-sized names and email
addresses, the only remaining limit is in the actual
formatting of the names into a buffer. The current limit is
1000 characters, which is not likely to be reached, but
using a strbuf is one less error condition we have to worry
about.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/ident.c b/ident.c
index b111e34..cefb829 100644
--- a/ident.c
+++ b/ident.c
@@ -121,15 +121,6 @@ const char *ident_default_date(void)
 	return git_default_date;
 }
 
-static int add_raw(char *buf, size_t size, int offset, const char *str)
-{
-	size_t len = strlen(str);
-	if (offset + len > size)
-		return size;
-	memcpy(buf + offset, str, len);
-	return offset + len;
-}
-
 static int crud(unsigned char c)
 {
 	return  c <= 32  ||
@@ -148,7 +139,7 @@ static int crud(unsigned char c)
  * Copy over a string to the destination, but avoid special
  * characters ('\n', '<' and '>') and remove crud at the end
  */
-static int copy(char *buf, size_t size, int offset, const char *src)
+static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src)
 {
 	size_t i, len;
 	unsigned char c;
@@ -172,19 +163,19 @@ static int copy(char *buf, size_t size, int offset, const char *src)
 	/*
 	 * Copy the rest to the buffer, but avoid the special
 	 * characters '\n' '<' and '>' that act as delimiters on
-	 * an identification line
+	 * an identification line. We can only remove crud, never add it,
+	 * so 'len' is our maximum.
 	 */
+	strbuf_grow(sb, len);
 	for (i = 0; i < len; i++) {
 		c = *src++;
 		switch (c) {
 		case '\n': case '<': case '>':
 			continue;
 		}
-		if (offset >= size)
-			return size;
-		buf[offset++] = c;
+		sb->buf[sb->len++] = c;
 	}
-	return offset;
+	sb->buf[sb->len] = '\0';
 }
 
 /*
@@ -271,9 +262,8 @@ static const char *env_hint =
 const char *fmt_ident(const char *name, const char *email,
 		      const char *date_str, int flag)
 {
-	static char buffer[1000];
+	static struct strbuf ident = STRBUF_INIT;
 	char date[50];
-	int i;
 	int error_on_no_name = (flag & IDENT_ERROR_ON_NO_NAME);
 	int name_addr_only = (flag & IDENT_NO_DATE);
 
@@ -300,19 +290,16 @@ const char *fmt_ident(const char *name, const char *email,
 			die("invalid date format: %s", date_str);
 	}
 
-	i = copy(buffer, sizeof(buffer), 0, name);
-	i = add_raw(buffer, sizeof(buffer), i, " <");
-	i = copy(buffer, sizeof(buffer), i, email);
+	strbuf_reset(&ident);
+	strbuf_addstr_without_crud(&ident, name);
+	strbuf_addstr(&ident, " <");
+	strbuf_addstr_without_crud(&ident, email);
+	strbuf_addch(&ident, '>');
 	if (!name_addr_only) {
-		i = add_raw(buffer, sizeof(buffer), i,  "> ");
-		i = copy(buffer, sizeof(buffer), i, date);
-	} else {
-		i = add_raw(buffer, sizeof(buffer), i, ">");
+		strbuf_addch(&ident, ' ');
+		strbuf_addstr_without_crud(&ident, date);
 	}
-	if (i >= sizeof(buffer))
-		die("Impossibly long personal identifier");
-	buffer[i] = 0;
-	return buffer;
+	return ident.buf;
 }
 
 const char *fmt_name(const char *name, const char *email)
-- 
1.7.10.1.19.g711d603

[PATCHv2 14/15] ident: trim whitespace from default name/email

[Jeff King]

Usually these values get fed to fmt_ident, which will trim
any cruft anyway, but there are a few code paths which use
them directly. Let's clean them up for the benefit of those
callers. Furthermore, fmt_ident will look at the pre-trimmed
value and decide whether to invoke ERROR_ON_NO_NAME; this
check can be fooled by a name consisting only of spaces.

Note that we only bother to clean up when we are pulling the
information from gecos or from system files. Any other value
comes from a config file, where we will have cleaned up
accidental whitespace already.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ident.c b/ident.c
index cefb829..e279039 100644
--- a/ident.c
+++ b/ident.c
@@ -95,8 +95,10 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
 
 const char *ident_default_name(void)
 {
-	if (!git_default_name.len)
+	if (!git_default_name.len) {
 		copy_gecos(xgetpwuid_self(), &git_default_name);
+		strbuf_trim(&git_default_name);
+	}
 	return git_default_name.buf;
 }
 
@@ -110,6 +112,7 @@ const char *ident_default_email(void)
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else
 			copy_email(xgetpwuid_self(), &git_default_email);
+		strbuf_trim(&git_default_email);
 	}
 	return git_default_email.buf;
 }
-- 
1.7.10.1.19.g711d603

[PATCHv2 15/15] format-patch: refactor get_patch_filename

[Jeff King]

The get_patch_filename function expects a commit argument
and uses it to get the sanitized subject line when making a
patch filename. However, we also want to use this same
function for the cover letter, which does not have a commit
object. The current solution is to create a fake commit with
the subject "cover letter". Instead, let's make the
get_patch_filename interface more flexibile, and allow
passing a direct subject.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c | 35 +++++++----------------------------
 log-tree.c    | 19 +++++++++++--------
 log-tree.h    |  4 ++--
 3 files changed, 20 insertions(+), 38 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 656bddf..8010a40 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -663,7 +663,8 @@ static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
-static int reopen_stdout(struct commit *commit, struct rev_info *rev, int quiet)
+static int reopen_stdout(struct commit *commit, const char *subject,
+			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
 	int suffix_len = strlen(fmt_patch_suffix) + 1;
@@ -677,7 +678,7 @@ static int reopen_stdout(struct commit *commit, struct rev_info *rev, int quiet)
 			strbuf_addch(&filename, '/');
 	}
 
-	get_patch_filename(commit, rev->nr, fmt_patch_suffix, &filename);
+	get_patch_filename(commit, subject, rev->nr, fmt_patch_suffix, &filename);
 
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
@@ -778,7 +779,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	const char *encoding = "UTF-8";
 	struct diff_options opts;
 	int need_8bit_cte = 0;
-	struct commit *commit = NULL;
 	struct pretty_print_context pp = {0};
 
 	if (rev->commit_format != CMIT_FMT_EMAIL)
@@ -786,31 +786,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	committer = git_committer_info(0);
 
-	if (!numbered_files) {
-		/*
-		 * We fake a commit for the cover letter so we get the filename
-		 * desired.
-		 */
-		commit = xcalloc(1, sizeof(*commit));
-		commit->buffer = xmalloc(400);
-		snprintf(commit->buffer, 400,
-			"tree 0000000000000000000000000000000000000000\n"
-			"parent %s\n"
-			"author %s\n"
-			"committer %s\n\n"
-			"cover letter\n",
-			sha1_to_hex(head->object.sha1), committer, committer);
-	}
-
-	if (!use_stdout && reopen_stdout(commit, rev, quiet))
+	if (!use_stdout &&
+	    reopen_stdout(NULL, numbered_files ? NULL : "cover-letter", rev, quiet))
 		return;
 
-	if (commit) {
-
-		free(commit->buffer);
-		free(commit);
-	}
-
 	log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
 				&need_8bit_cte);
 
@@ -1405,8 +1384,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, sha1_to_hex(commit->object.sha1));
 		}
 
-		if (!use_stdout && reopen_stdout(numbered_files ? NULL : commit,
-						 &rev, quiet))
+		if (!use_stdout &&
+		    reopen_stdout(numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("Failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
 		free(commit->buffer);
diff --git a/log-tree.c b/log-tree.c
index 376d973..c894930 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -299,19 +299,22 @@ static unsigned int digits_in_number(unsigned int number)
 	return result;
 }
 
-void get_patch_filename(struct commit *commit, int nr, const char *suffix,
-			struct strbuf *buf)
+void get_patch_filename(struct commit *commit, const char *subject, int nr,
+			const char *suffix, struct strbuf *buf)
 {
 	int suffix_len = strlen(suffix) + 1;
 	int start_len = buf->len;
 
-	strbuf_addf(buf, commit ? "%04d-" : "%d", nr);
-	if (commit) {
+	strbuf_addf(buf, commit || subject ? "%04d-" : "%d", nr);
+	if (commit || subject) {
 		int max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;
 		struct pretty_print_context ctx = {0};
-		ctx.date_mode = DATE_NORMAL;
 
-		format_commit_message(commit, "%f", buf, &ctx);
+		if (subject)
+			strbuf_addstr(buf, subject);
+		else if (commit)
+			format_commit_message(commit, "%f", buf, &ctx);
+
 		if (max_len < buf->len)
 			strbuf_setlen(buf, max_len);
 		strbuf_addstr(buf, suffix);
@@ -384,8 +387,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			 mime_boundary_leader, opt->mime_boundary);
 		extra_headers = subject_buffer;
 
-		get_patch_filename(opt->numbered_files ? NULL : commit, opt->nr,
-				    opt->patch_suffix, &filename);
+		get_patch_filename(opt->numbered_files ? NULL : commit, NULL,
+				   opt->nr, opt->patch_suffix, &filename);
 		snprintf(buffer, sizeof(buffer) - 1,
 			 "\n--%s%s\n"
 			 "Content-Type: text/x-patch;"
diff --git a/log-tree.h b/log-tree.h
index 5c4cf7c..f5ac238 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -21,7 +21,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 void load_ref_decorations(int flags);
 
 #define FORMAT_PATCH_NAME_MAX 64
-void get_patch_filename(struct commit *commit, int nr, const char *suffix,
-			struct strbuf *buf);
+void get_patch_filename(struct commit *commit, const char *subject, int nr,
+			const char *suffix, struct strbuf *buf);
 
 #endif
-- 
1.7.10.1.19.g711d603

Re: [PATCHv2 14/15] ident: trim whitespace from default name/email

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... Any other value
> comes from a config file, where we will have cleaned up
> accidental whitespace already.

Are you referring to the behaviour of the config parser that removes
leading and trailing whitespaces when reading an unquoted string value?

If the user really wanted to have trailing whitespaces by quoting, we
would let it pass, in other words; it probably is a reasonable behaviour.

The same can be said about the environment variable GIT_COMMITTER_NAME and
friends, but "accidental whitespaces are cleaned up already" does not
apply to them.

So, isn't the real rationale behind this choice to allow users who give
leading or trailing whitespaces in the configuration and environment
variables on purpose use whatever value they specified?

I agree with the placement of trimming in this patch, but I do not quite
get (I do not mean "I do not agree with") what the quoted sentence wanted
to say.

Other than that single small "hrm...", the entire series was a pleasant
read.  Thanks.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  ident.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ident.c b/ident.c
> index cefb829..e279039 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -95,8 +95,10 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
>  
>  const char *ident_default_name(void)
>  {
> -	if (!git_default_name.len)
> +	if (!git_default_name.len) {
>  		copy_gecos(xgetpwuid_self(), &git_default_name);
> +		strbuf_trim(&git_default_name);
> +	}
>  	return git_default_name.buf;
>  }
>  
> @@ -110,6 +112,7 @@ const char *ident_default_email(void)
>  			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
>  		} else
>  			copy_email(xgetpwuid_self(), &git_default_email);
> +		strbuf_trim(&git_default_email);
>  	}
>  	return git_default_email.buf;
>  }

Re: [PATCHv2 14/15] ident: trim whitespace from default name/email

[Jeff King]

On Tue, May 22, 2012 at 09:55:19AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... Any other value
> > comes from a config file, where we will have cleaned up
> > accidental whitespace already.
> 
> Are you referring to the behaviour of the config parser that removes
> leading and trailing whitespaces when reading an unquoted string value?

Yes, exactly.

> If the user really wanted to have trailing whitespaces by quoting, we
> would let it pass, in other words; it probably is a reasonable behaviour.

Right, that's why I said "accidental" above; you can still do it, but I
think you'd have to be really trying. But I really wonder if we should
be blocking that, too. fmt_ident will drop it anyway, so this is really
only for code paths that want to use the email directly.

> The same can be said about the environment variable GIT_COMMITTER_NAME and
> friends, but "accidental whitespaces are cleaned up already" does not
> apply to them.

Yes, but those variables don't hit this code path at all. They go
straight to fmt_ident, which does the cleanup.  If you have code like
this:

  email = getenv("GIT_COMMITTER_EMAIL");
  if (!email)
          email = ident_default_email();

you might have trim-able spaces in the getenv result, and nobody is
handling that (fmt_ident does, but accessing it directly does not).
We could go a step further and replace ident_default_email with a
git_committer_email() which does the above, plus any whitespace
trimming.

> So, isn't the real rationale behind this choice to allow users who give
> leading or trailing whitespaces in the configuration and environment
> variables on purpose use whatever value they specified?

To be honest, the real rationale was laziness and a desire to keep the
interfaces simple. The risky code path (in my opinion) for getting extra
whitespace is pulling data from gecos, or from a file like
/etc/mailname. IOW, my intent was to block common accidents, but not
worry about somebody intentionally trying to push whitespace through.

> I agree with the placement of trimming in this patch, but I do not quite
> get (I do not mean "I do not agree with") what the quoted sentence wanted
> to say.
> 
> Other than that single small "hrm...", the entire series was a pleasant
> read.  Thanks.

Thanks. I'm happy to update this patch if we want it to be more
paranoid, or I can update the commit message if it just needs explained
better.

-Peff

Re: [PATCHv2 14/15] ident: trim whitespace from default name/email

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, May 22, 2012 at 09:55:19AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > ... Any other value
>> > comes from a config file, where we will have cleaned up
>> > accidental whitespace already.
>> 
>> Are you referring to the behaviour of the config parser that removes
>> leading and trailing whitespaces when reading an unquoted string value?
>
> Yes, exactly.
>
>> If the user really wanted to have trailing whitespaces by quoting, we
>> would let it pass, in other words; it probably is a reasonable behaviour.
>
> Right, that's why I said "accidental" above; you can still do it, but I
> think you'd have to be really trying. But I really wonder if we should
> be blocking that, too. fmt_ident will drop it anyway, so this is really
> only for code paths that want to use the email directly.

Thanks.

>> Other than that single small "hrm...", the entire series was a pleasant
>> read.  Thanks.
>
> Thanks. I'm happy to update this patch if we want it to be more
> paranoid, or I can update the commit message if it just needs explained
> better.

Neither is necessary. I was just puzzled a bit and was worried if I was
missing something.

[regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails

[Jonathan Nieder]

Hi Jeff,

Jeff King wrote:

> When we pull the user's name from the GECOS field of the
> passwd file (or generate an email address based on their
> username and hostname), we put the result into a
> static buffer.
[...]
> The conversion is mostly mechanical: replace string copies
> with strbuf equivalents, and access the strbuf.buf directly.
> There are a few exceptions:

This broke /etc/mailname handling.  Before:

	$ git var GIT_COMMITTER_IDENT
	Jonathan Nieder <jrn@mailname.example.com> 1359069165 -0800

After:

	$ git var GIT_COMMITTER_IDENT
	Jonathan Nieder <mailname.example.com> 1359069192 -0800

The cause:

[...]
> --- a/ident.c
> +++ b/ident.c
> @@ -19,42 +18,27 @@ int user_ident_explicitly_given;
[...]
> -static int add_mailname_host(char *buf, size_t len)
> +static int add_mailname_host(struct strbuf *buf)
>  {
>  	FILE *mailname;
>  
> @@ -65,7 +49,7 @@ static int add_mailname_host(char *buf, size_t len)
>  				strerror(errno));
>  		return -1;
>  	}
> -	if (!fgets(buf, len, mailname)) {
> +	if (strbuf_getline(buf, mailname, '\n') == EOF) {

This clears the strbuf.

How about something like this as a quick fix?

Reported-by: Mihai Rusu <dizzy@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git a/ident.c b/ident.c
index 73a06a1..cabd73f 100644
--- a/ident.c
+++ b/ident.c
@@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
 static int add_mailname_host(struct strbuf *buf)
 {
 	FILE *mailname;
+	struct strbuf mailnamebuf = STRBUF_INIT;
 
 	mailname = fopen("/etc/mailname", "r");
 	if (!mailname) {
@@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
 				strerror(errno));
 		return -1;
 	}
-	if (strbuf_getline(buf, mailname, '\n') == EOF) {
+	if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
 		if (ferror(mailname))
 			warning("cannot read /etc/mailname: %s",
 				strerror(errno));
+		strbuf_release(&mailnamebuf);
 		fclose(mailname);
 		return -1;
 	}
 	/* success! */
+	strbuf_addbuf(buf, &mailnamebuf);
+	strbuf_release(&mailnamebuf);
 	fclose(mailname);
 	return 0;
 }

Re: [regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails

[Jeff King]

On Thu, Jan 24, 2013 at 03:21:46PM -0800, Jonathan Nieder wrote:

> This broke /etc/mailname handling.  Before:
> 
> 	$ git var GIT_COMMITTER_IDENT
> 	Jonathan Nieder <jrn@mailname.example.com> 1359069165 -0800
> 
> After:
> 
> 	$ git var GIT_COMMITTER_IDENT
> 	Jonathan Nieder <mailname.example.com> 1359069192 -0800

Ick. I wonder how that slipped through...I know I was testing with
/etc/mailname when developing the series, because I'm on a Debian
system. We do even check this code path in t7502 (if you have the
AUTOIDENT prereq), but of course we can't verify the actual value
automatically, because it could be anything. So I guess I just missed it
during my manual testing, and the automated testing is insufficient to
catch this particular breakage.

> > -	if (!fgets(buf, len, mailname)) {
> > +	if (strbuf_getline(buf, mailname, '\n') == EOF) {
> 
> This clears the strbuf.

Right. Definitely the problem.

> How about something like this as a quick fix?
> 
> Reported-by: Mihai Rusu <dizzy@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> diff --git a/ident.c b/ident.c
> index 73a06a1..cabd73f 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
>  static int add_mailname_host(struct strbuf *buf)
>  {
>  	FILE *mailname;
> +	struct strbuf mailnamebuf = STRBUF_INIT;
>  
>  	mailname = fopen("/etc/mailname", "r");
>  	if (!mailname) {
> @@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
>  				strerror(errno));
>  		return -1;
>  	}
> -	if (strbuf_getline(buf, mailname, '\n') == EOF) {
> +	if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
>  		if (ferror(mailname))
>  			warning("cannot read /etc/mailname: %s",
>  				strerror(errno));
> +		strbuf_release(&mailnamebuf);
>  		fclose(mailname);
>  		return -1;
>  	}
>  	/* success! */
> +	strbuf_addbuf(buf, &mailnamebuf);
> +	strbuf_release(&mailnamebuf);
>  	fclose(mailname);
>  	return 0;
>  }

I think that is the only reasonable fix. Thanks for figuring it out.

We could expand the test in t7502 to check for "@" in the email, but it
feels weirdly specific to this bug. Either way,

Acked-by: Jeff King <peff@peff.net>

(with a proper commit message, of course).

-Peff

Re: [regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> (with a proper commit message, of course).

Will queue this one, to be merged to 'maint' and 'master'.

-- >8 --
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 24 Jan 2013 15:21:46 -0800
Subject: [PATCH] ident: do not drop username when reading from /etc/mailname

An earlier conversion from fgets() to strbuf_getline() in the
codepath to read from /etc/mailname to learn the default host-part
of the ident e-mail address forgot that strbuf_getline() stores the
line at the beginning of the buffer just like fgets().

The "username@" the caller has prepared in the strbuf, expecting the
function to append the host-part to it, was lost because of this.

Reported-by: Mihai Rusu <dizzy@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ident.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ident.c b/ident.c
index 484e0a9..fb4cc72 100644
--- a/ident.c
+++ b/ident.c
@@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
 static int add_mailname_host(struct strbuf *buf)
 {
 	FILE *mailname;
+	struct strbuf mailnamebuf = STRBUF_INIT;
 
 	mailname = fopen("/etc/mailname", "r");
 	if (!mailname) {
@@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
 				strerror(errno));
 		return -1;
 	}
-	if (strbuf_getline(buf, mailname, '\n') == EOF) {
+	if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
 		if (ferror(mailname))
 			warning("cannot read /etc/mailname: %s",
 				strerror(errno));
+		strbuf_release(&mailnamebuf);
 		fclose(mailname);
 		return -1;
 	}
 	/* success! */
+	strbuf_addbuf(buf, &mailnamebuf);
+	strbuf_release(&mailnamebuf);
 	fclose(mailname);
 	return 0;
 }
-- 
1.8.1.1.525.gdace530

Re: [regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails

[Jeff King]

On Fri, Jan 25, 2013 at 10:46:48AM -0800, Junio C Hamano wrote:

> Will queue this one, to be merged to 'maint' and 'master'.
> 
> -- >8 --
> From: Jonathan Nieder <jrnieder@gmail.com>
> Date: Thu, 24 Jan 2013 15:21:46 -0800
> Subject: [PATCH] ident: do not drop username when reading from /etc/mailname

Thanks, looks fine to me (and thanks to Jonathan). One nit:

> -	if (strbuf_getline(buf, mailname, '\n') == EOF) {
> +	if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
>  		if (ferror(mailname))
>  			warning("cannot read /etc/mailname: %s",
>  				strerror(errno));
> +		strbuf_release(&mailnamebuf);
>  		fclose(mailname);
>  		return -1;
>  	}

This strbuf_release is unnecessary, as an EOF return by definition means
we did not read anything. I don't mind it as a defensive measure,
though, in case the strbuf implementation changes to pre-allocate.

-Peff