committer-date-is-author-date flag removes email in "Commit"

committer-date-is-author-date flag removes email in "Commit"

[VenomVendor]

committer-date-is-author-date flag removes email in "Commit"


What did you do before the bug happened? (Steps to reproduce your issue)
* Create empty repo using `git init`
* Make few commits, at least two
* execute `git log --format=fuller`
* Notice the log, with "Author", "AuthorDate", "Commit", "CommitDate"
* Note, "Commit"
* execute `git rebase --committer-date-is-author-date HEAD~1`
* execute `git log --format=fuller`
* Note, email from "Commit" is empty <>


What did you expect to happen? (Expected behavior)
* Email must not be removed.

What happened instead? (Actual behavior)
* email got removed/replaced with empty `< >`

What's different between what you expected and what actually happened?

Expected:
```
Author:     VenomVendor <info@VenomVendor.com>
AuthorDate: Thu Oct 22 19:41:05 2020 -0200
Commit:     VenomVendor <info@VenomVendor.com>
CommitDate: Thu Oct 22 19:41:05 2020 -0200
```

Happened (Missing email in Commit)
```
Author:     VenomVendor <info@VenomVendor.com>
AuthorDate: Thu Oct 22 19:41:05 2020 -0200
Commit:     VenomVendor <>
CommitDate: Thu Oct 22 19:41:05 2020 -0200
```

Anything else you want to add:

```
$ cd ~

$ mkdir committer-date-is-author-date-bug

$ cd committer-date-is-author-date-bug

$ git --version
git version 2.29.0

$ git init
Initialized empty Git repository in 
~/committer-date-is-author-date-bug/.git/

$ git log --format=fuller
fatal: your current branch 'master' does not have any commits yet


$ TEXT=1; echo "${TEXT}" > sample.txt && git add . && git commit -m 
"${TEXT}"
[master (root-commit) 20731cb] 1
  1 file changed, 1 insertion(+)
  create mode 100644 sample.txt


$ TEXT=2; echo "${TEXT}" > sample.txt && git add . && git commit -m 
"${TEXT}"
[master e3d8ce8] 2
  1 file changed, 1 insertion(+), 1 deletion(-)


$ TEXT=3; echo "${TEXT}" > sample.txt && git add . && git commit -m 
"${TEXT}"
[master 30f2b84] 3
  1 file changed, 1 insertion(+), 1 deletion(-)


$ git log --format=fuller
commit 30f2b84e360d2ea79fe3355b5fbfa5cdb401c65f (HEAD -> master)
Author:     VenomVendor <info@VenomVendor.com>
AuthorDate: Thu Oct 22 19:41:05 2020 -0200
Commit:     VenomVendor <info@VenomVendor.com>
CommitDate: Thu Oct 22 19:41:05 2020 -0200

     3

commit e3d8ce88ea25b14cf2840ccd41cebc8f68663b8e
Author:     VenomVendor <info@VenomVendor.com>
AuthorDate: Thu Oct 22 19:41:02 2020 -0200
Commit:     VenomVendor <info@VenomVendor.com>
CommitDate: Thu Oct 22 19:41:02 2020 -0200

     2

commit 20731cb051e42dd5435dec4157aa58203e5fc60d
Author:     VenomVendor <info@VenomVendor.com>
AuthorDate: Thu Oct 22 19:40:59 2020 -0200
Commit:     VenomVendor <info@VenomVendor.com>
CommitDate: Thu Oct 22 19:40:59 2020 -0200

     1


$ git rebase --committer-date-is-author-date HEAD~2
Current branch master is up to date, rebase forced.
Successfully rebased and updated refs/heads/master.


$ git log --format=fuller
commit be24ee5cd9b76b0f11599b4f8af2609f6c5f3b06 (HEAD -> master)
Author:     VenomVendor <info@VenomVendor.com>
AuthorDate: Thu Oct 22 19:41:05 2020 -0200
Commit:     VenomVendor <>
CommitDate: Thu Oct 22 19:41:05 2020 -0200

     3

commit c45d01b6969281d11fc78f60cb0c4d830f216892
Author:     VenomVendor <info@VenomVendor.com>
AuthorDate: Thu Oct 22 19:41:02 2020 -0200
Commit:     VenomVendor <>
CommitDate: Thu Oct 22 19:41:02 2020 -0200

     2

commit 20731cb051e42dd5435dec4157aa58203e5fc60d
Author:     VenomVendor <info@VenomVendor.com>
AuthorDate: Thu Oct 22 19:40:59 2020 -0200
Commit:     VenomVendor <info@VenomVendor.com>
CommitDate: Thu Oct 22 19:40:59 2020 -0200

     1
```

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.29.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 
PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64
compiler info: clang: 12.0.0 (clang-1200.0.32.2)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]

-VenomVendor

Re: committer-date-is-author-date flag removes email in "Commit"

[Jeff King]

On Fri, Oct 23, 2020 at 11:18:51AM +0530, VenomVendor wrote:

> What did you do before the bug happened? (Steps to reproduce your issue)
> * Create empty repo using `git init`
> * Make few commits, at least two
> * execute `git log --format=fuller`
> * Notice the log, with "Author", "AuthorDate", "Commit", "CommitDate"
> * Note, "Commit"
> * execute `git rebase --committer-date-is-author-date HEAD~1`
> * execute `git log --format=fuller`
> * Note, email from "Commit" is empty <>

Thanks for a clear report. I was able to easily reproduce the problem.
There are actually two related bugs here, and they're both regressions
in v2.29.0.

  [1/3]: t3436: check --committer-date-is-author-date result more carefully
  [2/3]: am: fix broken email with --committer-date-is-author-date
  [3/3]: rebase: fix broken email with --committer-date-is-author-date

 builtin/am.c                   | 4 ++--
 sequencer.c                    | 2 +-
 t/t3436-rebase-more-options.sh | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

-Peff

[PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully

[Jeff King]

After running "rebase --committer-date-is-author-date", we confirm that
the committer date is the same as the author date. However, we don't
look at any other parts of the committer ident line to make sure we
didn't screw them up. And indeed, there are a few bugs here. Depending
on the rebase backend in use, we may accidentally use the author email
instead of the committer's, or even an empty string.

Let's teach our test_ctime_is_atime helper to check the committer name
and email, which reveals several failing tests.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3436-rebase-more-options.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 996e82787e..6f2f49717b 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -65,31 +65,31 @@ test_expect_success '--ignore-whitespace is remembered when continuing' '
 '
 
 test_ctime_is_atime () {
-	git log $1 --format=%ai >authortime &&
-	git log $1 --format=%ci >committertime &&
+	git log $1 --format="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> %ai" >authortime &&
+	git log $1 --format="%cn <%ce> %ci" >committertime &&
 	test_cmp authortime committertime
 }
 
-test_expect_success '--committer-date-is-author-date works with apply backend' '
+test_expect_failure '--committer-date-is-author-date works with apply backend' '
 	GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author &&
 	git rebase --apply --committer-date-is-author-date HEAD^ &&
 	test_ctime_is_atime -1
 '
 
-test_expect_success '--committer-date-is-author-date works with merge backend' '
+test_expect_failure '--committer-date-is-author-date works with merge backend' '
 	GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author &&
 	git rebase -m --committer-date-is-author-date HEAD^ &&
 	test_ctime_is_atime -1
 '
 
-test_expect_success '--committer-date-is-author-date works with rebase -r' '
+test_expect_failure '--committer-date-is-author-date works with rebase -r' '
 	git checkout side &&
 	GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
 	git rebase -r --root --committer-date-is-author-date &&
 	test_ctime_is_atime
 '
 
-test_expect_success '--committer-date-is-author-date works when forking merge' '
+test_expect_failure '--committer-date-is-author-date works when forking merge' '
 	git checkout side &&
 	GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
 	PATH="./test-bin:$PATH" git rebase -r --root --strategy=test \
@@ -145,7 +145,7 @@ test_expect_success '--reset-author-date works with rebase -r' '
 	test_atime_is_ignored
 '
 
-test_expect_success '--reset-author-date with --committer-date-is-author-date works' '
+test_expect_failure '--reset-author-date with --committer-date-is-author-date works' '
 	test_must_fail git rebase -m --committer-date-is-author-date \
 		--reset-author-date --onto commit2^^ commit2^ commit3 &&
 	git checkout --theirs foo &&
-- 
2.29.0.583.g8e3ac41d8f

[PATCH 2/3] am: fix broken email with --committer-date-is-author-date

[Jeff King]

Commit e8cbe2118a (am: stop exporting GIT_COMMITTER_DATE, 2020-08-17)
rewrote the code for setting the committer date to use fmt_ident(),
rather than setting an environment variable and letting commit_tree()
handle it. But it introduced two bugs:

  - we use the author email string instead of the committer email

  - when parsing the committer ident, we used the wrong variable to
    compute the length of the email, resulting in it always being a
    zero-length string

This commit fixes both, which causes our test of this option via the
rebase "apply" backend to now succeed.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c                   | 4 ++--
 t/t3436-rebase-more-options.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 2c7673f74e..4949535a7f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -161,7 +161,7 @@ static void am_state_init(struct am_state *state)
 	state->committer_name =
 		xmemdupz(id.name_begin, id.name_end - id.name_begin);
 	state->committer_email =
-		xmemdupz(id.mail_begin, id.mail_end - id.mail_end);
+		xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
 }
 
 /**
@@ -1595,7 +1595,7 @@ static void do_commit(const struct am_state *state)
 
 	if (state->committer_date_is_author_date)
 		committer = fmt_ident(state->committer_name,
-				      state->author_email, WANT_COMMITTER_IDENT,
+				      state->committer_email, WANT_COMMITTER_IDENT,
 				      state->ignore_date ? NULL
 							 : state->author_date,
 				      IDENT_STRICT);
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 6f2f49717b..3fda2235bd 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -70,7 +70,7 @@ test_ctime_is_atime () {
 	test_cmp authortime committertime
 }
 
-test_expect_failure '--committer-date-is-author-date works with apply backend' '
+test_expect_success '--committer-date-is-author-date works with apply backend' '
 	GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author &&
 	git rebase --apply --committer-date-is-author-date HEAD^ &&
 	test_ctime_is_atime -1
-- 
2.29.0.583.g8e3ac41d8f

[PATCH 3/3] rebase: fix broken email with --committer-date-is-author-date

[Jeff King]

Commit 7573cec52c (rebase -i: support --committer-date-is-author-date,
2020-08-17) copied the committer ident-parsing code from builtin/am.c.
And in doing so, it copied a bug in which we always set the email to an
empty string. We fixed the version in git-am in the previous commit;
this commit fixes the copied code.

Reported-by: VenomVendor <info@venomvendor.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c                    | 2 +-
 t/t3436-rebase-more-options.sh | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 00acb12496..d76cbded00 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4478,7 +4478,7 @@ static int init_committer(struct replay_opts *opts)
 	opts->committer_name =
 		xmemdupz(id.name_begin, id.name_end - id.name_begin);
 	opts->committer_email =
-		xmemdupz(id.mail_begin, id.mail_end - id.mail_end);
+		xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
 
 	return 0;
 }
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 3fda2235bd..eaaf4c8d1d 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -76,20 +76,20 @@ test_expect_success '--committer-date-is-author-date works with apply backend' '
 	test_ctime_is_atime -1
 '
 
-test_expect_failure '--committer-date-is-author-date works with merge backend' '
+test_expect_success '--committer-date-is-author-date works with merge backend' '
 	GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author &&
 	git rebase -m --committer-date-is-author-date HEAD^ &&
 	test_ctime_is_atime -1
 '
 
-test_expect_failure '--committer-date-is-author-date works with rebase -r' '
+test_expect_success '--committer-date-is-author-date works with rebase -r' '
 	git checkout side &&
 	GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
 	git rebase -r --root --committer-date-is-author-date &&
 	test_ctime_is_atime
 '
 
-test_expect_failure '--committer-date-is-author-date works when forking merge' '
+test_expect_success '--committer-date-is-author-date works when forking merge' '
 	git checkout side &&
 	GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
 	PATH="./test-bin:$PATH" git rebase -r --root --strategy=test \
@@ -145,7 +145,7 @@ test_expect_success '--reset-author-date works with rebase -r' '
 	test_atime_is_ignored
 '
 
-test_expect_failure '--reset-author-date with --committer-date-is-author-date works' '
+test_expect_success '--reset-author-date with --committer-date-is-author-date works' '
 	test_must_fail git rebase -m --committer-date-is-author-date \
 		--reset-author-date --onto commit2^^ commit2^ commit3 &&
 	git checkout --theirs foo &&
-- 
2.29.0.583.g8e3ac41d8f

[PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Jeff King]

On Fri, Oct 23, 2020 at 03:09:39AM -0400, Jeff King wrote:

> Commit e8cbe2118a (am: stop exporting GIT_COMMITTER_DATE, 2020-08-17)
> rewrote the code for setting the committer date to use fmt_ident(),
> rather than setting an environment variable and letting commit_tree()
> handle it. But it introduced two bugs:
> 
>   - we use the author email string instead of the committer email
> 
>   - when parsing the committer ident, we used the wrong variable to
>     compute the length of the email, resulting in it always being a
>     zero-length string

By the way, I wondered why we needed to do this parsing at all. The
patch below does this in a much simpler way. It's a little bit ugly, I
think, because we have to call getenv() ourselves. But that's the way
fmt_ident() has always worked. We could probably improve that now that
it takes a whose_ident flag (before that, it had no idea if we wanted
author or committer ident).

This is on top of the fixes (but we'd perhaps just want to do those on
'maint' as the minimal fix).

-- >8 --
Subject: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

For the --committer-date-is-author-date option of git-am and git-rebase,
we format the committer ident, then re-parse it to find the name and
email, and then feed those back to fmt_ident().

We can simplify this by handling it all at the time of the fmt_ident()
call. We pass in the appropriate getenv() results, and if they're not
present, then our WANT_COMMITTER_IDENT flag tells fmt_ident() to fill in
the appropriate value from the config. Which is exactly what
git_committer_ident() was doing under the hood.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c | 19 +++----------------
 sequencer.c  | 28 ++--------------------------
 sequencer.h  |  2 --
 3 files changed, 5 insertions(+), 44 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4949535a7f..52206bc56b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -98,8 +98,6 @@ struct am_state {
 	char *author_name;
 	char *author_email;
 	char *author_date;
-	char *committer_name;
-	char *committer_email;
 	char *msg;
 	size_t msg_len;
 
@@ -132,8 +130,6 @@ struct am_state {
  */
 static void am_state_init(struct am_state *state)
 {
-	const char *committer;
-	struct ident_split id;
 	int gpgsign;
 
 	memset(state, 0, sizeof(*state));
@@ -154,14 +150,6 @@ static void am_state_init(struct am_state *state)
 
 	if (!git_config_get_bool("commit.gpgsign", &gpgsign))
 		state->sign_commit = gpgsign ? "" : NULL;
-
-	committer = git_committer_info(IDENT_STRICT);
-	if (split_ident_line(&id, committer, strlen(committer)) < 0)
-		die(_("invalid committer: %s"), committer);
-	state->committer_name =
-		xmemdupz(id.name_begin, id.name_end - id.name_begin);
-	state->committer_email =
-		xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
 }
 
 /**
@@ -173,8 +161,6 @@ static void am_state_release(struct am_state *state)
 	free(state->author_name);
 	free(state->author_email);
 	free(state->author_date);
-	free(state->committer_name);
-	free(state->committer_email);
 	free(state->msg);
 	strvec_clear(&state->git_apply_opts);
 }
@@ -1594,8 +1580,9 @@ static void do_commit(const struct am_state *state)
 			IDENT_STRICT);
 
 	if (state->committer_date_is_author_date)
-		committer = fmt_ident(state->committer_name,
-				      state->committer_email, WANT_COMMITTER_IDENT,
+		committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
+				      getenv("GIT_COMMITTER_EMAIL"),
+				      WANT_COMMITTER_IDENT,
 				      state->ignore_date ? NULL
 							 : state->author_date,
 				      IDENT_STRICT);
diff --git a/sequencer.c b/sequencer.c
index d76cbded00..07321a7d95 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -314,8 +314,6 @@ int sequencer_remove_state(struct replay_opts *opts)
 		}
 	}
 
-	free(opts->committer_name);
-	free(opts->committer_email);
 	free(opts->gpg_sign);
 	free(opts->strategy);
 	for (i = 0; i < opts->xopts_nr; i++)
@@ -1460,8 +1458,8 @@ static int try_to_commit(struct repository *r,
 		} else {
 			reset_ident_date();
 		}
-		committer = fmt_ident(opts->committer_name,
-				      opts->committer_email,
+		committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
+				      getenv("GIT_COMMITTER_EMAIL"),
 				      WANT_COMMITTER_IDENT,
 				      opts->ignore_date ? NULL : date.buf,
 				      IDENT_STRICT);
@@ -4467,22 +4465,6 @@ static int commit_staged_changes(struct repository *r,
 	return 0;
 }
 
-static int init_committer(struct replay_opts *opts)
-{
-	struct ident_split id;
-	const char *committer;
-
-	committer = git_committer_info(IDENT_STRICT);
-	if (split_ident_line(&id, committer, strlen(committer)) < 0)
-		return error(_("invalid committer '%s'"), committer);
-	opts->committer_name =
-		xmemdupz(id.name_begin, id.name_end - id.name_begin);
-	opts->committer_email =
-		xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
-
-	return 0;
-}
-
 int sequencer_continue(struct repository *r, struct replay_opts *opts)
 {
 	struct todo_list todo_list = TODO_LIST_INIT;
@@ -4494,9 +4476,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (read_populate_opts(opts))
 		return -1;
 	if (is_rebase_i(opts)) {
-		if (opts->committer_date_is_author_date && init_committer(opts))
-			return -1;
-
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
 
@@ -5391,9 +5370,6 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 
 	res = -1;
 
-	if (opts->committer_date_is_author_date && init_committer(opts))
-		goto cleanup;
-
 	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
 		goto cleanup;
 
diff --git a/sequencer.h b/sequencer.h
index b2a501e445..f925e349c5 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -50,8 +50,6 @@ struct replay_opts {
 
 	int mainline;
 
-	char *committer_name;
-	char *committer_email;
 	char *gpg_sign;
 	enum commit_msg_cleanup_mode default_msg_cleanup;
 	int explicit_cleanup;
-- 
2.29.0.583.g8e3ac41d8f

Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Jeff King]

On Fri, Oct 23, 2020 at 03:26:30AM -0400, Jeff King wrote:

> By the way, I wondered why we needed to do this parsing at all. The
> patch below does this in a much simpler way. It's a little bit ugly, I
> think, because we have to call getenv() ourselves. But that's the way
> fmt_ident() has always worked. We could probably improve that now that
> it takes a whose_ident flag (before that, it had no idea if we wanted
> author or committer ident).

I took a brief look at refactoring fmt_ident() to auto-fill from the
environment variables (patch below). It's mostly sensible for
name/email, because callers pass either the environment variables or
some custom-provided value. But for the date, we sometimes pass NULL to
mean "use the current time, even if $GIT_*_DATE is set". I'm not 100%
sure that isn't a bug, though.

---
 builtin/am.c |  4 +--
 ident.c      | 47 ++++++++++++++++-------------------
 sequencer.c  |  4 +--
 3 files changed, 23 insertions(+), 32 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 52206bc56b..9f0d1b4859 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1580,9 +1580,7 @@ static void do_commit(const struct am_state *state)
 			IDENT_STRICT);
 
 	if (state->committer_date_is_author_date)
-		committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
-				      getenv("GIT_COMMITTER_EMAIL"),
-				      WANT_COMMITTER_IDENT,
+		committer = fmt_ident(NULL, NULL, WANT_COMMITTER_IDENT,
 				      state->ignore_date ? NULL
 							 : state->author_date,
 				      IDENT_STRICT);
diff --git a/ident.c b/ident.c
index 6aba4b5cb6..7743c1ed05 100644
--- a/ident.c
+++ b/ident.c
@@ -384,6 +384,12 @@ const char *fmt_ident(const char *name, const char *email,
 	struct strbuf *ident = &ident_pool[index];
 	index = (index + 1) % ARRAY_SIZE(ident_pool);
 
+	if (!email) {
+		if (whose_ident == WANT_AUTHOR_IDENT)
+			email = getenv("GIT_AUTHOR_EMAIL");
+		else if (whose_ident == WANT_COMMITTER_IDENT)
+			email = getenv("GIT_COMMITTER_EMAIL");
+	}
 	if (!email) {
 		if (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)
 			email = git_author_email.buf;
@@ -405,6 +411,12 @@ const char *fmt_ident(const char *name, const char *email,
 
 	if (want_name) {
 		int using_default = 0;
+		if (!name) {
+			if (whose_ident == WANT_AUTHOR_IDENT)
+				name = getenv("GIT_AUTHOR_NAME");
+			else if (whose_ident == WANT_COMMITTER_IDENT)
+				name = getenv("GIT_COMMITTER_NAME");
+		}
 		if (!name) {
 			if (whose_ident == WANT_AUTHOR_IDENT && git_author_name.len)
 				name = git_author_name.buf;
@@ -449,6 +461,12 @@ const char *fmt_ident(const char *name, const char *email,
 		strbuf_addch(ident, '>');
 	if (want_date) {
 		strbuf_addch(ident, ' ');
+		if (!date_str) {
+			if (whose_ident == WANT_AUTHOR_IDENT)
+				date_str = getenv("GIT_AUTHOR_DATE");
+			else if (whose_ident == WANT_COMMITTER_IDENT)
+				date_str = getenv("GIT_COMMITTER_DATE");
+		}
 		if (date_str && date_str[0]) {
 			if (parse_date(date_str, ident) < 0)
 				die(_("invalid date format: %s"), date_str);
@@ -462,22 +480,7 @@ const char *fmt_ident(const char *name, const char *email,
 
 const char *fmt_name(enum want_ident whose_ident)
 {
-	char *name = NULL;
-	char *email = NULL;
-
-	switch (whose_ident) {
-	case WANT_BLANK_IDENT:
-		break;
-	case WANT_AUTHOR_IDENT:
-		name = getenv("GIT_AUTHOR_NAME");
-		email = getenv("GIT_AUTHOR_EMAIL");
-		break;
-	case WANT_COMMITTER_IDENT:
-		name = getenv("GIT_COMMITTER_NAME");
-		email = getenv("GIT_COMMITTER_EMAIL");
-		break;
-	}
-	return fmt_ident(name, email, whose_ident, NULL,
+	return fmt_ident(NULL, NULL, whose_ident, NULL,
 			IDENT_STRICT | IDENT_NO_DATE);
 }
 
@@ -487,11 +490,7 @@ const char *git_author_info(int flag)
 		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_AUTHOR_EMAIL"))
 		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
-			 getenv("GIT_AUTHOR_EMAIL"),
-			 WANT_AUTHOR_IDENT,
-			 getenv("GIT_AUTHOR_DATE"),
-			 flag);
+	return fmt_ident(NULL, NULL, WANT_AUTHOR_IDENT, NULL, flag);
 }
 
 const char *git_committer_info(int flag)
@@ -500,11 +499,7 @@ const char *git_committer_info(int flag)
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_COMMITTER_EMAIL"))
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
-			 getenv("GIT_COMMITTER_EMAIL"),
-			 WANT_COMMITTER_IDENT,
-			 getenv("GIT_COMMITTER_DATE"),
-			 flag);
+	return fmt_ident(NULL, NULL, WANT_COMMITTER_IDENT, NULL, flag);
 }
 
 static int ident_is_sufficient(int user_ident_explicitly_given)
diff --git a/sequencer.c b/sequencer.c
index 07321a7d95..6a8a5c077b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1458,9 +1458,7 @@ static int try_to_commit(struct repository *r,
 		} else {
 			reset_ident_date();
 		}
-		committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
-				      getenv("GIT_COMMITTER_EMAIL"),
-				      WANT_COMMITTER_IDENT,
+		committer = fmt_ident(NULL, NULL, WANT_COMMITTER_IDENT,
 				      opts->ignore_date ? NULL : date.buf,
 				      IDENT_STRICT);
 		strbuf_release(&date);

Re: [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully

[Phillip Wood]

Hi Peff

Thanks for fixing this embarrassing mistake and strengthening the tests, 
the first three patches look fine to me. We don't check the committer 
name and email in t4150-am.sh which we probably should do (the rebase 
tests were based on those) though that can wait as some of the rebase 
tests are testing the am implementation.

Thanks again

Phillip

On 23/10/2020 08:08, Jeff King wrote:
> After running "rebase --committer-date-is-author-date", we confirm that
> the committer date is the same as the author date. However, we don't
> look at any other parts of the committer ident line to make sure we
> didn't screw them up. And indeed, there are a few bugs here. Depending
> on the rebase backend in use, we may accidentally use the author email
> instead of the committer's, or even an empty string.
> 
> Let's teach our test_ctime_is_atime helper to check the committer name
> and email, which reveals several failing tests.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   t/t3436-rebase-more-options.sh | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> index 996e82787e..6f2f49717b 100755
> --- a/t/t3436-rebase-more-options.sh
> +++ b/t/t3436-rebase-more-options.sh
> @@ -65,31 +65,31 @@ test_expect_success '--ignore-whitespace is remembered when continuing' '
>   '
>   
>   test_ctime_is_atime () {
> -	git log $1 --format=%ai >authortime &&
> -	git log $1 --format=%ci >committertime &&
> +	git log $1 --format="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> %ai" >authortime &&
> +	git log $1 --format="%cn <%ce> %ci" >committertime &&
>   	test_cmp authortime committertime
>   }
>   
> -test_expect_success '--committer-date-is-author-date works with apply backend' '
> +test_expect_failure '--committer-date-is-author-date works with apply backend' '
>   	GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author &&
>   	git rebase --apply --committer-date-is-author-date HEAD^ &&
>   	test_ctime_is_atime -1
>   '
>   
> -test_expect_success '--committer-date-is-author-date works with merge backend' '
> +test_expect_failure '--committer-date-is-author-date works with merge backend' '
>   	GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author &&
>   	git rebase -m --committer-date-is-author-date HEAD^ &&
>   	test_ctime_is_atime -1
>   '
>   
> -test_expect_success '--committer-date-is-author-date works with rebase -r' '
> +test_expect_failure '--committer-date-is-author-date works with rebase -r' '
>   	git checkout side &&
>   	GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
>   	git rebase -r --root --committer-date-is-author-date &&
>   	test_ctime_is_atime
>   '
>   
> -test_expect_success '--committer-date-is-author-date works when forking merge' '
> +test_expect_failure '--committer-date-is-author-date works when forking merge' '
>   	git checkout side &&
>   	GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
>   	PATH="./test-bin:$PATH" git rebase -r --root --strategy=test \
> @@ -145,7 +145,7 @@ test_expect_success '--reset-author-date works with rebase -r' '
>   	test_atime_is_ignored
>   '
>   
> -test_expect_success '--reset-author-date with --committer-date-is-author-date works' '
> +test_expect_failure '--reset-author-date with --committer-date-is-author-date works' '
>   	test_must_fail git rebase -m --committer-date-is-author-date \
>   		--reset-author-date --onto commit2^^ commit2^ commit3 &&
>   	git checkout --theirs foo &&
> 

Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Phillip Wood]

Hi Peff

On 23/10/2020 08:26, Jeff King wrote:
> On Fri, Oct 23, 2020 at 03:09:39AM -0400, Jeff King wrote:
> 
>> Commit e8cbe2118a (am: stop exporting GIT_COMMITTER_DATE, 2020-08-17)
>> rewrote the code for setting the committer date to use fmt_ident(),
>> rather than setting an environment variable and letting commit_tree()
>> handle it. But it introduced two bugs:
>>
>>    - we use the author email string instead of the committer email
>>
>>    - when parsing the committer ident, we used the wrong variable to
>>      compute the length of the email, resulting in it always being a
>>      zero-length string
> 
> By the way, I wondered why we needed to do this parsing at all. The
> patch below does this in a much simpler way. It's a little bit ugly, I
> think, because we have to call getenv() ourselves. But that's the way
> fmt_ident() has always worked. We could probably improve that now that
> it takes a whose_ident flag (before that, it had no idea if we wanted
> author or committer ident).
> 
> This is on top of the fixes (but we'd perhaps just want to do those on
> 'maint' as the minimal fix).
> 
> -- >8 --
> Subject: [PATCH 4/3] am, sequencer: stop parsing our own committer ident
> 
> For the --committer-date-is-author-date option of git-am and git-rebase,
> we format the committer ident, then re-parse it to find the name and
> email, and then feed those back to fmt_ident().
> 
> We can simplify this by handling it all at the time of the fmt_ident()
> call. We pass in the appropriate getenv() results, and if they're not
> present, then our WANT_COMMITTER_IDENT flag tells fmt_ident() to fill in
> the appropriate value from the config. Which is exactly what
> git_committer_ident() was doing under the hood.

Makes sense and it simplifies the code nicely

Thanks

Phillip

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   builtin/am.c | 19 +++----------------
>   sequencer.c  | 28 ++--------------------------
>   sequencer.h  |  2 --
>   3 files changed, 5 insertions(+), 44 deletions(-)
> 
> diff --git a/builtin/am.c b/builtin/am.c
> index 4949535a7f..52206bc56b 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -98,8 +98,6 @@ struct am_state {
>   	char *author_name;
>   	char *author_email;
>   	char *author_date;
> -	char *committer_name;
> -	char *committer_email;
>   	char *msg;
>   	size_t msg_len;
>   
> @@ -132,8 +130,6 @@ struct am_state {
>    */
>   static void am_state_init(struct am_state *state)
>   {
> -	const char *committer;
> -	struct ident_split id;
>   	int gpgsign;
>   
>   	memset(state, 0, sizeof(*state));
> @@ -154,14 +150,6 @@ static void am_state_init(struct am_state *state)
>   
>   	if (!git_config_get_bool("commit.gpgsign", &gpgsign))
>   		state->sign_commit = gpgsign ? "" : NULL;
> -
> -	committer = git_committer_info(IDENT_STRICT);
> -	if (split_ident_line(&id, committer, strlen(committer)) < 0)
> -		die(_("invalid committer: %s"), committer);
> -	state->committer_name =
> -		xmemdupz(id.name_begin, id.name_end - id.name_begin);
> -	state->committer_email =
> -		xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
>   }
>   
>   /**
> @@ -173,8 +161,6 @@ static void am_state_release(struct am_state *state)
>   	free(state->author_name);
>   	free(state->author_email);
>   	free(state->author_date);
> -	free(state->committer_name);
> -	free(state->committer_email);
>   	free(state->msg);
>   	strvec_clear(&state->git_apply_opts);
>   }
> @@ -1594,8 +1580,9 @@ static void do_commit(const struct am_state *state)
>   			IDENT_STRICT);
>   
>   	if (state->committer_date_is_author_date)
> -		committer = fmt_ident(state->committer_name,
> -				      state->committer_email, WANT_COMMITTER_IDENT,
> +		committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
> +				      getenv("GIT_COMMITTER_EMAIL"),
> +				      WANT_COMMITTER_IDENT,
>   				      state->ignore_date ? NULL
>   							 : state->author_date,
>   				      IDENT_STRICT);
> diff --git a/sequencer.c b/sequencer.c
> index d76cbded00..07321a7d95 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -314,8 +314,6 @@ int sequencer_remove_state(struct replay_opts *opts)
>   		}
>   	}
>   
> -	free(opts->committer_name);
> -	free(opts->committer_email);
>   	free(opts->gpg_sign);
>   	free(opts->strategy);
>   	for (i = 0; i < opts->xopts_nr; i++)
> @@ -1460,8 +1458,8 @@ static int try_to_commit(struct repository *r,
>   		} else {
>   			reset_ident_date();
>   		}
> -		committer = fmt_ident(opts->committer_name,
> -				      opts->committer_email,
> +		committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
> +				      getenv("GIT_COMMITTER_EMAIL"),
>   				      WANT_COMMITTER_IDENT,
>   				      opts->ignore_date ? NULL : date.buf,
>   				      IDENT_STRICT);
> @@ -4467,22 +4465,6 @@ static int commit_staged_changes(struct repository *r,
>   	return 0;
>   }
>   
> -static int init_committer(struct replay_opts *opts)
> -{
> -	struct ident_split id;
> -	const char *committer;
> -
> -	committer = git_committer_info(IDENT_STRICT);
> -	if (split_ident_line(&id, committer, strlen(committer)) < 0)
> -		return error(_("invalid committer '%s'"), committer);
> -	opts->committer_name =
> -		xmemdupz(id.name_begin, id.name_end - id.name_begin);
> -	opts->committer_email =
> -		xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
> -
> -	return 0;
> -}
> -
>   int sequencer_continue(struct repository *r, struct replay_opts *opts)
>   {
>   	struct todo_list todo_list = TODO_LIST_INIT;
> @@ -4494,9 +4476,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
>   	if (read_populate_opts(opts))
>   		return -1;
>   	if (is_rebase_i(opts)) {
> -		if (opts->committer_date_is_author_date && init_committer(opts))
> -			return -1;
> -
>   		if ((res = read_populate_todo(r, &todo_list, opts)))
>   			goto release_todo_list;
>   
> @@ -5391,9 +5370,6 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   
>   	res = -1;
>   
> -	if (opts->committer_date_is_author_date && init_committer(opts))
> -		goto cleanup;
> -
>   	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
>   		goto cleanup;
>   
> diff --git a/sequencer.h b/sequencer.h
> index b2a501e445..f925e349c5 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,8 +50,6 @@ struct replay_opts {
>   
>   	int mainline;
>   
> -	char *committer_name;
> -	char *committer_email;
>   	char *gpg_sign;
>   	enum commit_msg_cleanup_mode default_msg_cleanup;
>   	int explicit_cleanup;
> 

Re: committer-date-is-author-date flag removes email in "Commit"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Oct 23, 2020 at 11:18:51AM +0530, VenomVendor wrote:
>
>> What did you do before the bug happened? (Steps to reproduce your issue)
>> * Create empty repo using `git init`
>> * Make few commits, at least two
>> * execute `git log --format=fuller`
>> * Notice the log, with "Author", "AuthorDate", "Commit", "CommitDate"
>> * Note, "Commit"
>> * execute `git rebase --committer-date-is-author-date HEAD~1`
>> * execute `git log --format=fuller`
>> * Note, email from "Commit" is empty <>
>
> Thanks for a clear report. I was able to easily reproduce the problem.
> There are actually two related bugs here, and they're both regressions
> in v2.29.0.
>
>   [1/3]: t3436: check --committer-date-is-author-date result more carefully
>   [2/3]: am: fix broken email with --committer-date-is-author-date
>   [3/3]: rebase: fix broken email with --committer-date-is-author-date

Thanks for taking quick care of this.  It counts as an embarrasing
brown-paper-bag bug; it is a bit surprising that nobody noticed it
while the original change was discussed.

I wonder if we even needed to do the original change to begin with
(stopping to export means not giving information to the hooks), but
that is a separate matter.

Will take a look and queue.  Thanks.

>
>  builtin/am.c                   | 4 ++--
>  sequencer.c                    | 2 +-
>  t/t3436-rebase-more-options.sh | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> -Peff

Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Taylor Blau]

On Fri, Oct 23, 2020 at 03:45:10AM -0400, Jeff King wrote:
> On Fri, Oct 23, 2020 at 03:26:30AM -0400, Jeff King wrote:
>
> > By the way, I wondered why we needed to do this parsing at all. The
> > patch below does this in a much simpler way. It's a little bit ugly, I
> > think, because we have to call getenv() ourselves. But that's the way
> > fmt_ident() has always worked. We could probably improve that now that
> > it takes a whose_ident flag (before that, it had no idea if we wanted
> > author or committer ident).
>
> I took a brief look at refactoring fmt_ident() to auto-fill from the
> environment variables (patch below). It's mostly sensible for
> name/email, because callers pass either the environment variables or
> some custom-provided value. But for the date, we sometimes pass NULL to
> mean "use the current time, even if $GIT_*_DATE is set". I'm not 100%
> sure that isn't a bug, though.

That seems like an area where further investigation would be helpful.
In the meantime (especially if this is something that Junio wants to
queue and call v2.29.2), I'd be happy with the four patches you posted
(but waiting on this one below).

Thanks for investigating.

Thanks,
Taylor

Re: committer-date-is-author-date flag removes email in "Commit"

[Phillip Wood]

On 23/10/2020 16:23, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Fri, Oct 23, 2020 at 11:18:51AM +0530, VenomVendor wrote:
>>
>>> What did you do before the bug happened? (Steps to reproduce your issue)
>>> * Create empty repo using `git init`
>>> * Make few commits, at least two
>>> * execute `git log --format=fuller`
>>> * Notice the log, with "Author", "AuthorDate", "Commit", "CommitDate"
>>> * Note, "Commit"
>>> * execute `git rebase --committer-date-is-author-date HEAD~1`
>>> * execute `git log --format=fuller`
>>> * Note, email from "Commit" is empty <>
>>
>> Thanks for a clear report. I was able to easily reproduce the problem.
>> There are actually two related bugs here, and they're both regressions
>> in v2.29.0.
>>
>>    [1/3]: t3436: check --committer-date-is-author-date result more carefully
>>    [2/3]: am: fix broken email with --committer-date-is-author-date
>>    [3/3]: rebase: fix broken email with --committer-date-is-author-date
> 
> Thanks for taking quick care of this.  It counts as an embarrasing
> brown-paper-bag bug; it is a bit surprising that nobody noticed it
> while the original change was discussed.

It is indeed embarrassing. That change only appeared in the last round 
of patches and unfortunately I think most people had stopped looking 
them it by then as their comments had been addressed in previous rounds.

> I wonder if we even needed to do the original change to begin with
> (stopping to export means not giving information to the hooks), but
> that is a separate matter.

I think the main motivation was to stop polluting the environment of 
exec commands

Best Wishes

Phillip

> Will take a look and queue.  Thanks.
> 
>>
>>   builtin/am.c                   | 4 ++--
>>   sequencer.c                    | 2 +-
>>   t/t3436-rebase-more-options.sh | 4 ++--
>>   3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> -Peff

Re: committer-date-is-author-date flag removes email in "Commit"

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> I wonder if we even needed to do the original change to begin with
>> (stopping to export means not giving information to the hooks), but
>> that is a separate matter.
>
> I think the main motivation was to stop polluting the environment of
> exec commands

As long as the information that used to be exported can be learned
by the hook script when/if it wants to (how?  read the original
commit object with "git show -s --format=..."?  How would it know
which one is the original commit?), it is fine.

If not, a script that does not want the exported environment
variable can easily ignore it (just "unset" it) but to a script that
wanted to learn the information that used to be in the environment
variable, it's not "stop polluting" but "lose information", and that
is where my earlier "I wonder" comes from.

Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Johannes Schindelin]

Hi Peff,

On Fri, 23 Oct 2020, Jeff King wrote:

> diff --git a/ident.c b/ident.c
> index 6aba4b5cb6..7743c1ed05 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -384,6 +384,12 @@ const char *fmt_ident(const char *name, const char *email,
>  	struct strbuf *ident = &ident_pool[index];
>  	index = (index + 1) % ARRAY_SIZE(ident_pool);
>
> +	if (!email) {
> +		if (whose_ident == WANT_AUTHOR_IDENT)
> +			email = getenv("GIT_AUTHOR_EMAIL");
> +		else if (whose_ident == WANT_COMMITTER_IDENT)
> +			email = getenv("GIT_COMMITTER_EMAIL");

I *guess* that this is a strict improvement, calling `getenv()` much
closer to the time the value is actually used (and hence avoiding the
problem where pointers returned by `getenv()` get stale due to environment
changes).

Thanks,
Dscho

Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> By the way, I wondered why we needed to do this parsing at all. The
> patch below does this in a much simpler way. It's a little bit ugly, I
> think, because we have to call getenv() ourselves. But that's the way
> fmt_ident() has always worked. We could probably improve that now that
> it takes a whose_ident flag (before that, it had no idea if we wanted
> author or committer ident).
>
> This is on top of the fixes (but we'd perhaps just want to do those on
> 'maint' as the minimal fix).

This could be the nicest step in the whole series, but let's leave
it out of the branch meant for 'maint'.

Thanks.

Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Jeff King]

On Mon, Oct 26, 2020 at 05:25:01PM +0100, Johannes Schindelin wrote:

> On Fri, 23 Oct 2020, Jeff King wrote:
> 
> > diff --git a/ident.c b/ident.c
> > index 6aba4b5cb6..7743c1ed05 100644
> > --- a/ident.c
> > +++ b/ident.c
> > @@ -384,6 +384,12 @@ const char *fmt_ident(const char *name, const char *email,
> >  	struct strbuf *ident = &ident_pool[index];
> >  	index = (index + 1) % ARRAY_SIZE(ident_pool);
> >
> > +	if (!email) {
> > +		if (whose_ident == WANT_AUTHOR_IDENT)
> > +			email = getenv("GIT_AUTHOR_EMAIL");
> > +		else if (whose_ident == WANT_COMMITTER_IDENT)
> > +			email = getenv("GIT_COMMITTER_EMAIL");
> 
> I *guess* that this is a strict improvement, calling `getenv()` much
> closer to the time the value is actually used (and hence avoiding the
> problem where pointers returned by `getenv()` get stale due to environment
> changes).

I don't think it changes much in practice. Most of the callers are
passing the values directly in to this function, and there's not much
that happens between the function starting and these calls.

The more worrisome stretch is that we likely call strbuf functions while
holding on to a getenv() pointer. And those potentially do things like
xmalloc(), which looks at GIT_ALLOC_LIMIT, etc. But though POSIX
promises only one getenv() result at a time, we definitely don't adhere
to that (after all, we routinely pass the results of three separate
getenv() calls to fmt_ident!). As you well know, because mingw_getenv()
has a circular buffer hack to deal with this. :)

So it certainly isn't making anything worse, but I'd be surprised if it
actually helped at all.

-Peff

Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Jeff King]

On Mon, Oct 26, 2020 at 10:12:45AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > By the way, I wondered why we needed to do this parsing at all. The
> > patch below does this in a much simpler way. It's a little bit ugly, I
> > think, because we have to call getenv() ourselves. But that's the way
> > fmt_ident() has always worked. We could probably improve that now that
> > it takes a whose_ident flag (before that, it had no idea if we wanted
> > author or committer ident).
> >
> > This is on top of the fixes (but we'd perhaps just want to do those on
> > 'maint' as the minimal fix).
> 
> This could be the nicest step in the whole series, but let's leave
> it out of the branch meant for 'maint'.

I think that's sensible. Looks like you queued it separately already, so
thank you. :)

-Peff