{"thread":{"id":"30500","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 ","startedAt":"2012-05-10T19:06:09Z","lastAt":"2012-05-15T18:10:36Z","messageCount":23,"participants":["Angus Hammond","Jeff King","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"191339","messageId":"1336676770-17965-1-git-send-email-angusgh@gmail.com","threadId":"30500","inReplyTo":null,"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 ","fromName":"Angus Hammond","fromEmail":"angusgh@gmail.com","sentAt":"2012-05-10T19:06:09Z","receivedAt":"2012-05-10T19:06:09Z","isPatch":true,"sender":{"key":"angusgh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1839397?v=4"},"body":"---\n ident.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex 87c697c..51a7a73 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -46,7 +46,7 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz)\n \tif (len < sz)\n \t\tname[len] = 0;\n \telse\n-\t\tdie(\"Your parents must have hated you!\");\n+\t\tdie(\"Your GECOS field is too long.\");\n \n }\n \n@@ -106,7 +106,7 @@ static void copy_email(const struct passwd *pw)\n \t */\n \tsize_t len = strlen(pw->pw_name);\n \tif (len > sizeof(git_default_email)/2)\n-\t\tdie(\"Your sysadmin must hate you!\");\n+\t\tdie(\"Your name field in is too long.\");\n \tmemcpy(git_default_email, pw->pw_name, len);\n \tgit_default_email[len++] = '@';\n \n@@ -125,7 +125,7 @@ static void setup_ident(const char **name, const char **emailp)\n \tif (!*name && !git_default_name[0]) {\n \t\tpw = getpwuid(getuid());\n \t\tif (!pw)\n-\t\t\tdie(\"You don't exist. Go away!\");\n+\t\t\tdie(\"Could not read your GECOS field.\");\n \t\tcopy_gecos(pw, git_default_name, sizeof(git_default_name));\n \t}\n \tif (!*name)\n@@ -142,7 +142,7 @@ static void setup_ident(const char **name, const char **emailp)\n \t\t\tif (!pw)\n \t\t\t\tpw = getpwuid(getuid());\n \t\t\tif (!pw)\n-\t\t\t\tdie(\"You don't exist. Go away!\");\n+\t\t\t\tdie(\"Could not read your GECOS field.\");\n \t\t\tcopy_email(pw);\n \t\t}\n \t}\n@@ -325,7 +325,7 @@ const char *fmt_ident(const char *name, const char *email,\n \t\t\tdie(\"empty ident %s <%s> not allowed\", name, email);\n \t\tpw = getpwuid(getuid());\n \t\tif (!pw)\n-\t\t\tdie(\"You don't exist. Go away!\");\n+\t\t\tdie(\"Could not read your GECOS field.\");\n \t\tstrlcpy(git_default_name, pw->pw_name,\n \t\t\tsizeof(git_default_name));\n \t\tname = git_default_name;\n-- \n1.7.9.5\n"},{"id":"191340","messageId":"1336676770-17965-2-git-send-email-angusgh@gmail.com","threadId":"30500","inReplyTo":"1336676770-17965-1-git-send-email-angusgh@gmail.com","subject":"[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 ","fromName":"Angus Hammond","fromEmail":"angusgh@gmail.com","sentAt":"2012-05-10T19:06:10Z","receivedAt":"2012-05-10T19:06:10Z","isPatch":true,"sender":{"key":"angusgh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1839397?v=4"},"body":"---\n Documentation/git-commit-tree.txt | 9 ---------\n Documentation/git-var.txt | 9 ---------\n 2 files changed, 18 deletions(-)\n\ndiff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt\nindex cfb9906..eb8ee99 100644\n--- a/Documentation/git-commit-tree.txt\n+++ b/Documentation/git-commit-tree.txt\n@@ -88,15 +88,6 @@ for one to be entered and terminated with ^D.\n \n include::date-formats.txt[]\n \n-Diagnostics\n------------\n-You don't exist. Go away!::\n- The passwd(5) gecos field couldn't be read\n-Your parents must have hated you!::\n- The passwd(5) gecos field is longer than a giant static buffer.\n-Your sysadmin must hate you!::\n- The passwd(5) name field is longer than a giant static buffer.\n-\n Discussion\n ----------\n \ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 988a323..67edf58 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -59,15 +59,6 @@ ifdef::git-default-pager[]\n The build you are using chose '{git-default-pager}' as the default.\n endif::git-default-pager[]\n \n-Diagnostics\n------------\n-You don't exist. Go away!::\n- The passwd(5) gecos field couldn't be read\n-Your parents must have hated you!::\n- The passwd(5) gecos field is longer than a giant static buffer.\n-Your sysadmin must hate you!::\n- The passwd(5) name field is longer than a giant static buffer.\n-\n SEE ALSO\n --------\n linkgit:git-commit-tree[1]\n-- \n1.7.9.5\n"},{"id":"191342","messageId":"CAOBOgRb3d+oLLLYk6yU5-JUYXKth+UKguJ7gc-SX6wUcb5z1Fw@mail.gmail.com","threadId":"30500","inReplyTo":"1336676770-17965-2-git-send-email-angusgh@gmail.com","subject":"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 ","fromName":"Angus Hammond","fromEmail":"angusgh@gmail.com","sentAt":"2012-05-10T19:21:01Z","receivedAt":"2012-05-10T19:21:01Z","isPatch":true,"sender":{"key":"angusgh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1839397?v=4"},"body":"I have no idea how I managed to send this to myself and see it as one,\nproperly formatted email despite it being 2 formatted diabolically with\nentire commits messages in the subjects. Sorry about that.\nThese were meant to offer an alternative solution to the current issue over\nunusual error messages from commit-tree by bypassing the whole unix humour\nissue. They look it'll still be possible to apply them even if they are\nbadly sent. If not and people think they're worth while I'd be happy (try\nand) send them again without this mess.\nThanks\nAngus\n"},{"id":"191343","messageId":"20120510192339.GA32357@sigill.intra.peff.net","threadId":"30500","inReplyTo":"1336676770-17965-1-git-send-email-angusgh@gmail.com","subject":"Re: [PATCH 1/2] Change error messages in ident.c...","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-10T19:23:39Z","receivedAt":"2012-05-10T19:23:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 10, 2012 at 08:06:09PM +0100, Angus Hammond wrote:\n\n> Subject: Re: [PATCH 1/2] Change error messages in ident.c Make error messages\n> caused by failed reads of the /etc/passwd file easier to understand.\n> Signed-off-by: Angus Hammond \n\nHoly line-breaks, Batman!\n\nAs amusing as I find the existing messages, this is probably a good\ndirection (although I find it unlikely that most people would see the\nmessages under normal use).\n\nI am also tempted to suggest that we simply replace the static buffers\nwith dynamic strbufs. I guess that may open up new vectors for an\nattacker to convince git to allocate arbitrary amounts of memory, but\nthat is already pretty easy to do, so I doubt it's a big deal.\n\n> @@ -46,7 +46,7 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz)\n> \tif (len < sz)\n> \t\tname[len] = 0;\n> \telse\n> -\t\tdie(\"Your parents must have hated you!\");\n> +\t\tdie(\"Your GECOS field is too long.\");\n\nI know that \"GECOS\" is the standard name for the field, but I wonder if\nit is a bit unnecessarily jargon-y. Wouldn't something like:\n\n die(\"unable to get real name from system password file: name too long\");\n\nbe a little more friendly? It tells what operation we were actually\nperforming, and it doesn't use any jargon.\n\n> @@ -106,7 +106,7 @@ static void copy_email(const struct passwd *pw)\n> \t */\n> \tsize_t len = strlen(pw->pw_name);\n> \tif (len > sizeof(git_default_email)/2)\n> -\t\tdie(\"Your sysadmin must hate you!\");\n> +\t\tdie(\"Your name field in is too long.\");\n\ns/in is/is/. Also, similar complaints to above (if you see this message\nunexpectedly, you might ask \"which name field? One inside a commit\nobject?\").\n\n> [...]\n\nAnd similar comments for the rest of the messages.\n\n-Peff\n"},{"id":"191344","messageId":"7vpqabn7o1.fsf@alter.siamese.dyndns.org","threadId":"30500","inReplyTo":"1336676770-17965-1-git-send-email-angusgh@gmail.com","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 ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-10T19:43:42Z","receivedAt":"2012-05-10T19:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"They are one of the oldest and humorous messages we have in the system,\nand more importantly, users will see them only once on a badly configured\nsystem. If there is no real-life reason (e.g. \"if we do not change this\nmessage, Nuclear reactors will start misbehaving\"), I would rather keep\nthem as they are for hysterical raisins.\n\nBut that is just my preference to keep Linus's twisted sense of humor.\n"},{"id":"191345","messageId":"20120510195646.GA18276@sigill.intra.peff.net","threadId":"30500","inReplyTo":"20120510192339.GA32357@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] Change error messages in ident.c...","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-10T19:56:46Z","receivedAt":"2012-05-10T19:56:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 10, 2012 at 03:23:39PM -0400, Jeff King wrote:\n\n> I am also tempted to suggest that we simply replace the static buffers\n> with dynamic strbufs. I guess that may open up new vectors for an\n> attacker to convince git to allocate arbitrary amounts of memory, but\n> that is already pretty easy to do, so I doubt it's a big deal.\n\nFor reference, that patch would look like something like this:\n\n---\n builtin/fmt-merge-msg.c | 14 ++++----\n cache.h | 5 ++-\n config.c | 4 +--\n environment.c | 4 +--\n http-push.c | 2 +-\n ident.c | 94 ++++++++++++++++++-------------------------------\n 6 files changed, 50 insertions(+), 73 deletions(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex a517f17..bb716c8 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -230,7 +230,8 @@ static void add_branch_desc(struct strbuf *out, const char *name)\n static void record_person(int which, struct string_list *people,\n \t\t\t struct commit *commit)\n {\n-\tchar name_buf[MAX_GITNAME], *name, *name_end;\n+\tstruct strbuf name_buf = STRBUF_INIT;\n+\tchar *name, *name_end;\n \tstruct string_list_item *elem;\n \tconst char *field = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n \n@@ -243,17 +244,18 @@ static void record_person(int which, struct string_list *people,\n \t\tname_end--;\n \twhile (isspace(*name_end) && name <= name_end)\n \t\tname_end--;\n-\tif (name_end < name || name + MAX_GITNAME <= name_end)\n+\tif (name_end < name)\n \t\treturn;\n-\tmemcpy(name_buf, name, name_end - name + 1);\n-\tname_buf[name_end - name + 1] = '\\0';\n+\tstrbuf_add(&name_buf, name, name_end - name + 1);\n \n-\telem = string_list_lookup(people, name_buf);\n+\telem = string_list_lookup(people, name_buf.buf);\n \tif (!elem) {\n-\t\telem = string_list_insert(people, name_buf);\n+\t\telem = string_list_insert(people, name_buf.buf);\n \t\telem->util = (void *)0;\n \t}\n \telem->util = (void*)(util_as_integral(elem) + 1);\n+\n+\tstrbuf_release(&name_buf);\n }\n \n static int cmp_string_list_util_as_integral(const void *a_, const void *b_)\ndiff --git a/cache.h b/cache.h\nindex e14ffcd..0c1a332 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1138,9 +1138,8 @@ struct config_include_data {\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n-#define MAX_GITNAME (1000)\n-extern char git_default_email[MAX_GITNAME];\n-extern char git_default_name[MAX_GITNAME];\n+extern struct strbuf git_default_email;\n+extern struct strbuf git_default_name;\n #define IDENT_NAME_GIVEN 01\n #define IDENT_MAIL_GIVEN 02\n #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\ndiff --git a/config.c b/config.c\nindex eeee986..69cb08c 100644\n--- a/config.c\n+++ b/config.c\n@@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)\n \tif (!strcmp(var, \"user.name\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tstrlcpy(git_default_name, value, sizeof(git_default_name));\n+\t\tstrbuf_addstr(&git_default_name, value);\n \t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \t\treturn 0;\n \t}\n@@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)\n \tif (!strcmp(var, \"user.email\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tstrlcpy(git_default_email, value, sizeof(git_default_email));\n+\t\tstrbuf_addstr(&git_default_email, value);\n \t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\treturn 0;\n \t}\ndiff --git a/environment.c b/environment.c\nindex d7e6c65..f4e3b53 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -11,8 +11,8 @@\n #include \"refs.h\"\n #include \"fmt-merge-msg.h\"\n \n-char git_default_email[MAX_GITNAME];\n-char git_default_name[MAX_GITNAME];\n+struct strbuf git_default_email = STRBUF_INIT;\n+struct strbuf git_default_name = STRBUF_INIT;\n int user_ident_explicitly_given;\n int trust_executable_bit = 1;\n int trust_ctime = 1;\ndiff --git a/http-push.c b/http-push.c\nindex 1df7ab5..2362ffd 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \t\tep = strchr(ep + 1, '/');\n \t}\n \n-\tescaped = xml_entities(git_default_email);\n+\tescaped = xml_entities(git_default_email.buf);\n \tstrbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);\n \tfree(escaped);\n \ndiff --git a/ident.c b/ident.c\nindex 87c697c..c7bdb3f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -15,42 +15,27 @@ static char git_default_date[50];\n #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)\n #endif\n \n-static void copy_gecos(const struct passwd *w, char *name, size_t sz)\n+static void copy_gecos(const struct passwd *w, struct strbuf *name)\n {\n-\tchar *src, *dst;\n-\tsize_t len, nlen;\n-\n-\tnlen = strlen(w->pw_name);\n+\tchar *src;\n \n \t/* Traditionally GECOS field had office phone numbers etc, separated\n \t * with commas. Also & stands for capitalized form of the login name.\n \t */\n \n-\tfor (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {\n+\tfor (src = get_gecos(w); *src && *src != ','; src++) {\n \t\tint ch = *src;\n-\t\tif (ch != '&') {\n-\t\t\t*dst++ = ch;\n-\t\t\tif (ch == 0 || ch == ',')\n-\t\t\t\tbreak;\n-\t\t\tlen++;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (len + nlen < sz) {\n+\t\tif (ch != '&')\n+\t\t\tstrbuf_addch(name, ch);\n+\t\telse {\n \t\t\t/* Sorry, Mr. McDonald... */\n-\t\t\t*dst++ = toupper(*w->pw_name);\n-\t\t\tmemcpy(dst, w->pw_name + 1, nlen - 1);\n-\t\t\tdst += nlen - 1;\n-\t\t\tlen += nlen;\n+\t\t\tstrbuf_addch(name, toupper(*w->pw_name));\n+\t\t\tstrbuf_addstr(name, w->pw_name + 1);\n \t\t}\n \t}\n-\tif (len < sz)\n-\t\tname[len] = 0;\n-\telse\n-\t\tdie(\"Your parents must have hated you!\");\n-\n }\n \n-static int add_mailname_host(char *buf, size_t len)\n+static int add_mailname_host(struct strbuf *buf)\n {\n \tFILE *mailname;\n \n@@ -61,7 +46,7 @@ static int add_mailname_host(char *buf, size_t len)\n \t\t\t\tstrerror(errno));\n \t\treturn -1;\n \t}\n-\tif (!fgets(buf, len, mailname)) {\n+\tif (strbuf_getline(buf, mailname, '\\n') == EOF) {\n \t\tif (ferror(mailname))\n \t\t\twarning(\"cannot read /etc/mailname: %s\",\n \t\t\t\tstrerror(errno));\n@@ -73,48 +58,41 @@ static int add_mailname_host(char *buf, size_t len)\n \treturn 0;\n }\n \n-static void add_domainname(char *buf, size_t len)\n+static void add_domainname(struct strbuf *out)\n {\n+\tchar buf[1024];\n \tstruct hostent *he;\n-\tsize_t namelen;\n \tconst char *domainname;\n \n-\tif (gethostname(buf, len)) {\n+\tif (gethostname(buf, sizeof(buf))) {\n \t\twarning(\"cannot get host name: %s\", strerror(errno));\n-\t\tstrlcpy(buf, \"(none)\", len);\n+\t\tstrbuf_addstr(out, \"(none)\");\n \t\treturn;\n \t}\n-\tnamelen = strlen(buf);\n-\tif (memchr(buf, '.', namelen))\n+\tstrbuf_addstr(out, buf);\n+\tif (strchr(buf, '.'))\n \t\treturn;\n \n \the = gethostbyname(buf);\n-\tbuf[namelen++] = '.';\n-\tbuf += namelen;\n-\tlen -= namelen;\n+\tstrbuf_addch(out, '.');\n \tif (he && (domainname = strchr(he->h_name, '.')))\n-\t\tstrlcpy(buf, domainname + 1, len);\n+\t\tstrbuf_addstr(out, domainname + 1);\n \telse\n-\t\tstrlcpy(buf, \"(none)\", len);\n+\t\tstrbuf_addstr(out, \"(none)\");\n }\n \n-static void copy_email(const struct passwd *pw)\n+static void copy_email(const struct passwd *pw, struct strbuf *email)\n {\n \t/*\n \t * Make up a fake email address\n \t * (name + '@' + hostname [+ '.' + domainname])\n \t */\n-\tsize_t len = strlen(pw->pw_name);\n-\tif (len > sizeof(git_default_email)/2)\n-\t\tdie(\"Your sysadmin must hate you!\");\n-\tmemcpy(git_default_email, pw->pw_name, len);\n-\tgit_default_email[len++] = '@';\n-\n-\tif (!add_mailname_host(git_default_email + len,\n-\t\t\t\tsizeof(git_default_email) - len))\n+\tstrbuf_addstr(email, pw->pw_name);\n+\tstrbuf_addch(email, '@');\n+\n+\tif (!add_mailname_host(email))\n \t\treturn;\t/* read from \"/etc/mailname\" (Debian) */\n-\tadd_domainname(git_default_email + len,\n-\t\t\tsizeof(git_default_email) - len);\n+\tadd_domainname(email);\n }\n \n static void setup_ident(const char **name, const char **emailp)\n@@ -122,32 +100,31 @@ static void setup_ident(const char **name, const char **emailp)\n \tstruct passwd *pw = NULL;\n \n \t/* Get the name (\"gecos\") */\n-\tif (!*name && !git_default_name[0]) {\n+\tif (!*name && !git_default_name.len) {\n \t\tpw = getpwuid(getuid());\n \t\tif (!pw)\n \t\t\tdie(\"You don't exist. Go away!\");\n-\t\tcopy_gecos(pw, git_default_name, sizeof(git_default_name));\n+\t\tcopy_gecos(pw, &git_default_name);\n \t}\n \tif (!*name)\n-\t\t*name = git_default_name;\n+\t\t*name = git_default_name.buf;\n \n-\tif (!*emailp && !git_default_email[0]) {\n+\tif (!*emailp && !git_default_email.len) {\n \t\tconst char *email = getenv(\"EMAIL\");\n \n \t\tif (email && email[0]) {\n-\t\t\tstrlcpy(git_default_email, email,\n-\t\t\t\tsizeof(git_default_email));\n+\t\t\tstrbuf_addstr(&git_default_email, email);\n \t\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\t} else {\n \t\t\tif (!pw)\n \t\t\t\tpw = getpwuid(getuid());\n \t\t\tif (!pw)\n \t\t\t\tdie(\"You don't exist. Go away!\");\n-\t\t\tcopy_email(pw);\n+\t\t\tcopy_email(pw, &git_default_email);\n \t\t}\n \t}\n \tif (!*emailp)\n-\t\t*emailp = git_default_email;\n+\t\t*emailp = git_default_email.buf;\n \n \t/* And set the default date */\n \tif (!git_default_date[0])\n@@ -317,7 +294,7 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tstruct passwd *pw;\n \n \t\tif ((warn_on_no_name || error_on_no_name) &&\n-\t\t name == git_default_name && env_hint) {\n+\t\t name == git_default_name.buf && env_hint) {\n \t\t\tfputs(env_hint, stderr);\n \t\t\tenv_hint = NULL; /* warn only once */\n \t\t}\n@@ -326,9 +303,8 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tpw = getpwuid(getuid());\n \t\tif (!pw)\n \t\t\tdie(\"You don't exist. Go away!\");\n-\t\tstrlcpy(git_default_name, pw->pw_name,\n-\t\t\tsizeof(git_default_name));\n-\t\tname = git_default_name;\n+\t\tstrbuf_addstr(&git_default_name, pw->pw_name);\n+\t\tname = git_default_name.buf;\n \t}\n \n \tstrcpy(date, git_default_date);\n"},{"id":"191346","messageId":"CAOBOgRaAv=BoopuepHzBjDyMf-JVbmabwaGipczAtCjeUPtepw@mail.gmail.com","threadId":"30500","inReplyTo":"7vpqabn7o1.fsf@alter.siamese.dyndns.org","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 ","fromName":"Angus Hammond","fromEmail":"angusgh@gmail.com","sentAt":"2012-05-10T19:57:03Z","receivedAt":"2012-05-10T19:57:03Z","isPatch":true,"sender":{"key":"angusgh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1839397?v=4"},"body":"On 10 May 2012 20:43, Junio C Hamano wrote:\n> They are one of the oldest and humorous messages we have in the system,\n> and more importantly, users will see them only once on a badly configured\n> system.  If there is no real-life reason (e.g. \"if we do not change this\n> message, Nuclear reactors will start misbehaving\"), I would rather keep\n> them as they are for hysterical raisins.\nI'm not too worried either way, just tried to knock the patch out\nquickly because it came up and this seemed like the logical solution.\nIn all honesty though, whilst I don't have a problem with unix humour\nbeing in git, I do have a bit of a problem with it being in error\nmessages since when these are displayed it means that a users system\nis preventing them from using git for whatever reason, and at those\ntimes there's a good chance you're worried about fixing that problem,\nnot laughing at a joke made by Linus several years ago.\n\nJust my 2 cents. It's probably not worth too much bother since it'll\nonly ever show up very rarely.\nThanks\nAngus\n"},{"id":"191347","messageId":"7vipg3n6pu.fsf@alter.siamese.dyndns.org","threadId":"30500","inReplyTo":"20120510192339.GA32357@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] Change error messages in ident.c...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-10T20:04:13Z","receivedAt":"2012-05-10T20:04:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I am also tempted to suggest that we simply replace the static buffers\n> with dynamic strbufs.\n\nYeah, I think that is a proper approach for this issue, as it will make\ntwo of these messages unnecessary (or all? I couldn't think of a way\nto deal with missing getpwent case myself, though).\n"},{"id":"191350","messageId":"20120510202227.GA30965@sigill.intra.peff.net","threadId":"30500","inReplyTo":"7vipg3n6pu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Change error messages in ident.c...","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-10T20:22:27Z","receivedAt":"2012-05-10T20:22:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 10, 2012 at 01:04:13PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I am also tempted to suggest that we simply replace the static buffers\n> > with dynamic strbufs.\n> \n> Yeah, I think that is a proper approach for this issue, as it will make\n> two of these messages unnecessary (or all? I couldn't think of a way\n> to deal with missing getpwent case myself, though).\n\nIt doesn't get rid of the \"you don't exist\" message, and I think just\ndying there makes sense. But that is actually the one that I consider\nthe most likely to happen in practice, and should probably have a more\nuseful error message.\n\n-Peff\n"},{"id":"191351","messageId":"7vehqrn5lm.fsf@alter.siamese.dyndns.org","threadId":"30500","inReplyTo":"20120510202227.GA30965@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] Change error messages in ident.c...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-10T20:28:21Z","receivedAt":"2012-05-10T20:28:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It doesn't get rid of the \"you don't exist\" message, and I think just\n> dying there makes sense. But that is actually the one that I consider\n> the most likely to happen in practice, and should probably have a more\n> useful error message.\n\nYeah, I do not think anybody minds losing that phrasing from that message\n(the \"parents\" and \"sysadmin\" were the humorous ones), and we certainly\ncan phrase it differently, e.g.\n\n Your system didn't tell me your real name; hint: git help config\n and look for user.name\n\nor something.\n"},{"id":"191385","messageId":"CACsJy8AfrF8YyOA41F80igwG8DGfWyi+wRwpo6TvADe=FnZgag@mail.gmail.com","threadId":"30500","inReplyTo":"1336676770-17965-1-git-send-email-angusgh@gmail.com","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 ","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-11T11:35:12Z","receivedAt":"2012-05-11T11:35:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 11, 2012 at 2:06 AM, Angus Hammond wrote:\n> ---\n>  ident.c |   10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n\nWhile you are touching this, perhaps you can also turn all die(xxx) in\nthis file to die(_(xxx)), same for warning()? You touch 5 out of 11\nalready. And it helps make sure all the new strings are in the same\nhumor level (aka none). _() allows the messages to be translated in\nanother language, by the way.\n\nAlso this on top so we get nice advice\n\ndiff --git a/ident.c b/ident.c\nindex 87c697c..b5a631f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -289,7 +289,7 @@ person_only:\n }\n\n static const char *env_hint =\n-\"\\n\"\n+N_(\"\\n\"\n \"*** Please tell me who you are.\\n\"\n \"\\n\"\n \"Run\\n\"\n@@ -299,7 +299,7 @@ static const char *env_hint =\n \"\\n\"\n \"to set your account\\'s default identity.\\n\"\n \"Omit --global to set the identity only in this repository.\\n\"\n-\"\\n\";\n+\"\\n\");\n\n const char *fmt_ident(const char *name, const char *email,\n \t\t const char *date_str, int flag)\n@@ -318,7 +318,7 @@ const char *fmt_ident(const char *name, const char *email,\n\n \t\tif ((warn_on_no_name || error_on_no_name) &&\n \t\t name == git_default_name && env_hint) {\n-\t\t\tfputs(env_hint, stderr);\n+\t\t\tfputs(_(env_hint), stderr);\n \t\t\tenv_hint = NULL; /* warn only once */\n \t\t}\n \t\tif (error_on_no_name)\n-- \nDuy\n"},{"id":"191425","messageId":"7vehqqjpmw.fsf@alter.siamese.dyndns.org","threadId":"30500","inReplyTo":"20120510195646.GA18276@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] Change error messages in ident.c...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-11T22:53:43Z","receivedAt":"2012-05-11T22:53:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 10, 2012 at 03:23:39PM -0400, Jeff King wrote:\n>\n>> I am also tempted to suggest that we simply replace the static buffers\n>> with dynamic strbufs. I guess that may open up new vectors for an\n>> attacker to convince git to allocate arbitrary amounts of memory, but\n>> that is already pretty easy to do, so I doubt it's a big deal.\n>\n> For reference, that patch would look like something like this:\n\nLooks quite straight-forward and readable, I would say. Not only you gave\nus a legitimate excuse to get rid of the humourous messages, you lifted\nmost of the artificial limitations ('domainname' limit is still there but\nthat is not anything new) and use of strlcpy(), the last of which is a\nhuge win from my point of view ;-)\n\n>\n> ---\n> builtin/fmt-merge-msg.c | 14 ++++----\n> cache.h | 5 ++-\n> config.c | 4 +--\n> environment.c | 4 +--\n> http-push.c | 2 +-\n> ident.c | 94 ++++++++++++++++++-------------------------------\n> 6 files changed, 50 insertions(+), 73 deletions(-)\n>\n> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\n> index a517f17..bb716c8 100644\n> --- a/builtin/fmt-merge-msg.c\n> +++ b/builtin/fmt-merge-msg.c\n> @@ -230,7 +230,8 @@ static void add_branch_desc(struct strbuf *out, const char *name)\n> static void record_person(int which, struct string_list *people,\n> \t\t\t struct commit *commit)\n> {\n> -\tchar name_buf[MAX_GITNAME], *name, *name_end;\n> +\tstruct strbuf name_buf = STRBUF_INIT;\n> +\tchar *name, *name_end;\n> \tstruct string_list_item *elem;\n> \tconst char *field = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n> \n> @@ -243,17 +244,18 @@ static void record_person(int which, struct string_list *people,\n> \t\tname_end--;\n> \twhile (isspace(*name_end) && name <= name_end)\n> \t\tname_end--;\n> -\tif (name_end < name || name + MAX_GITNAME <= name_end)\n> +\tif (name_end < name)\n> \t\treturn;\n> -\tmemcpy(name_buf, name, name_end - name + 1);\n> -\tname_buf[name_end - name + 1] = '\\0';\n> +\tstrbuf_add(&name_buf, name, name_end - name + 1);\n> \n> -\telem = string_list_lookup(people, name_buf);\n> +\telem = string_list_lookup(people, name_buf.buf);\n> \tif (!elem) {\n> -\t\telem = string_list_insert(people, name_buf);\n> +\t\telem = string_list_insert(people, name_buf.buf);\n> \t\telem->util = (void *)0;\n> \t}\n> \telem->util = (void*)(util_as_integral(elem) + 1);\n> +\n> +\tstrbuf_release(&name_buf);\n> }\n> \n> static int cmp_string_list_util_as_integral(const void *a_, const void *b_)\n> diff --git a/cache.h b/cache.h\n> index e14ffcd..0c1a332 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1138,9 +1138,8 @@ struct config_include_data {\n> #define CONFIG_INCLUDE_INIT { 0 }\n> extern int git_config_include(const char *name, const char *value, void *data);\n> \n> -#define MAX_GITNAME (1000)\n> -extern char git_default_email[MAX_GITNAME];\n> -extern char git_default_name[MAX_GITNAME];\n> +extern struct strbuf git_default_email;\n> +extern struct strbuf git_default_name;\n> #define IDENT_NAME_GIVEN 01\n> #define IDENT_MAIL_GIVEN 02\n> #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n> diff --git a/config.c b/config.c\n> index eeee986..69cb08c 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)\n> \tif (!strcmp(var, \"user.name\")) {\n> \t\tif (!value)\n> \t\t\treturn config_error_nonbool(var);\n> -\t\tstrlcpy(git_default_name, value, sizeof(git_default_name));\n> +\t\tstrbuf_addstr(&git_default_name, value);\n> \t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n> \t\treturn 0;\n> \t}\n> @@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)\n> \tif (!strcmp(var, \"user.email\")) {\n> \t\tif (!value)\n> \t\t\treturn config_error_nonbool(var);\n> -\t\tstrlcpy(git_default_email, value, sizeof(git_default_email));\n> +\t\tstrbuf_addstr(&git_default_email, value);\n> \t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> \t\treturn 0;\n> \t}\n> diff --git a/environment.c b/environment.c\n> index d7e6c65..f4e3b53 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -11,8 +11,8 @@\n> #include \"refs.h\"\n> #include \"fmt-merge-msg.h\"\n> \n> -char git_default_email[MAX_GITNAME];\n> -char git_default_name[MAX_GITNAME];\n> +struct strbuf git_default_email = STRBUF_INIT;\n> +struct strbuf git_default_name = STRBUF_INIT;\n> int user_ident_explicitly_given;\n> int trust_executable_bit = 1;\n> int trust_ctime = 1;\n> diff --git a/http-push.c b/http-push.c\n> index 1df7ab5..2362ffd 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n> \t\tep = strchr(ep + 1, '/');\n> \t}\n> \n> -\tescaped = xml_entities(git_default_email);\n> +\tescaped = xml_entities(git_default_email.buf);\n> \tstrbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);\n> \tfree(escaped);\n> \n> diff --git a/ident.c b/ident.c\n> index 87c697c..c7bdb3f 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -15,42 +15,27 @@ static char git_default_date[50];\n> #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)\n> #endif\n> \n> -static void copy_gecos(const struct passwd *w, char *name, size_t sz)\n> +static void copy_gecos(const struct passwd *w, struct strbuf *name)\n> {\n> -\tchar *src, *dst;\n> -\tsize_t len, nlen;\n> -\n> -\tnlen = strlen(w->pw_name);\n> +\tchar *src;\n> \n> \t/* Traditionally GECOS field had office phone numbers etc, separated\n> \t * with commas. Also & stands for capitalized form of the login name.\n> \t */\n> \n> -\tfor (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {\n> +\tfor (src = get_gecos(w); *src && *src != ','; src++) {\n> \t\tint ch = *src;\n> -\t\tif (ch != '&') {\n> -\t\t\t*dst++ = ch;\n> -\t\t\tif (ch == 0 || ch == ',')\n> -\t\t\t\tbreak;\n> -\t\t\tlen++;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tif (len + nlen < sz) {\n> +\t\tif (ch != '&')\n> +\t\t\tstrbuf_addch(name, ch);\n> +\t\telse {\n> \t\t\t/* Sorry, Mr. McDonald... */\n> -\t\t\t*dst++ = toupper(*w->pw_name);\n> -\t\t\tmemcpy(dst, w->pw_name + 1, nlen - 1);\n> -\t\t\tdst += nlen - 1;\n> -\t\t\tlen += nlen;\n> +\t\t\tstrbuf_addch(name, toupper(*w->pw_name));\n> +\t\t\tstrbuf_addstr(name, w->pw_name + 1);\n> \t\t}\n> \t}\n> -\tif (len < sz)\n> -\t\tname[len] = 0;\n> -\telse\n> -\t\tdie(\"Your parents must have hated you!\");\n> -\n> }\n> \n> -static int add_mailname_host(char *buf, size_t len)\n> +static int add_mailname_host(struct strbuf *buf)\n> {\n> \tFILE *mailname;\n> \n> @@ -61,7 +46,7 @@ static int add_mailname_host(char *buf, size_t len)\n> \t\t\t\tstrerror(errno));\n> \t\treturn -1;\n> \t}\n> -\tif (!fgets(buf, len, mailname)) {\n> +\tif (strbuf_getline(buf, mailname, '\\n') == EOF) {\n> \t\tif (ferror(mailname))\n> \t\t\twarning(\"cannot read /etc/mailname: %s\",\n> \t\t\t\tstrerror(errno));\n> @@ -73,48 +58,41 @@ static int add_mailname_host(char *buf, size_t len)\n> \treturn 0;\n> }\n> \n> -static void add_domainname(char *buf, size_t len)\n> +static void add_domainname(struct strbuf *out)\n> {\n> +\tchar buf[1024];\n> \tstruct hostent *he;\n> -\tsize_t namelen;\n> \tconst char *domainname;\n> \n> -\tif (gethostname(buf, len)) {\n> +\tif (gethostname(buf, sizeof(buf))) {\n> \t\twarning(\"cannot get host name: %s\", strerror(errno));\n> -\t\tstrlcpy(buf, \"(none)\", len);\n> +\t\tstrbuf_addstr(out, \"(none)\");\n> \t\treturn;\n> \t}\n> -\tnamelen = strlen(buf);\n> -\tif (memchr(buf, '.', namelen))\n> +\tstrbuf_addstr(out, buf);\n> +\tif (strchr(buf, '.'))\n> \t\treturn;\n> \n> \the = gethostbyname(buf);\n> -\tbuf[namelen++] = '.';\n> -\tbuf += namelen;\n> -\tlen -= namelen;\n> +\tstrbuf_addch(out, '.');\n> \tif (he && (domainname = strchr(he->h_name, '.')))\n> -\t\tstrlcpy(buf, domainname + 1, len);\n> +\t\tstrbuf_addstr(out, domainname + 1);\n> \telse\n> -\t\tstrlcpy(buf, \"(none)\", len);\n> +\t\tstrbuf_addstr(out, \"(none)\");\n> }\n> \n> -static void copy_email(const struct passwd *pw)\n> +static void copy_email(const struct passwd *pw, struct strbuf *email)\n> {\n> \t/*\n> \t * Make up a fake email address\n> \t * (name + '@' + hostname [+ '.' + domainname])\n> \t */\n> -\tsize_t len = strlen(pw->pw_name);\n> -\tif (len > sizeof(git_default_email)/2)\n> -\t\tdie(\"Your sysadmin must hate you!\");\n> -\tmemcpy(git_default_email, pw->pw_name, len);\n> -\tgit_default_email[len++] = '@';\n> -\n> -\tif (!add_mailname_host(git_default_email + len,\n> -\t\t\t\tsizeof(git_default_email) - len))\n> +\tstrbuf_addstr(email, pw->pw_name);\n> +\tstrbuf_addch(email, '@');\n> +\n> +\tif (!add_mailname_host(email))\n> \t\treturn;\t/* read from \"/etc/mailname\" (Debian) */\n> -\tadd_domainname(git_default_email + len,\n> -\t\t\tsizeof(git_default_email) - len);\n> +\tadd_domainname(email);\n> }\n> \n> static void setup_ident(const char **name, const char **emailp)\n> @@ -122,32 +100,31 @@ static void setup_ident(const char **name, const char **emailp)\n> \tstruct passwd *pw = NULL;\n> \n> \t/* Get the name (\"gecos\") */\n> -\tif (!*name && !git_default_name[0]) {\n> +\tif (!*name && !git_default_name.len) {\n> \t\tpw = getpwuid(getuid());\n> \t\tif (!pw)\n> \t\t\tdie(\"You don't exist. Go away!\");\n> -\t\tcopy_gecos(pw, git_default_name, sizeof(git_default_name));\n> +\t\tcopy_gecos(pw, &git_default_name);\n> \t}\n> \tif (!*name)\n> -\t\t*name = git_default_name;\n> +\t\t*name = git_default_name.buf;\n> \n> -\tif (!*emailp && !git_default_email[0]) {\n> +\tif (!*emailp && !git_default_email.len) {\n> \t\tconst char *email = getenv(\"EMAIL\");\n> \n> \t\tif (email && email[0]) {\n> -\t\t\tstrlcpy(git_default_email, email,\n> -\t\t\t\tsizeof(git_default_email));\n> +\t\t\tstrbuf_addstr(&git_default_email, email);\n> \t\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> \t\t} else {\n> \t\t\tif (!pw)\n> \t\t\t\tpw = getpwuid(getuid());\n> \t\t\tif (!pw)\n> \t\t\t\tdie(\"You don't exist. Go away!\");\n> -\t\t\tcopy_email(pw);\n> +\t\t\tcopy_email(pw, &git_default_email);\n> \t\t}\n> \t}\n> \tif (!*emailp)\n> -\t\t*emailp = git_default_email;\n> +\t\t*emailp = git_default_email.buf;\n> \n> \t/* And set the default date */\n> \tif (!git_default_date[0])\n> @@ -317,7 +294,7 @@ const char *fmt_ident(const char *name, const char *email,\n> \t\tstruct passwd *pw;\n> \n> \t\tif ((warn_on_no_name || error_on_no_name) &&\n> -\t\t name == git_default_name && env_hint) {\n> +\t\t name == git_default_name.buf && env_hint) {\n> \t\t\tfputs(env_hint, stderr);\n> \t\t\tenv_hint = NULL; /* warn only once */\n> \t\t}\n> @@ -326,9 +303,8 @@ const char *fmt_ident(const char *name, const char *email,\n> \t\tpw = getpwuid(getuid());\n> \t\tif (!pw)\n> \t\t\tdie(\"You don't exist. Go away!\");\n> -\t\tstrlcpy(git_default_name, pw->pw_name,\n> -\t\t\tsizeof(git_default_name));\n> -\t\tname = git_default_name;\n> +\t\tstrbuf_addstr(&git_default_name, pw->pw_name);\n> +\t\tname = git_default_name.buf;\n> \t}\n> \n> \tstrcpy(date, git_default_date);\n"},{"id":"191426","messageId":"20120511231303.GA24611@sigill.intra.peff.net","threadId":"30500","inReplyTo":"7vehqqjpmw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Change error messages in ident.c...","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-11T23:13:03Z","receivedAt":"2012-05-11T23:13:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 11, 2012 at 03:53:43PM -0700, Junio C Hamano wrote:\n\n> >> I am also tempted to suggest that we simply replace the static buffers\n> >> with dynamic strbufs. I guess that may open up new vectors for an\n> >> attacker to convince git to allocate arbitrary amounts of memory, but\n> >> that is already pretty easy to do, so I doubt it's a big deal.\n> >\n> > For reference, that patch would look like something like this:\n> \n> Looks quite straight-forward and readable, I would say. Not only you gave\n> us a legitimate excuse to get rid of the humourous messages, you lifted\n> most of the artificial limitations ('domainname' limit is still there but\n> that is not anything new) and use of strlcpy(), the last of which is a\n> huge win from my point of view ;-)\n\nThanks. I'll re-roll with a commit message, and a follow-on patch to fix\nthe \"you don't exist\" message. But probably tomorrow, as I am just\nfinishing gitting for the day.\n\n-Peff\n"},{"id":"191499","messageId":"20120514162824.GA24457@sigill.intra.peff.net","threadId":"30500","inReplyTo":"20120511231303.GA24611@sigill.intra.peff.net","subject":"[PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-14T16:28:24Z","receivedAt":"2012-05-14T16:28:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we pull the user's name from the GECOS field of the\npasswd file (or generate an email address based on their\nusername and hostname), we put the result into a\nstatic buffer. While it's extremely unlikely that anybody\never hit these limits (after all, in such a case their\nparents must have hated them), we still had to deal with the\nerror cases in our code.\n\nConverting these static buffers to strbufs lets us simplify\nthe code and drop some error messages from the documentation\nthat have confused some users.\n\nNote that there is still one length limitation: the\ngethostname interface requires us to provide a static\nbuffer, so we arbitrarily choose 1024 bytes for the\nhostname.\n\nSigned-off-by: Jeff King \n---\nI noticed in add_domainname that we look up the host via gethostname,\nand then if it is not fully qualified, call gethostbyname and steal the\ndomain portion of the result, tacking it onto the hostname we got.\n\nThat seems oddly complex to me, and like it could result in a bogus\nhostname if the unqualified name does not match the first part of the\nreturned qualified name. E.g., if the /etc/hosts file contains something\nlike:\n\n 192.168.1.1 foo.example.com bar.example.com bar\n\n(and your hostname is \"bar\"). I doubt it matters much in practice, and\nit is outside the scope of this patch, so I left it for now.\n\n Documentation/git-commit-tree.txt | 4 --\n Documentation/git-var.txt | 4 --\n builtin/fmt-merge-msg.c | 14 +++---\n cache.h | 5 +--\n config.c | 4 +-\n environment.c | 4 +-\n http-push.c | 2 +-\n ident.c | 94 +++++++++++++++------------------------\n 8 files changed, 50 insertions(+), 81 deletions(-)\n\ndiff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt\nindex cfb9906..eb12b2d 100644\n--- a/Documentation/git-commit-tree.txt\n+++ b/Documentation/git-commit-tree.txt\n@@ -92,10 +92,6 @@ Diagnostics\n -----------\n You don't exist. Go away!::\n The passwd(5) gecos field couldn't be read\n-Your parents must have hated you!::\n- The passwd(5) gecos field is longer than a giant static buffer.\n-Your sysadmin must hate you!::\n- The passwd(5) name field is longer than a giant static buffer.\n \n Discussion\n ----------\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 988a323..3f703e3 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -63,10 +63,6 @@ Diagnostics\n -----------\n You don't exist. Go away!::\n The passwd(5) gecos field couldn't be read\n-Your parents must have hated you!::\n- The passwd(5) gecos field is longer than a giant static buffer.\n-Your sysadmin must hate you!::\n- The passwd(5) name field is longer than a giant static buffer.\n \n SEE ALSO\n --------\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex a517f17..bb716c8 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -230,7 +230,8 @@ static void add_branch_desc(struct strbuf *out, const char *name)\n static void record_person(int which, struct string_list *people,\n \t\t\t struct commit *commit)\n {\n-\tchar name_buf[MAX_GITNAME], *name, *name_end;\n+\tstruct strbuf name_buf = STRBUF_INIT;\n+\tchar *name, *name_end;\n \tstruct string_list_item *elem;\n \tconst char *field = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n \n@@ -243,17 +244,18 @@ static void record_person(int which, struct string_list *people,\n \t\tname_end--;\n \twhile (isspace(*name_end) && name <= name_end)\n \t\tname_end--;\n-\tif (name_end < name || name + MAX_GITNAME <= name_end)\n+\tif (name_end < name)\n \t\treturn;\n-\tmemcpy(name_buf, name, name_end - name + 1);\n-\tname_buf[name_end - name + 1] = '\\0';\n+\tstrbuf_add(&name_buf, name, name_end - name + 1);\n \n-\telem = string_list_lookup(people, name_buf);\n+\telem = string_list_lookup(people, name_buf.buf);\n \tif (!elem) {\n-\t\telem = string_list_insert(people, name_buf);\n+\t\telem = string_list_insert(people, name_buf.buf);\n \t\telem->util = (void *)0;\n \t}\n \telem->util = (void*)(util_as_integral(elem) + 1);\n+\n+\tstrbuf_release(&name_buf);\n }\n \n static int cmp_string_list_util_as_integral(const void *a_, const void *b_)\ndiff --git a/cache.h b/cache.h\nindex e14ffcd..0c1a332 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1138,9 +1138,8 @@ struct config_include_data {\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n-#define MAX_GITNAME (1000)\n-extern char git_default_email[MAX_GITNAME];\n-extern char git_default_name[MAX_GITNAME];\n+extern struct strbuf git_default_email;\n+extern struct strbuf git_default_name;\n #define IDENT_NAME_GIVEN 01\n #define IDENT_MAIL_GIVEN 02\n #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\ndiff --git a/config.c b/config.c\nindex eeee986..69cb08c 100644\n--- a/config.c\n+++ b/config.c\n@@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)\n \tif (!strcmp(var, \"user.name\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tstrlcpy(git_default_name, value, sizeof(git_default_name));\n+\t\tstrbuf_addstr(&git_default_name, value);\n \t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \t\treturn 0;\n \t}\n@@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)\n \tif (!strcmp(var, \"user.email\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tstrlcpy(git_default_email, value, sizeof(git_default_email));\n+\t\tstrbuf_addstr(&git_default_email, value);\n \t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\treturn 0;\n \t}\ndiff --git a/environment.c b/environment.c\nindex d7e6c65..f4e3b53 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -11,8 +11,8 @@\n #include \"refs.h\"\n #include \"fmt-merge-msg.h\"\n \n-char git_default_email[MAX_GITNAME];\n-char git_default_name[MAX_GITNAME];\n+struct strbuf git_default_email = STRBUF_INIT;\n+struct strbuf git_default_name = STRBUF_INIT;\n int user_ident_explicitly_given;\n int trust_executable_bit = 1;\n int trust_ctime = 1;\ndiff --git a/http-push.c b/http-push.c\nindex 1df7ab5..2362ffd 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \t\tep = strchr(ep + 1, '/');\n \t}\n \n-\tescaped = xml_entities(git_default_email);\n+\tescaped = xml_entities(git_default_email.buf);\n \tstrbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);\n \tfree(escaped);\n \ndiff --git a/ident.c b/ident.c\nindex 87c697c..c7bdb3f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -15,42 +15,27 @@ static char git_default_date[50];\n #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)\n #endif\n \n-static void copy_gecos(const struct passwd *w, char *name, size_t sz)\n+static void copy_gecos(const struct passwd *w, struct strbuf *name)\n {\n-\tchar *src, *dst;\n-\tsize_t len, nlen;\n-\n-\tnlen = strlen(w->pw_name);\n+\tchar *src;\n \n \t/* Traditionally GECOS field had office phone numbers etc, separated\n \t * with commas. Also & stands for capitalized form of the login name.\n \t */\n \n-\tfor (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {\n+\tfor (src = get_gecos(w); *src && *src != ','; src++) {\n \t\tint ch = *src;\n-\t\tif (ch != '&') {\n-\t\t\t*dst++ = ch;\n-\t\t\tif (ch == 0 || ch == ',')\n-\t\t\t\tbreak;\n-\t\t\tlen++;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (len + nlen < sz) {\n+\t\tif (ch != '&')\n+\t\t\tstrbuf_addch(name, ch);\n+\t\telse {\n \t\t\t/* Sorry, Mr. McDonald... */\n-\t\t\t*dst++ = toupper(*w->pw_name);\n-\t\t\tmemcpy(dst, w->pw_name + 1, nlen - 1);\n-\t\t\tdst += nlen - 1;\n-\t\t\tlen += nlen;\n+\t\t\tstrbuf_addch(name, toupper(*w->pw_name));\n+\t\t\tstrbuf_addstr(name, w->pw_name + 1);\n \t\t}\n \t}\n-\tif (len < sz)\n-\t\tname[len] = 0;\n-\telse\n-\t\tdie(\"Your parents must have hated you!\");\n-\n }\n \n-static int add_mailname_host(char *buf, size_t len)\n+static int add_mailname_host(struct strbuf *buf)\n {\n \tFILE *mailname;\n \n@@ -61,7 +46,7 @@ static int add_mailname_host(char *buf, size_t len)\n \t\t\t\tstrerror(errno));\n \t\treturn -1;\n \t}\n-\tif (!fgets(buf, len, mailname)) {\n+\tif (strbuf_getline(buf, mailname, '\\n') == EOF) {\n \t\tif (ferror(mailname))\n \t\t\twarning(\"cannot read /etc/mailname: %s\",\n \t\t\t\tstrerror(errno));\n@@ -73,48 +58,41 @@ static int add_mailname_host(char *buf, size_t len)\n \treturn 0;\n }\n \n-static void add_domainname(char *buf, size_t len)\n+static void add_domainname(struct strbuf *out)\n {\n+\tchar buf[1024];\n \tstruct hostent *he;\n-\tsize_t namelen;\n \tconst char *domainname;\n \n-\tif (gethostname(buf, len)) {\n+\tif (gethostname(buf, sizeof(buf))) {\n \t\twarning(\"cannot get host name: %s\", strerror(errno));\n-\t\tstrlcpy(buf, \"(none)\", len);\n+\t\tstrbuf_addstr(out, \"(none)\");\n \t\treturn;\n \t}\n-\tnamelen = strlen(buf);\n-\tif (memchr(buf, '.', namelen))\n+\tstrbuf_addstr(out, buf);\n+\tif (strchr(buf, '.'))\n \t\treturn;\n \n \the = gethostbyname(buf);\n-\tbuf[namelen++] = '.';\n-\tbuf += namelen;\n-\tlen -= namelen;\n+\tstrbuf_addch(out, '.');\n \tif (he && (domainname = strchr(he->h_name, '.')))\n-\t\tstrlcpy(buf, domainname + 1, len);\n+\t\tstrbuf_addstr(out, domainname + 1);\n \telse\n-\t\tstrlcpy(buf, \"(none)\", len);\n+\t\tstrbuf_addstr(out, \"(none)\");\n }\n \n-static void copy_email(const struct passwd *pw)\n+static void copy_email(const struct passwd *pw, struct strbuf *email)\n {\n \t/*\n \t * Make up a fake email address\n \t * (name + '@' + hostname [+ '.' + domainname])\n \t */\n-\tsize_t len = strlen(pw->pw_name);\n-\tif (len > sizeof(git_default_email)/2)\n-\t\tdie(\"Your sysadmin must hate you!\");\n-\tmemcpy(git_default_email, pw->pw_name, len);\n-\tgit_default_email[len++] = '@';\n-\n-\tif (!add_mailname_host(git_default_email + len,\n-\t\t\t\tsizeof(git_default_email) - len))\n+\tstrbuf_addstr(email, pw->pw_name);\n+\tstrbuf_addch(email, '@');\n+\n+\tif (!add_mailname_host(email))\n \t\treturn;\t/* read from \"/etc/mailname\" (Debian) */\n-\tadd_domainname(git_default_email + len,\n-\t\t\tsizeof(git_default_email) - len);\n+\tadd_domainname(email);\n }\n \n static void setup_ident(const char **name, const char **emailp)\n@@ -122,32 +100,31 @@ static void setup_ident(const char **name, const char **emailp)\n \tstruct passwd *pw = NULL;\n \n \t/* Get the name (\"gecos\") */\n-\tif (!*name && !git_default_name[0]) {\n+\tif (!*name && !git_default_name.len) {\n \t\tpw = getpwuid(getuid());\n \t\tif (!pw)\n \t\t\tdie(\"You don't exist. Go away!\");\n-\t\tcopy_gecos(pw, git_default_name, sizeof(git_default_name));\n+\t\tcopy_gecos(pw, &git_default_name);\n \t}\n \tif (!*name)\n-\t\t*name = git_default_name;\n+\t\t*name = git_default_name.buf;\n \n-\tif (!*emailp && !git_default_email[0]) {\n+\tif (!*emailp && !git_default_email.len) {\n \t\tconst char *email = getenv(\"EMAIL\");\n \n \t\tif (email && email[0]) {\n-\t\t\tstrlcpy(git_default_email, email,\n-\t\t\t\tsizeof(git_default_email));\n+\t\t\tstrbuf_addstr(&git_default_email, email);\n \t\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\t} else {\n \t\t\tif (!pw)\n \t\t\t\tpw = getpwuid(getuid());\n \t\t\tif (!pw)\n \t\t\t\tdie(\"You don't exist. Go away!\");\n-\t\t\tcopy_email(pw);\n+\t\t\tcopy_email(pw, &git_default_email);\n \t\t}\n \t}\n \tif (!*emailp)\n-\t\t*emailp = git_default_email;\n+\t\t*emailp = git_default_email.buf;\n \n \t/* And set the default date */\n \tif (!git_default_date[0])\n@@ -317,7 +294,7 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tstruct passwd *pw;\n \n \t\tif ((warn_on_no_name || error_on_no_name) &&\n-\t\t name == git_default_name && env_hint) {\n+\t\t name == git_default_name.buf && env_hint) {\n \t\t\tfputs(env_hint, stderr);\n \t\t\tenv_hint = NULL; /* warn only once */\n \t\t}\n@@ -326,9 +303,8 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tpw = getpwuid(getuid());\n \t\tif (!pw)\n \t\t\tdie(\"You don't exist. Go away!\");\n-\t\tstrlcpy(git_default_name, pw->pw_name,\n-\t\t\tsizeof(git_default_name));\n-\t\tname = git_default_name;\n+\t\tstrbuf_addstr(&git_default_name, pw->pw_name);\n+\t\tname = git_default_name.buf;\n \t}\n \n \tstrcpy(date, git_default_date);\n-- \n1.7.10.2.8.g1101eed\n"},{"id":"191502","messageId":"20120514163622.GB24457@sigill.intra.peff.net","threadId":"30500","inReplyTo":"20120511231303.GA24611@sigill.intra.peff.net","subject":"[PATCH 2/2] ident: report passwd errors with a more friendly message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-14T16:36:22Z","receivedAt":"2012-05-14T16:36:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When getpwuid fails, we give a cute but cryptic message.\nWhile it makes sense if you know that getpwuid or identity\nfunctions are being called, this code is triggered behind\nthe scenes by quite a few git commands these days (e.g.,\nreceive-pack on a remote server might use it for a reflog;\nthe current message is hard to distinguish from an\nauthentication error). Let's switch to something that gives\na little more context.\n\nWhile we're at it, we can factor out all of the\ncut-and-pastes of the \"you don't exist\" message into a\nwrapper function. Rather than provide xgetpwuid, let's make\nit even more specific to just getting the passwd entry for\nthe current uid. That's the only way we use getpwuid anyway,\nand it lets us make an even more specific error message.\n\nThe current message also fails to mention errno. While the\nusual cause for getpwuid failing is that the user does not\nexist, mentioning errno makes it easier to diagnose these\nproblems. Note that POSIX specifies that errno remain\nuntouched if the passwd entry does not exist (but will be\nset on actual errors), whereas some systems will return\nENOENT or similar for a missing entry. We handle both cases\nin our wrapper.\n\nSigned-off-by: Jeff King \n---\nYou earlier suggested to show a hint to set \"user.name\". That might be\ncomplicated by the fact that this message can come from a remote server.\nOr maybe since that is by far the minority case, we should disregard it\nand show the hint. I left it out of this patch, as it can be trivially\nadded on top due to the refactoring.\n\nI also noticed that the version of getpwuid in compat/mingw.c completely\ndisregards its uid argument. This isn't a problem in the current\ncodebase, since we always feed getuid(). But since the new wrapper is\nexplicitly about getting our _own_ pw entry, it might make more sense to\nconvert our getpwuid() replacement into an xgetpwuid_self() replacement,\nwhich is slightly more accurate. I'll leave that cleanup to Johannes if\nhe cares to do it.\n\n Documentation/git-commit-tree.txt | 5 -----\n Documentation/git-var.txt | 5 -----\n git-compat-util.h | 3 +++\n ident.c | 12 +++---------\n wrapper.c | 12 ++++++++++++\n 5 files changed, 18 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt\nindex eb12b2d..eb8ee99 100644\n--- a/Documentation/git-commit-tree.txt\n+++ b/Documentation/git-commit-tree.txt\n@@ -88,11 +88,6 @@ for one to be entered and terminated with ^D.\n \n include::date-formats.txt[]\n \n-Diagnostics\n------------\n-You don't exist. Go away!::\n- The passwd(5) gecos field couldn't be read\n-\n Discussion\n ----------\n \ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 3f703e3..67edf58 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -59,11 +59,6 @@ ifdef::git-default-pager[]\n The build you are using chose '{git-default-pager}' as the default.\n endif::git-default-pager[]\n \n-Diagnostics\n------------\n-You don't exist. Go away!::\n- The passwd(5) gecos field couldn't be read\n-\n SEE ALSO\n --------\n linkgit:git-commit-tree[1]\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ed11ad8..5bd9ad7 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -595,4 +595,7 @@ int rmdir_or_warn(const char *path);\n */\n int remove_or_warn(unsigned int mode, const char *path);\n \n+/* Get the passwd entry for the UID of the current process. */\n+struct passwd *xgetpwuid_self(void);\n+\n #endif\ndiff --git a/ident.c b/ident.c\nindex c7bdb3f..72944ba 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -101,9 +101,7 @@ static void setup_ident(const char **name, const char **emailp)\n \n \t/* Get the name (\"gecos\") */\n \tif (!*name && !git_default_name.len) {\n-\t\tpw = getpwuid(getuid());\n-\t\tif (!pw)\n-\t\t\tdie(\"You don't exist. Go away!\");\n+\t\tpw = xgetpwuid_self();\n \t\tcopy_gecos(pw, &git_default_name);\n \t}\n \tif (!*name)\n@@ -117,9 +115,7 @@ static void setup_ident(const char **name, const char **emailp)\n \t\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\t} else {\n \t\t\tif (!pw)\n-\t\t\t\tpw = getpwuid(getuid());\n-\t\t\tif (!pw)\n-\t\t\t\tdie(\"You don't exist. Go away!\");\n+\t\t\t\tpw = xgetpwuid_self();\n \t\t\tcopy_email(pw, &git_default_email);\n \t\t}\n \t}\n@@ -300,9 +296,7 @@ const char *fmt_ident(const char *name, const char *email,\n \t\t}\n \t\tif (error_on_no_name)\n \t\t\tdie(\"empty ident %s <%s> not allowed\", name, email);\n-\t\tpw = getpwuid(getuid());\n-\t\tif (!pw)\n-\t\t\tdie(\"You don't exist. Go away!\");\n+\t\tpw = xgetpwuid_self();\n \t\tstrbuf_addstr(&git_default_name, pw->pw_name);\n \t\tname = git_default_name.buf;\n \t}\ndiff --git a/wrapper.c b/wrapper.c\nindex 6ccd059..b5e33e4 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -402,3 +402,15 @@ int remove_or_warn(unsigned int mode, const char *file)\n {\n \treturn S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);\n }\n+\n+struct passwd *xgetpwuid_self(void)\n+{\n+\tstruct passwd *pw;\n+\n+\terrno = 0;\n+\tpw = getpwuid(getuid());\n+\tif (!pw)\n+\t\tdie(_(\"unable to look up current user in the passwd file: %s\"),\n+\t\t errno ? strerror(errno) : _(\"no such user\"));\n+\treturn pw;\n+}\n-- \n1.7.10.2.8.g1101eed\n"},{"id":"191506","messageId":"20120514170533.GA29909@sigill.intra.peff.net","threadId":"30500","inReplyTo":"20120514162824.GA24457@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-14T17:05:33Z","receivedAt":"2012-05-14T17:05:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 14, 2012 at 12:28:24PM -0400, Jeff King wrote:\n\n> I noticed in add_domainname that we look up the host via gethostname,\n> and then if it is not fully qualified, call gethostbyname and steal the\n> domain portion of the result, tacking it onto the hostname we got.\n> \n> That seems oddly complex to me, and like it could result in a bogus\n> hostname if the unqualified name does not match the first part of the\n> returned qualified name. E.g., if the /etc/hosts file contains something\n> like:\n> \n> 192.168.1.1 foo.example.com bar.example.com bar\n> \n> (and your hostname is \"bar\"). I doubt it matters much in practice, and\n> it is outside the scope of this patch, so I left it for now.\n\nIt looks like a bug in adc3dbc (Use sensible domain name (the DNS one)\nwhen guessing ident information, 2005-10-21). Before that we used\ngetdomainname, where that procedure made more sense.\n\nThe patch below fixes it. I doubt it matters much in practice, but I\nthink the resulting code is way less confusing to read.\n\n-- >8 --\nSubject: [PATCH] ident: use full dns names to generate email addresses\n\nWhen we construct an email address from the username and\nhostname, we generate the host part of the email with this\nprocedure:\n\n 1. add the result of gethostname\n\n 2. if it has a dot, ok, it's fully qualified\n\n 3. if not, then look up the unqualified hostname via\n gethostbyname; take the domain name of the result and\n append it to the hostname\n\nStep 3 can actually produce a bogus result, as the name\nreturned by gethostbyname may not be related to the hostname\nwe fed it (e.g., consider a machine \"foo\" with names\n\"foo.one.example.com\" and \"bar.two.example.com\"; we may have\nthe latter returned and generate the bogus name\n\"foo.two.example.com\").\n\nThis patch simply uses the full hostname returned by\ngethostbyname. In the common case that the first part is the\nsame as the unqualified hostname, the behavior is identical.\nAnd in the case that it is not the same, we are much more\nlikely to be generating a valid name.\n\nSigned-off-by: Jeff King \n---\n ident.c | 13 ++++---------\n 1 file changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex 72944ba..e552e7f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -62,23 +62,18 @@ static void add_domainname(struct strbuf *out)\n {\n \tchar buf[1024];\n \tstruct hostent *he;\n-\tconst char *domainname;\n \n \tif (gethostname(buf, sizeof(buf))) {\n \t\twarning(\"cannot get host name: %s\", strerror(errno));\n \t\tstrbuf_addstr(out, \"(none)\");\n \t\treturn;\n \t}\n-\tstrbuf_addstr(out, buf);\n \tif (strchr(buf, '.'))\n-\t\treturn;\n-\n-\the = gethostbyname(buf);\n-\tstrbuf_addch(out, '.');\n-\tif (he && (domainname = strchr(he->h_name, '.')))\n-\t\tstrbuf_addstr(out, domainname + 1);\n+\t\tstrbuf_addstr(out, buf);\n+\telse if ((he = gethostbyname(buf)) && strchr(he->h_name, '.'))\n+\t\tstrbuf_addstr(out, he->h_name);\n \telse\n-\t\tstrbuf_addstr(out, \"(none)\");\n+\t\tstrbuf_addf(out, \"%s.(none)\", buf);\n }\n \n static void copy_email(const struct passwd *pw, struct strbuf *email)\n-- \n1.7.10.2.8.g1101eed\n"},{"id":"191529","messageId":"20120514210225.GA9677@sigill.intra.peff.net","threadId":"30500","inReplyTo":"20120514162824.GA24457@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-14T21:02:25Z","receivedAt":"2012-05-14T21:02:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 14, 2012 at 12:28:24PM -0400, Jeff King wrote:\n\n> When we pull the user's name from the GECOS field of the\n> passwd file (or generate an email address based on their\n> username and hostname), we put the result into a\n> static buffer. While it's extremely unlikely that anybody\n> ever hit these limits (after all, in such a case their\n> parents must have hated them), we still had to deal with the\n> error cases in our code.\n> \n> Converting these static buffers to strbufs lets us simplify\n> the code and drop some error messages from the documentation\n> that have confused some users.\n> \n> Note that there is still one length limitation: the\n> gethostname interface requires us to provide a static\n> buffer, so we arbitrarily choose 1024 bytes for the\n> hostname.\n> \n> Signed-off-by: Jeff King \n\nIck, there is something very wrong with this patch. While testing a\ncompletely unrelated bug, I noticed that it set my name to \"Jeff\nKingJeff KingJeff King\". Which, while a wonderful ego massage, is\nprobably excessive.\n\nI'm sure the problem is the switch to strbuf's appending semantics\nrather than strlcpy's overwriting semantics. I thought we were careful\nnot to bother re-run the gecos code if we had already gotten a name, but\nobviously that is not the case in some code paths. I'll investigate.\n\n-Peff\n"},{"id":"191530","messageId":"20120514211324.GA11578@sigill.intra.peff.net","threadId":"30500","inReplyTo":"20120514210225.GA9677@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-14T21:13:24Z","receivedAt":"2012-05-14T21:13:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 14, 2012 at 05:02:25PM -0400, Jeff King wrote:\n\n> Ick, there is something very wrong with this patch. While testing a\n> completely unrelated bug, I noticed that it set my name to \"Jeff\n> KingJeff KingJeff King\". Which, while a wonderful ego massage, is\n> probably excessive.\n> \n> I'm sure the problem is the switch to strbuf's appending semantics\n> rather than strlcpy's overwriting semantics. I thought we were careful\n> not to bother re-run the gecos code if we had already gotten a name, but\n> obviously that is not the case in some code paths. I'll investigate.\n\nAh, I see. The problem is here:\n\n> --- a/config.c\n> +++ b/config.c\n> @@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)\n> \tif (!strcmp(var, \"user.name\")) {\n> \t\tif (!value)\n> \t\t\treturn config_error_nonbool(var);\n> -\t\tstrlcpy(git_default_name, value, sizeof(git_default_name));\n> +\t\tstrbuf_addstr(&git_default_name, value);\n> \t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n> \t\treturn 0;\n> \t}\n> @@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)\n> \tif (!strcmp(var, \"user.email\")) {\n> \t\tif (!value)\n> \t\t\treturn config_error_nonbool(var);\n> -\t\tstrlcpy(git_default_email, value, sizeof(git_default_email));\n> +\t\tstrbuf_addstr(&git_default_email, value);\n> \t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n\nwhere we are not careful. The fix is trivial. However, while examining\nfmt_ident, I notice there is another potential spot there that needs\nfurther investigation (I think it may actually be unreachable code, but\nI need to look closer).\n\nI'll re-roll the series with the fixes after investigating fmt_ident.\n\n-Peff\n"},{"id":"191536","messageId":"20120515015437.GA13833@sigill.intra.peff.net","threadId":"30500","inReplyTo":"20120514211324.GA11578@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-15T01:54:37Z","receivedAt":"2012-05-15T01:54:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:\n\n> where we are not careful. The fix is trivial. However, while examining\n> fmt_ident, I notice there is another potential spot there that needs\n> further investigation (I think it may actually be unreachable code, but\n> I need to look closer).\n> \n> I'll re-roll the series with the fixes after investigating fmt_ident.\n\nHmm. This code from fmt_ident is very odd:\n\n> const char *fmt_ident(const char *name, const char *email,\n> \t\t const char *date_str, int flag)\n> {\n> [...]\n> \tsetup_ident(&name, &email);\n> \n> \tif (!*name) {\n> \t\tstruct passwd *pw;\n> \n> \t\tif ((warn_on_no_name || error_on_no_name) &&\n> \t\t name == git_default_name && env_hint) {\n> \t\t\tfputs(env_hint, stderr);\n> \t\t\tenv_hint = NULL; /* warn only once */\n> \t\t}\n> \t\tif (error_on_no_name)\n> \t\t\tdie(\"empty ident %s <%s> not allowed\", name, email);\n> \t\tpw = getpwuid(getuid());\n> \t\tif (!pw)\n> \t\t\tdie(\"You don't exist. Go away!\");\n> \t\tstrlcpy(git_default_name, pw->pw_name,\n> \t\t\tsizeof(git_default_name));\n> \t\tname = git_default_name;\n> \t}\n\nWe call setup_ident with our name pointer, which usually comes from\ngetenv(\"GIT_*_NAME\"), although could also come from something like \"git\ncommit -c $commit\". We feed that to setup_ident. If name is NULL, then\nsetup_ident will use git_default_name (filling it in from gecos or\nconfig). If it's not NULL, then we use it literally. And then we check\n_that_ result to see if it's empty. If it is, we either die or warn,\ndepending on the flags. In the latter case, we fallback to using the\nusername as the name.\n\nAnd that's what confuses me. Depending on what was passed in, we may\nhave checked that GIT_COMMITTER_NAME is an empty string, or we may have\nchecked that the config or gecos field yielded an empty string. In the\nlatter case, it makes sense to fall back to the username. But in the\nformer case, it doesn't; we should fall back to the config name or the\ngecos name. And worse, we've polluted git_default_name for the rest of\nthe program run.\n\nInstead of falling back to getpwuid(), should it fall back to:\n\n /* If this wasn't our default name already, then fall back to that. */\n if (name != git_default_name) {\n name = NULL;\n setup_ident(&name, &email);\n }\n\n /* If we _still_ don't have a non-empty name, then fall back to\n * username. */\n if (!*name) {\n pw = getpwuid(getuid());\n if (!pw)\n die(\"You don't exist. Go away!\");\n strlcpy(git_default_name, pw->pw_name, sizeof(git_default_name));\n nae = git_default_name;\n }\n\nOf course we've still polluted this crappy fake name into\ngit_default_name, so that later calls with error_on_no_name will see it\nand not error. I think so far it hasn't mattered because the only user\nof this \"warn\" code is format-patch, which otherwise does not care about\nident (and doesn't even end up using the name at all!). And I doubt this\ncode path gets triggered much anyway; do people really run\n\"GIT_COMMITTER_NAME= git format-patch\"?\n\nI can just leave it as it's not really hurting anybody, I think. But I\nwas refactoring in the area and it just seemed flaky and questionable. I\nwonder if we can simply get rid of the IDENT_WARN_ON_NO_NAME code path\nentirely. The use here is grabbing the email address to use as part of a\nmessage id. Could we just call setup_ident and then read from\ngit_default_email directly? There's no need to respect\nGIT_COMMITTER_EMAIL here at all.\n\n-Peff\n"},{"id":"191537","messageId":"20120515023220.GA22947@sigill.intra.peff.net","threadId":"30500","inReplyTo":"20120515015437.GA13833@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-15T02:32:20Z","receivedAt":"2012-05-15T02:32:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 14, 2012 at 09:54:37PM -0400, Jeff King wrote:\n\n> Of course we've still polluted this crappy fake name into\n> git_default_name, so that later calls with error_on_no_name will see it\n> and not error. I think so far it hasn't mattered because the only user\n> of this \"warn\" code is format-patch, which otherwise does not care about\n> ident (and doesn't even end up using the name at all!). And I doubt this\n> code path gets triggered much anyway; do people really run\n> \"GIT_COMMITTER_NAME= git format-patch\"?\n> \n> I can just leave it as it's not really hurting anybody, I think. But I\n> was refactoring in the area and it just seemed flaky and questionable. I\n> wonder if we can simply get rid of the IDENT_WARN_ON_NO_NAME code path\n> entirely. The use here is grabbing the email address to use as part of a\n> message id. Could we just call setup_ident and then read from\n> git_default_email directly? There's no need to respect\n> GIT_COMMITTER_EMAIL here at all.\n\nHmm, I was mistaken. This code path also gets followed whenever\nIDENT_ERROR_ON_NO_NAME is not set (regardless of IDENT_WARN_ON_NO_NAME).\nSo other programs may accidentally get this pollution of\ngit_default_name and show a username when we _could_ have shown the name\nfrom config. I can see the pollution in a debugger in \"git commit\", but\nI don't think you can actually trigger a commit with it, because later\ncalls to fmt_ident use ERROR_ON_NO_NAME.\n\nI really wonder if we can just get rid of all of the calls which do not\nuse ERROR_ON_NO_NAME. As far as I can tell, they are all part of\nprograms which later end up using ERROR_ON_NO_NAME anyway.\n\n-Peff\n"},{"id":"191550","messageId":"7vtxzhfpv9.fsf@alter.siamese.dyndns.org","threadId":"30500","inReplyTo":"20120515015437.GA13833@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-15T15:03:38Z","receivedAt":"2012-05-15T15:03:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:\n>\n> We call setup_ident with our name pointer, which usually comes from\n> getenv(\"GIT_*_NAME\"), although could also come from something like \"git\n> commit -c $commit\". We feed that to setup_ident. If name is NULL, then\n> setup_ident will use git_default_name (filling it in from gecos or\n> config). If it's not NULL, then we use it literally. And then we check\n> _that_ result to see if it's empty. If it is, we either die or warn,\n> depending on the flags. In the latter case, we fallback to using the\n> username as the name.\n>\n> And that's what confuses me. Depending on what was passed in, we may\n> have checked that GIT_COMMITTER_NAME is an empty string, or we may have\n> checked that the config or gecos field yielded an empty string. \n\nSounds quite sensible to me, though.\n\n> In the\n> latter case, it makes sense to fall back to the username.\n\nI agree that we should use something like \"Sorry, Mr. McDonald\" codepath\nwhen the GECOS field returns an empty string---after all that is what we\ndo when we are built with NO_GECOS_IN_PWENT.\n\n> But in the\n> former case, it doesn't; we should fall back to the config name or the\n> gecos name.\n\nIf the user said GIT_COMMITTER_NAME is empty with \"GIT_COMMITTER_NAME=\",\nthat is different from saying with \"unset GIT_COMMITTER_NAME\" that the\nuser does not want the environment to take effect, no? So I do not think\nfalling back to configured or gecos in the former case is the right thing\nto do, even though that would mean explicitly giving an empty string in\nthat configuration variable is asking only for an error without any\nrecourse, which is not useful at all.\n"},{"id":"191561","messageId":"20120515174724.GA329@sigill.intra.peff.net","threadId":"30500","inReplyTo":"7vtxzhfpv9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-15T17:47:24Z","receivedAt":"2012-05-15T17:47:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 15, 2012 at 08:03:38AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:\n> >\n> > We call setup_ident with our name pointer, which usually comes from\n> > getenv(\"GIT_*_NAME\"), although could also come from something like \"git\n> > commit -c $commit\". We feed that to setup_ident. If name is NULL, then\n> > setup_ident will use git_default_name (filling it in from gecos or\n> > config). If it's not NULL, then we use it literally. And then we check\n> > _that_ result to see if it's empty. If it is, we either die or warn,\n> > depending on the flags. In the latter case, we fallback to using the\n> > username as the name.\n> >\n> > And that's what confuses me. Depending on what was passed in, we may\n> > have checked that GIT_COMMITTER_NAME is an empty string, or we may have\n> > checked that the config or gecos field yielded an empty string. \n> \n> Sounds quite sensible to me, though.\n\nYes, I think it is OK to check what was given to us (or our fallback).\nBut using that check to decide which next step to take doesn't seem\nright.\n\n> > In the\n> > latter case, it makes sense to fall back to the username.\n> \n> I agree that we should use something like \"Sorry, Mr. McDonald\" codepath\n> when the GECOS field returns an empty string---after all that is what we\n> do when we are built with NO_GECOS_IN_PWENT.\n\nRight, and that is more or less what we do (just without the\ncapitalization).\n\n> > But in the\n> > former case, it doesn't; we should fall back to the config name or the\n> > gecos name.\n> \n> If the user said GIT_COMMITTER_NAME is empty with \"GIT_COMMITTER_NAME=\",\n> that is different from saying with \"unset GIT_COMMITTER_NAME\" that the\n> user does not want the environment to take effect, no?\n\nI agree two the cases are different. And for the most part, you are\ninsane to pass an empty GIT_COMMITTER_NAME. But if you do, why would the\nright behavior be to fall back to sticking the username into the name\nfield, and not the gecos name?\n\nPart of me is wondering why we should fall back at all in that case. If\na caller does not pass ERROR_ON_NO_NAME, then they don't really care\nwhat the name is, do they? The current callers that do not pass it are:\n\n - blame.c:fake_working_tree_commit, which is passing in a fake name\n buffer anyway (so will never trigger this code path)\n\n - log.c:gen_message_id, which only cares about the email\n portion anyway\n\n - fmt-merge-msg.c:credit_people; this caller compares the name field\n to what's in the commits, checking for differences. So it could just\n as easily be \"(none)\" or some other token\n\n - commit.c:prepare_to_commit; this compares and shows author and\n commiter ids, and does not care about a blank name for the committer\n (but does for the author). The commit can't go through anyway with a\n blank committer name, so should it not just use ERROR_ON_NO_NAME?\n\n - log.c:make_cover_letter; this uses the committer information to make\n a fake commit that we ultimately use just to get the \"%f\" pretty\n userformat from it. In other words, we don't care about the\n committer at all, and this is really just working around an\n absolutely horrific interface.\n\n - refs.c:log_ref_write; finally, a caller who actually cares about the\n name, but doesn't want to die if we don't have a good name. We are\n happy enough with the username, though if somebody passes\n GIT_COMMITTER_NAME=, wouldn't it be OK to fail?\n\nSo it seems to me like a much simpler set of rules would be:\n\n 1. When reading gecos, always fall back to the username if the gecos\n field is unavailable or blank.\n\n 2. Always die when the name field is blank. That means we will die\n when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty\n config name), which makes a lot more sense to me than falling back;\n those are bogus requests, not system config problems. And we won't\n ever have a blank gecos name, because we'll always fall back on the\n username.\n\nAgain, I'm sorry to belabor this, and we can just drop it; I don't think\nthere's currently a bug. It's just that I'm cleaning up in the area, and\nthe current behavior seems overly complex; in particular, I'm worried\nthat writing the username into the git_default_name field (overwriting\nthe _real_ name the user gave us!) is a maintenance time-bomb that will\nbite us later.\n\nIf I'm not being clear, I can express it in the form of patches, which\nmight be more obvious.\n\n-Peff\n"},{"id":"191562","messageId":"7vsjf1e2n7.fsf@alter.siamese.dyndns.org","threadId":"30500","inReplyTo":"20120515174724.GA329@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-15T18:10:36Z","receivedAt":"2012-05-15T18:10:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So it seems to me like a much simpler set of rules would be:\n>\n> 1. When reading gecos, always fall back to the username if the gecos\n> field is unavailable or blank.\n>\n> 2. Always die when the name field is blank. That means we will die\n> when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty\n> config name), which makes a lot more sense to me than falling back;\n> those are bogus requests, not system config problems. And we won't\n> ever have a blank gecos name, because we'll always fall back on the\n> username.\n\nThat certainly sounds very simple to explain and understand, and I do not\noffhand think of anything *sane* that would break ;-)\n\nThanks.\n"}]}