threads / patch / 30500

patch, 2 partsChange 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>

Subject: [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>

## tl;dr

23 messages between May 10, 2012 and May 15, 2012. Diffs are folded; open one to read it.

replies: 22people: 4as markdown or json

Angus Hammond· May 10, 2012, 19:06 UTC · lore
---
 ident.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to ident.c +5 −5
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
Angus Hammond· May 10, 2012, 19:06 UTC · re: Angus Hammond · lore

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

---
 Documentation/git-commit-tree.txt |    9 ---------
 Documentation/git-var.txt         |    9 ---------
 2 files changed, 18 deletions(-)
Show changes to 2 files +0 −16

Documentation/git-commit-tree.txt, Documentation/git-var.txt

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
Angus Hammond· May 10, 2012, 19:21 UTC · re: Angus Hammond · lore

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>

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

Jeff King· May 10, 2012, 19:23 UTC · re: Angus Hammond · lore

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

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.

Show 6 quoted lines
> @@ -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.

Show 6 quoted lines
> @@ -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
Jeff King· May 10, 2012, 19:56 UTC · re: Jeff King · lore

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

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(-)
Show changes to 6 files +50 −73

builtin/fmt-merge-msg.c, cache.h, config.c, environment.c, http-push.c, ident.c

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);
Junio C Hamano· May 11, 2012, 22:53 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> 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 ;-)

Show 304 quoted lines
>
> ---
>  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);
Jeff King· May 11, 2012, 23:13 UTC · re: Junio C Hamano · lore

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

On Fri, May 11, 2012 at 03:53:43PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> >> 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
Jeff King· May 14, 2012, 16:28 UTC · re: Jeff King · lore

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

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(-)
Show changes to 8 files +50 −81

Documentation/git-commit-tree.txt, Documentation/git-var.txt, builtin/fmt-merge-msg.c, cache.h, config.c, environment.c, http-push.c, ident.c

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
Jeff King· May 14, 2012, 17:05 UTC · re: Jeff King · lore

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

On Mon, May 14, 2012 at 12:28:24PM -0400, Jeff King wrote:
Show 13 quoted lines
> 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(-)
Show changes to ident.c +4 −9
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
Jeff King· May 14, 2012, 21:02 UTC · re: Jeff King · lore

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

On Mon, May 14, 2012 at 12:28:24PM -0400, Jeff King wrote:
Show 18 quoted lines
> 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
Jeff King· May 14, 2012, 21:13 UTC · re: Jeff King · lore

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

On Mon, May 14, 2012 at 05:02:25PM -0400, Jeff King wrote:
Show 9 quoted lines
> 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:
Show 18 quoted lines
> --- 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
Jeff King· May 15, 2012, 01:54 UTC · re: Jeff King · lore

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

On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:
Show 6 quoted lines
> 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:
Show 23 quoted lines
> 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
Jeff King· May 15, 2012, 02:32 UTC · re: Jeff King · lore

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

On Mon, May 14, 2012 at 09:54:37PM -0400, Jeff King wrote:
Show 15 quoted lines
> 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
Junio C Hamano· May 15, 2012, 15:03 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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.

Jeff King· May 15, 2012, 17:47 UTC · re: Junio C Hamano · lore

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

On Tue, May 15, 2012 at 08:03:38AM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> 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.

Show 6 quoted lines
> > 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).

Show 7 quoted lines
> > 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
Junio C Hamano· May 15, 2012, 18:10 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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.
Jeff King· May 14, 2012, 16:36 UTC · re: Jeff King · lore

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

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(-)
Show changes to 5 files +18 −17

Documentation/git-commit-tree.txt, Documentation/git-var.txt, git-compat-util.h, ident.c, wrapper.c

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
Junio C Hamano· May 10, 2012, 20:04 UTC · re: Jeff King · lore

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

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

Jeff King· May 10, 2012, 20:22 UTC · re: Junio C Hamano · lore

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

On Thu, May 10, 2012 at 01:04:13PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> 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
Junio C Hamano· May 10, 2012, 20:28 UTC · re: Jeff King · lore

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

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.
Junio C Hamano· May 10, 2012, 19:43 UTC · re: Angus Hammond · lore

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>

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.
Angus Hammond· May 10, 2012, 19:57 UTC · re: Junio C Hamano · lore

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>

On 10 May 2012 20:43, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> 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

Nguyen Thai Ngoc Duy· May 11, 2012, 11:35 UTC · re: Angus Hammond · lore

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>

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
Show changes to ident.c +3 −3
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

← back to recent threads