threads / patch / 30561

patch, 13 partsident: split setup_ident into separate functions

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

## tl;dr

45 messages between May 18, 2012 and Jan 25, 2013. Diffs are folded; open one to read it.

replies: 44people: 3as markdown or json

Jeff King· May 18, 2012, 23:05 UTC · lore

[PATCH 0/13] ident cleanups and bugfixes

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

Show 16 quoted lines

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

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

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

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

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

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

-Peff

Jeff King· May 18, 2012, 23:07 UTC · re: Jeff King · lore

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

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

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

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

Show changes to 2 files +22 −16

cache.h, ident.c

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

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:09 UTC · re: Jeff King · lore

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

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

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

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

Show changes to http-push.c +1 −1

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

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:10 UTC · re: Jeff King · lore

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

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

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

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

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

Show changes to builtin/fmt-merge-msg.c +4 −4

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

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:11 UTC · re: Jeff King · lore

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

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

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

Show changes to 3 files +23 −23

cache.h, config.c, ident.c

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

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:11 UTC · re: Jeff King · lore

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

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

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

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

Show changes to 3 files +4 −6

cache.h, environment.c, ident.c

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

-- 
1.7.10.1.16.g53a707b

Junio C Hamano· May 21, 2012, 04:07 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

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

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

Jeff King· May 21, 2012, 05:41 UTC · re: Junio C Hamano · lore

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

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

Show 19 quoted lines

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

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

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

-Peff

Jeff King· May 21, 2012, 06:41 UTC · re: Jeff King · lore

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

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

Show 17 quoted lines

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

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

-Peff

Jeff King· May 18, 2012, 23:13 UTC · re: Jeff King · lore

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

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

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

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

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

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

Show changes to builtin/log.c +2 −8

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

-- 
1.7.10.1.16.g53a707b

Junio C Hamano· May 21, 2012, 02:58 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

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

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

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

Jeff King· May 21, 2012, 06:36 UTC · re: Junio C Hamano · lore

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

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

Show 10 quoted lines

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

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

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

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

Show 8 quoted lines

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

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

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

-Peff

Jeff King· May 18, 2012, 23:14 UTC · re: Jeff King · lore

[PATCH 07/13] fmt_ident: drop IDENT_WARN_ON_NO_NAME code

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

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

Show changes to 2 files +6 −10

cache.h, ident.c

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

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:19 UTC · re: Jeff King · lore

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

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

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

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

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

       git_committer_info(IDENT_ERROR_ON_NO_NAME);

     but not this:

       git_committer_info(0);
       git_committer_info(IDENT_ERROR_ON_NO_NAME);

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

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

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

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

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

By the way, I left it so that:

  GIT_COMMITTER_NAME= git ...

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

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

Show changes to ident.c +1 −3

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

-- 
1.7.10.1.16.g53a707b

Junio C Hamano· May 21, 2012, 02:54 UTC · re: Jeff King · lore

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

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

Jeff King· May 21, 2012, 06:31 UTC · re: Junio C Hamano · lore

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

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

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

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

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

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

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

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

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

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

So I think my preference would be:

  - apply the patch below as 5.5/13

  - tweak patch 9 to remove the extra trimming

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

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

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

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

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

Show changes to ident.c +3 −0

diff --git a/ident.c b/ident.c
index af92b2c..e797e36 100644
--- a/ident.c
+++ b/ident.c
@@ -57,6 +57,7 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz)
 static int add_mailname_host(char *buf, size_t len)
 {
 	FILE *mailname;
+	char *p;
 
 	mailname = fopen("/etc/mailname", "r");
 	if (!mailname) {
@@ -74,6 +75,8 @@ static int add_mailname_host(char *buf, size_t len)
 	}
 	/* success! */
 	fclose(mailname);
+	for (p = buf + strlen(buf) - 1; p >= buf && isspace(*p); p--)
+		*p = '\0';
 	return 0;
 }

-- 
1.7.10.1.19.g711d603

Junio C Hamano· May 21, 2012, 09:11 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

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

Sounds sensible; thanks.

Jeff King· May 21, 2012, 23:09 UTC · re: Junio C Hamano · lore

[PATCHv2 0/15] ident cleanups git_default_name

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

Show 10 quoted lines

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

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

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

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

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

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

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

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

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

-Peff

Jeff King· May 21, 2012, 23:09 UTC · re: Jeff King · lore

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

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

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

Show changes to 2 files +22 −16

cache.h, ident.c

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:09 UTC · re: Jeff King · lore

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

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

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

Show changes to http-push.c +1 −1

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:09 UTC · re: Jeff King · lore

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

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

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

Show changes to builtin/fmt-merge-msg.c +4 −4

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:09 UTC · re: Jeff King · lore

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

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

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

Show changes to 3 files +23 −23

cache.h, config.c, ident.c

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:09 UTC · re: Jeff King · lore

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

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

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

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

Show changes to 3 files +4 −6

cache.h, environment.c, ident.c

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

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

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

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

Show changes to ident.c +4 −0

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

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

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

Show changes to builtin/log.c +2 −8

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

[PATCHv2 08/15] fmt_ident: drop IDENT_WARN_ON_NO_NAME code

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

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

Show changes to 2 files +6 −10

cache.h, ident.c

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

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

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

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

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

       git_committer_info(IDENT_ERROR_ON_NO_NAME);

     but not this:

       git_committer_info(0);
       git_committer_info(IDENT_ERROR_ON_NO_NAME);

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

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

Show changes to ident.c +1 −3

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

[PATCHv2 10/15] 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.

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

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

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

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

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

Show changes to 3 files +39 −73

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

-- 
1.7.10.1.19.g711d603

Jonathan Nieder· Jan 24, 2013, 23:21 UTC · re: Jeff King · lore

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

Hi Jeff,

Jeff King wrote:

> When we pull the user's name from the GECOS field of the
> passwd file (or generate an email address based on their
> username and hostname), we put the result into a
> static buffer.

[...]

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

This broke /etc/mailname handling.  Before:

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

After:

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

The cause:

[...]

> --- a/ident.c
> +++ b/ident.c
> @@ -19,42 +18,27 @@ int user_ident_explicitly_given;

[...]

Show 11 quoted lines

> -static int add_mailname_host(char *buf, size_t len)
> +static int add_mailname_host(struct strbuf *buf)
>  {
>  	FILE *mailname;
>  
> @@ -65,7 +49,7 @@ static int add_mailname_host(char *buf, size_t len)
>  				strerror(errno));
>  		return -1;
>  	}
> -	if (!fgets(buf, len, mailname)) {
> +	if (strbuf_getline(buf, mailname, '\n') == EOF) {

This clears the strbuf.

How about something like this as a quick fix?

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

Show changes to ident.c +5 −1

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

Jeff King· Jan 25, 2013, 01:05 UTC · re: Jonathan Nieder · lore

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

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

Show 9 quoted lines

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

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

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

Right. Definitely the problem.

Show 36 quoted lines

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

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

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

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

(with a proper commit message, of course).

-Peff

Junio C Hamano· Jan 25, 2013, 18:46 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

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

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

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

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

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

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

Show changes to ident.c +5 −1

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

-- 
1.8.1.1.525.gdace530

Jeff King· Jan 25, 2013, 22:10 UTC · re: Junio C Hamano · lore

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

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

Show 6 quoted lines

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

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

Show 9 quoted lines

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

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

-Peff

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

[PATCHv2 11/15] 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>
---
 Documentation/git-commit-tree.txt |  5 -----
 Documentation/git-var.txt         |  5 -----
 git-compat-util.h                 |  3 +++
 ident.c                           | 20 +++++---------------
 wrapper.c                         | 12 ++++++++++++
 5 files changed, 20 insertions(+), 25 deletions(-)

Show changes to 5 files +20 −23

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 73a06a1..5aec073 100644
--- a/ident.c
+++ b/ident.c
@@ -100,12 +100,8 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
 
 const char *ident_default_name(void)
 {
-	if (!git_default_name.len) {
-		struct passwd *pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
-		copy_gecos(pw, &git_default_name);
-	}
+	if (!git_default_name.len)
+		copy_gecos(xgetpwuid_self(), &git_default_name);
 	return git_default_name.buf;
 }
 
@@ -117,12 +113,8 @@ const char *ident_default_email(void)
 		if (email && email[0]) {
 			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		} else {
-			struct passwd *pw = getpwuid(getuid());
-			if (!pw)
-				die("You don't exist. Go away!");
-			copy_email(pw, &git_default_email);
-		}
+		} else
+			copy_email(xgetpwuid_self(), &git_default_email);
 	}
 	return git_default_email.buf;
 }
@@ -303,9 +295,7 @@ const char *fmt_ident(const char *name, const char *email,
 				fputs(env_hint, stderr);
 			die("empty ident %s <%s> not allowed", name, email);
 		}
-		pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
+		pw = xgetpwuid_self();
 		name = pw->pw_name;
 	}
 
diff --git a/wrapper.c b/wrapper.c
index 6ccd059..b5e33e4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -402,3 +402,15 @@ int remove_or_warn(unsigned int mode, const char *file)
 {
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
+
+struct passwd *xgetpwuid_self(void)
+{
+	struct passwd *pw;
+
+	errno = 0;
+	pw = getpwuid(getuid());
+	if (!pw)
+		die(_("unable to look up current user in the passwd file: %s"),
+		    errno ? strerror(errno) : _("no such user"));
+	return pw;
+}

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

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

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

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

Show changes to ident.c +15 −28

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

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

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

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

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

Show changes to ident.c +4 −1

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

-- 
1.7.10.1.19.g711d603

Junio C Hamano· May 22, 2012, 16:55 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

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

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

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

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

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

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

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

Show 30 quoted lines

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

Jeff King· May 22, 2012, 17:12 UTC · re: Junio C Hamano · lore

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

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

Show 8 quoted lines

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

Yes, exactly.

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

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

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

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

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

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

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

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

Show 6 quoted lines

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

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

-Peff

Junio C Hamano· May 22, 2012, 17:21 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

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

Thanks.

Show 6 quoted lines

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

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

Jeff King· May 21, 2012, 23:10 UTC · re: Jeff King · lore

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

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

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

Show changes to 3 files +20 −38

builtin/log.c, log-tree.c, log-tree.h

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

-- 
1.7.10.1.19.g711d603

Jeff King· May 18, 2012, 23:20 UTC · re: Jeff King · lore

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

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

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

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

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

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

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

Show changes to 3 files +39 −69

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

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:21 UTC · re: Jeff King · lore

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

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

Show changes to 5 files +20 −23

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 73a06a1..5aec073 100644
--- a/ident.c
+++ b/ident.c
@@ -100,12 +100,8 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
 
 const char *ident_default_name(void)
 {
-	if (!git_default_name.len) {
-		struct passwd *pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
-		copy_gecos(pw, &git_default_name);
-	}
+	if (!git_default_name.len)
+		copy_gecos(xgetpwuid_self(), &git_default_name);
 	return git_default_name.buf;
 }
 
@@ -117,12 +113,8 @@ const char *ident_default_email(void)
 		if (email && email[0]) {
 			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		} else {
-			struct passwd *pw = getpwuid(getuid());
-			if (!pw)
-				die("You don't exist. Go away!");
-			copy_email(pw, &git_default_email);
-		}
+		} else
+			copy_email(xgetpwuid_self(), &git_default_email);
 	}
 	return git_default_email.buf;
 }
@@ -303,9 +295,7 @@ const char *fmt_ident(const char *name, const char *email,
 				fputs(env_hint, stderr);
 			die("empty ident %s <%s> not allowed", name, email);
 		}
-		pw = getpwuid(getuid());
-		if (!pw)
-			die("You don't exist. Go away!");
+		pw = xgetpwuid_self();
 		name = pw->pw_name;
 	}
 
diff --git a/wrapper.c b/wrapper.c
index 6ccd059..b5e33e4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -402,3 +402,15 @@ int remove_or_warn(unsigned int mode, const char *file)
 {
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
+
+struct passwd *xgetpwuid_self(void)
+{
+	struct passwd *pw;
+
+	errno = 0;
+	pw = getpwuid(getuid());
+	if (!pw)
+		die(_("unable to look up current user in the passwd file: %s"),
+		    errno ? strerror(errno) : _("no such user"));
+	return pw;
+}

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:22 UTC · re: Jeff King · lore

[PATCH 11/13] 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

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

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

Show changes to ident.c +4 −9

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

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:23 UTC · re: Jeff King · lore

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

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

Show changes to ident.c +15 −28

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

-- 
1.7.10.1.16.g53a707b

Jeff King· May 18, 2012, 23:24 UTC · re: Jeff King · lore

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

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

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

Show changes to 3 files +20 −38

builtin/log.c, log-tree.c, log-tree.h

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

-- 
1.7.10.1.16.g53a707b