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

[Jeff King <peff@xxxxxxxx>]

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

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html