{"thread":{"id":"54489","subject":"committer-date-is-author-date flag removes email in \"Commit\"","startedAt":"2020-10-23T06:12:13Z","lastAt":"2020-10-27T07:24:48Z","messageCount":17,"participants":["VenomVendor","Jeff King","Phillip Wood","Junio C Hamano","Taylor Blau","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"408204","messageId":"e08df0d5792ab70a9158be32cfa28696@venomvendor.com","threadId":"54489","inReplyTo":null,"subject":"committer-date-is-author-date flag removes email in \"Commit\"","fromName":"VenomVendor","fromEmail":"info@venomvendor.com","sentAt":"2020-10-23T05:48:51Z","receivedAt":"2020-10-23T06:12:13Z","isPatch":false,"sender":{"key":"info@venomvendor.com","avatar":null},"body":"committer-date-is-author-date flag removes email in \"Commit\"\n\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n* Create empty repo using `git init`\n* Make few commits, at least two\n* execute `git log --format=fuller`\n* Notice the log, with \"Author\", \"AuthorDate\", \"Commit\", \"CommitDate\"\n* Note, \"Commit\"\n* execute `git rebase --committer-date-is-author-date HEAD~1`\n* execute `git log --format=fuller`\n* Note, email from \"Commit\" is empty <>\n\n\nWhat did you expect to happen? (Expected behavior)\n* Email must not be removed.\n\nWhat happened instead? (Actual behavior)\n* email got removed/replaced with empty `< >`\n\nWhat's different between what you expected and what actually happened?\n\nExpected:\n```\nAuthor:     VenomVendor <info@VenomVendor.com>\nAuthorDate: Thu Oct 22 19:41:05 2020 -0200\nCommit:     VenomVendor <info@VenomVendor.com>\nCommitDate: Thu Oct 22 19:41:05 2020 -0200\n```\n\nHappened (Missing email in Commit)\n```\nAuthor:     VenomVendor <info@VenomVendor.com>\nAuthorDate: Thu Oct 22 19:41:05 2020 -0200\nCommit:     VenomVendor <>\nCommitDate: Thu Oct 22 19:41:05 2020 -0200\n```\n\nAnything else you want to add:\n\n```\n$ cd ~\n\n$ mkdir committer-date-is-author-date-bug\n\n$ cd committer-date-is-author-date-bug\n\n$ git --version\ngit version 2.29.0\n\n$ git init\nInitialized empty Git repository in \n~/committer-date-is-author-date-bug/.git/\n\n$ git log --format=fuller\nfatal: your current branch 'master' does not have any commits yet\n\n\n$ TEXT=1; echo \"${TEXT}\" > sample.txt && git add . && git commit -m \n\"${TEXT}\"\n[master (root-commit) 20731cb] 1\n  1 file changed, 1 insertion(+)\n  create mode 100644 sample.txt\n\n\n$ TEXT=2; echo \"${TEXT}\" > sample.txt && git add . && git commit -m \n\"${TEXT}\"\n[master e3d8ce8] 2\n  1 file changed, 1 insertion(+), 1 deletion(-)\n\n\n$ TEXT=3; echo \"${TEXT}\" > sample.txt && git add . && git commit -m \n\"${TEXT}\"\n[master 30f2b84] 3\n  1 file changed, 1 insertion(+), 1 deletion(-)\n\n\n$ git log --format=fuller\ncommit 30f2b84e360d2ea79fe3355b5fbfa5cdb401c65f (HEAD -> master)\nAuthor:     VenomVendor <info@VenomVendor.com>\nAuthorDate: Thu Oct 22 19:41:05 2020 -0200\nCommit:     VenomVendor <info@VenomVendor.com>\nCommitDate: Thu Oct 22 19:41:05 2020 -0200\n\n     3\n\ncommit e3d8ce88ea25b14cf2840ccd41cebc8f68663b8e\nAuthor:     VenomVendor <info@VenomVendor.com>\nAuthorDate: Thu Oct 22 19:41:02 2020 -0200\nCommit:     VenomVendor <info@VenomVendor.com>\nCommitDate: Thu Oct 22 19:41:02 2020 -0200\n\n     2\n\ncommit 20731cb051e42dd5435dec4157aa58203e5fc60d\nAuthor:     VenomVendor <info@VenomVendor.com>\nAuthorDate: Thu Oct 22 19:40:59 2020 -0200\nCommit:     VenomVendor <info@VenomVendor.com>\nCommitDate: Thu Oct 22 19:40:59 2020 -0200\n\n     1\n\n\n$ git rebase --committer-date-is-author-date HEAD~2\nCurrent branch master is up to date, rebase forced.\nSuccessfully rebased and updated refs/heads/master.\n\n\n$ git log --format=fuller\ncommit be24ee5cd9b76b0f11599b4f8af2609f6c5f3b06 (HEAD -> master)\nAuthor:     VenomVendor <info@VenomVendor.com>\nAuthorDate: Thu Oct 22 19:41:05 2020 -0200\nCommit:     VenomVendor <>\nCommitDate: Thu Oct 22 19:41:05 2020 -0200\n\n     3\n\ncommit c45d01b6969281d11fc78f60cb0c4d830f216892\nAuthor:     VenomVendor <info@VenomVendor.com>\nAuthorDate: Thu Oct 22 19:41:02 2020 -0200\nCommit:     VenomVendor <>\nCommitDate: Thu Oct 22 19:41:02 2020 -0200\n\n     2\n\ncommit 20731cb051e42dd5435dec4157aa58203e5fc60d\nAuthor:     VenomVendor <info@VenomVendor.com>\nAuthorDate: Thu Oct 22 19:40:59 2020 -0200\nCommit:     VenomVendor <info@VenomVendor.com>\nCommitDate: Thu Oct 22 19:40:59 2020 -0200\n\n     1\n```\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.29.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 \nPDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64\ncompiler info: clang: 12.0.0 (clang-1200.0.32.2)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/zsh\n\n\n[Enabled Hooks]\n\n-VenomVendor\n"},{"id":"408205","messageId":"20201023070747.GA2198273@coredump.intra.peff.net","threadId":"54489","inReplyTo":"e08df0d5792ab70a9158be32cfa28696@venomvendor.com","subject":"Re: committer-date-is-author-date flag removes email in \"Commit\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T07:07:47Z","receivedAt":"2020-10-23T07:07:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 23, 2020 at 11:18:51AM +0530, VenomVendor wrote:\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> * Create empty repo using `git init`\n> * Make few commits, at least two\n> * execute `git log --format=fuller`\n> * Notice the log, with \"Author\", \"AuthorDate\", \"Commit\", \"CommitDate\"\n> * Note, \"Commit\"\n> * execute `git rebase --committer-date-is-author-date HEAD~1`\n> * execute `git log --format=fuller`\n> * Note, email from \"Commit\" is empty <>\n\nThanks for a clear report. I was able to easily reproduce the problem.\nThere are actually two related bugs here, and they're both regressions\nin v2.29.0.\n\n  [1/3]: t3436: check --committer-date-is-author-date result more carefully\n  [2/3]: am: fix broken email with --committer-date-is-author-date\n  [3/3]: rebase: fix broken email with --committer-date-is-author-date\n\n builtin/am.c                   | 4 ++--\n sequencer.c                    | 2 +-\n t/t3436-rebase-more-options.sh | 4 ++--\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\n-Peff\n"},{"id":"408206","messageId":"20201023070843.GA2913115@coredump.intra.peff.net","threadId":"54489","inReplyTo":"20201023070747.GA2198273@coredump.intra.peff.net","subject":"[PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T07:08:43Z","receivedAt":"2020-10-23T07:08:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After running \"rebase --committer-date-is-author-date\", we confirm that\nthe committer date is the same as the author date. However, we don't\nlook at any other parts of the committer ident line to make sure we\ndidn't screw them up. And indeed, there are a few bugs here. Depending\non the rebase backend in use, we may accidentally use the author email\ninstead of the committer's, or even an empty string.\n\nLet's teach our test_ctime_is_atime helper to check the committer name\nand email, which reveals several failing tests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3436-rebase-more-options.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 996e82787e..6f2f49717b 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -65,31 +65,31 @@ test_expect_success '--ignore-whitespace is remembered when continuing' '\n '\n \n test_ctime_is_atime () {\n-\tgit log $1 --format=%ai >authortime &&\n-\tgit log $1 --format=%ci >committertime &&\n+\tgit log $1 --format=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> %ai\" >authortime &&\n+\tgit log $1 --format=\"%cn <%ce> %ci\" >committertime &&\n \ttest_cmp authortime committertime\n }\n \n-test_expect_success '--committer-date-is-author-date works with apply backend' '\n+test_expect_failure '--committer-date-is-author-date works with apply backend' '\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n \tgit rebase --apply --committer-date-is-author-date HEAD^ &&\n \ttest_ctime_is_atime -1\n '\n \n-test_expect_success '--committer-date-is-author-date works with merge backend' '\n+test_expect_failure '--committer-date-is-author-date works with merge backend' '\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n \tgit rebase -m --committer-date-is-author-date HEAD^ &&\n \ttest_ctime_is_atime -1\n '\n \n-test_expect_success '--committer-date-is-author-date works with rebase -r' '\n+test_expect_failure '--committer-date-is-author-date works with rebase -r' '\n \tgit checkout side &&\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git merge --no-ff commit3 &&\n \tgit rebase -r --root --committer-date-is-author-date &&\n \ttest_ctime_is_atime\n '\n \n-test_expect_success '--committer-date-is-author-date works when forking merge' '\n+test_expect_failure '--committer-date-is-author-date works when forking merge' '\n \tgit checkout side &&\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git merge --no-ff commit3 &&\n \tPATH=\"./test-bin:$PATH\" git rebase -r --root --strategy=test \\\n@@ -145,7 +145,7 @@ test_expect_success '--reset-author-date works with rebase -r' '\n \ttest_atime_is_ignored\n '\n \n-test_expect_success '--reset-author-date with --committer-date-is-author-date works' '\n+test_expect_failure '--reset-author-date with --committer-date-is-author-date works' '\n \ttest_must_fail git rebase -m --committer-date-is-author-date \\\n \t\t--reset-author-date --onto commit2^^ commit2^ commit3 &&\n \tgit checkout --theirs foo &&\n-- \n2.29.0.583.g8e3ac41d8f\n\n"},{"id":"408207","messageId":"20201023070939.GB2913115@coredump.intra.peff.net","threadId":"54489","inReplyTo":"20201023070747.GA2198273@coredump.intra.peff.net","subject":"[PATCH 2/3] am: fix broken email with --committer-date-is-author-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T07:09:39Z","receivedAt":"2020-10-23T07:09:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit e8cbe2118a (am: stop exporting GIT_COMMITTER_DATE, 2020-08-17)\nrewrote the code for setting the committer date to use fmt_ident(),\nrather than setting an environment variable and letting commit_tree()\nhandle it. But it introduced two bugs:\n\n  - we use the author email string instead of the committer email\n\n  - when parsing the committer ident, we used the wrong variable to\n    compute the length of the email, resulting in it always being a\n    zero-length string\n\nThis commit fixes both, which causes our test of this option via the\nrebase \"apply\" backend to now succeed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c                   | 4 ++--\n t/t3436-rebase-more-options.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 2c7673f74e..4949535a7f 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -161,7 +161,7 @@ static void am_state_init(struct am_state *state)\n \tstate->committer_name =\n \t\txmemdupz(id.name_begin, id.name_end - id.name_begin);\n \tstate->committer_email =\n-\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_end);\n+\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_begin);\n }\n \n /**\n@@ -1595,7 +1595,7 @@ static void do_commit(const struct am_state *state)\n \n \tif (state->committer_date_is_author_date)\n \t\tcommitter = fmt_ident(state->committer_name,\n-\t\t\t\t      state->author_email, WANT_COMMITTER_IDENT,\n+\t\t\t\t      state->committer_email, WANT_COMMITTER_IDENT,\n \t\t\t\t      state->ignore_date ? NULL\n \t\t\t\t\t\t\t : state->author_date,\n \t\t\t\t      IDENT_STRICT);\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 6f2f49717b..3fda2235bd 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -70,7 +70,7 @@ test_ctime_is_atime () {\n \ttest_cmp authortime committertime\n }\n \n-test_expect_failure '--committer-date-is-author-date works with apply backend' '\n+test_expect_success '--committer-date-is-author-date works with apply backend' '\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n \tgit rebase --apply --committer-date-is-author-date HEAD^ &&\n \ttest_ctime_is_atime -1\n-- \n2.29.0.583.g8e3ac41d8f\n\n"},{"id":"408208","messageId":"20201023071015.GC2913115@coredump.intra.peff.net","threadId":"54489","inReplyTo":"20201023070747.GA2198273@coredump.intra.peff.net","subject":"[PATCH 3/3] rebase: fix broken email with --committer-date-is-author-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T07:10:15Z","receivedAt":"2020-10-23T07:10:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 7573cec52c (rebase -i: support --committer-date-is-author-date,\n2020-08-17) copied the committer ident-parsing code from builtin/am.c.\nAnd in doing so, it copied a bug in which we always set the email to an\nempty string. We fixed the version in git-am in the previous commit;\nthis commit fixes the copied code.\n\nReported-by: VenomVendor <info@venomvendor.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c                    | 2 +-\n t/t3436-rebase-more-options.sh | 8 ++++----\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 00acb12496..d76cbded00 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4478,7 +4478,7 @@ static int init_committer(struct replay_opts *opts)\n \topts->committer_name =\n \t\txmemdupz(id.name_begin, id.name_end - id.name_begin);\n \topts->committer_email =\n-\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_end);\n+\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_begin);\n \n \treturn 0;\n }\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 3fda2235bd..eaaf4c8d1d 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -76,20 +76,20 @@ test_expect_success '--committer-date-is-author-date works with apply backend' '\n \ttest_ctime_is_atime -1\n '\n \n-test_expect_failure '--committer-date-is-author-date works with merge backend' '\n+test_expect_success '--committer-date-is-author-date works with merge backend' '\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n \tgit rebase -m --committer-date-is-author-date HEAD^ &&\n \ttest_ctime_is_atime -1\n '\n \n-test_expect_failure '--committer-date-is-author-date works with rebase -r' '\n+test_expect_success '--committer-date-is-author-date works with rebase -r' '\n \tgit checkout side &&\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git merge --no-ff commit3 &&\n \tgit rebase -r --root --committer-date-is-author-date &&\n \ttest_ctime_is_atime\n '\n \n-test_expect_failure '--committer-date-is-author-date works when forking merge' '\n+test_expect_success '--committer-date-is-author-date works when forking merge' '\n \tgit checkout side &&\n \tGIT_AUTHOR_DATE=\"@1234 +0300\" git merge --no-ff commit3 &&\n \tPATH=\"./test-bin:$PATH\" git rebase -r --root --strategy=test \\\n@@ -145,7 +145,7 @@ test_expect_success '--reset-author-date works with rebase -r' '\n \ttest_atime_is_ignored\n '\n \n-test_expect_failure '--reset-author-date with --committer-date-is-author-date works' '\n+test_expect_success '--reset-author-date with --committer-date-is-author-date works' '\n \ttest_must_fail git rebase -m --committer-date-is-author-date \\\n \t\t--reset-author-date --onto commit2^^ commit2^ commit3 &&\n \tgit checkout --theirs foo &&\n-- \n2.29.0.583.g8e3ac41d8f\n"},{"id":"408209","messageId":"20201023072630.GA2918369@coredump.intra.peff.net","threadId":"54489","inReplyTo":"20201023070939.GB2913115@coredump.intra.peff.net","subject":"[PATCH 4/3] am, sequencer: stop parsing our own committer ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T07:26:30Z","receivedAt":"2020-10-23T07:26:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 23, 2020 at 03:09:39AM -0400, Jeff King wrote:\n\n> Commit e8cbe2118a (am: stop exporting GIT_COMMITTER_DATE, 2020-08-17)\n> rewrote the code for setting the committer date to use fmt_ident(),\n> rather than setting an environment variable and letting commit_tree()\n> handle it. But it introduced two bugs:\n> \n>   - we use the author email string instead of the committer email\n> \n>   - when parsing the committer ident, we used the wrong variable to\n>     compute the length of the email, resulting in it always being a\n>     zero-length string\n\nBy the way, I wondered why we needed to do this parsing at all. The\npatch below does this in a much simpler way. It's a little bit ugly, I\nthink, because we have to call getenv() ourselves. But that's the way\nfmt_ident() has always worked. We could probably improve that now that\nit takes a whose_ident flag (before that, it had no idea if we wanted\nauthor or committer ident).\n\nThis is on top of the fixes (but we'd perhaps just want to do those on\n'maint' as the minimal fix).\n\n-- >8 --\nSubject: [PATCH 4/3] am, sequencer: stop parsing our own committer ident\n\nFor the --committer-date-is-author-date option of git-am and git-rebase,\nwe format the committer ident, then re-parse it to find the name and\nemail, and then feed those back to fmt_ident().\n\nWe can simplify this by handling it all at the time of the fmt_ident()\ncall. We pass in the appropriate getenv() results, and if they're not\npresent, then our WANT_COMMITTER_IDENT flag tells fmt_ident() to fill in\nthe appropriate value from the config. Which is exactly what\ngit_committer_ident() was doing under the hood.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c | 19 +++----------------\n sequencer.c  | 28 ++--------------------------\n sequencer.h  |  2 --\n 3 files changed, 5 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4949535a7f..52206bc56b 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -98,8 +98,6 @@ struct am_state {\n \tchar *author_name;\n \tchar *author_email;\n \tchar *author_date;\n-\tchar *committer_name;\n-\tchar *committer_email;\n \tchar *msg;\n \tsize_t msg_len;\n \n@@ -132,8 +130,6 @@ struct am_state {\n  */\n static void am_state_init(struct am_state *state)\n {\n-\tconst char *committer;\n-\tstruct ident_split id;\n \tint gpgsign;\n \n \tmemset(state, 0, sizeof(*state));\n@@ -154,14 +150,6 @@ static void am_state_init(struct am_state *state)\n \n \tif (!git_config_get_bool(\"commit.gpgsign\", &gpgsign))\n \t\tstate->sign_commit = gpgsign ? \"\" : NULL;\n-\n-\tcommitter = git_committer_info(IDENT_STRICT);\n-\tif (split_ident_line(&id, committer, strlen(committer)) < 0)\n-\t\tdie(_(\"invalid committer: %s\"), committer);\n-\tstate->committer_name =\n-\t\txmemdupz(id.name_begin, id.name_end - id.name_begin);\n-\tstate->committer_email =\n-\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_begin);\n }\n \n /**\n@@ -173,8 +161,6 @@ static void am_state_release(struct am_state *state)\n \tfree(state->author_name);\n \tfree(state->author_email);\n \tfree(state->author_date);\n-\tfree(state->committer_name);\n-\tfree(state->committer_email);\n \tfree(state->msg);\n \tstrvec_clear(&state->git_apply_opts);\n }\n@@ -1594,8 +1580,9 @@ static void do_commit(const struct am_state *state)\n \t\t\tIDENT_STRICT);\n \n \tif (state->committer_date_is_author_date)\n-\t\tcommitter = fmt_ident(state->committer_name,\n-\t\t\t\t      state->committer_email, WANT_COMMITTER_IDENT,\n+\t\tcommitter = fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n+\t\t\t\t      getenv(\"GIT_COMMITTER_EMAIL\"),\n+\t\t\t\t      WANT_COMMITTER_IDENT,\n \t\t\t\t      state->ignore_date ? NULL\n \t\t\t\t\t\t\t : state->author_date,\n \t\t\t\t      IDENT_STRICT);\ndiff --git a/sequencer.c b/sequencer.c\nindex d76cbded00..07321a7d95 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -314,8 +314,6 @@ int sequencer_remove_state(struct replay_opts *opts)\n \t\t}\n \t}\n \n-\tfree(opts->committer_name);\n-\tfree(opts->committer_email);\n \tfree(opts->gpg_sign);\n \tfree(opts->strategy);\n \tfor (i = 0; i < opts->xopts_nr; i++)\n@@ -1460,8 +1458,8 @@ static int try_to_commit(struct repository *r,\n \t\t} else {\n \t\t\treset_ident_date();\n \t\t}\n-\t\tcommitter = fmt_ident(opts->committer_name,\n-\t\t\t\t      opts->committer_email,\n+\t\tcommitter = fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n+\t\t\t\t      getenv(\"GIT_COMMITTER_EMAIL\"),\n \t\t\t\t      WANT_COMMITTER_IDENT,\n \t\t\t\t      opts->ignore_date ? NULL : date.buf,\n \t\t\t\t      IDENT_STRICT);\n@@ -4467,22 +4465,6 @@ static int commit_staged_changes(struct repository *r,\n \treturn 0;\n }\n \n-static int init_committer(struct replay_opts *opts)\n-{\n-\tstruct ident_split id;\n-\tconst char *committer;\n-\n-\tcommitter = git_committer_info(IDENT_STRICT);\n-\tif (split_ident_line(&id, committer, strlen(committer)) < 0)\n-\t\treturn error(_(\"invalid committer '%s'\"), committer);\n-\topts->committer_name =\n-\t\txmemdupz(id.name_begin, id.name_end - id.name_begin);\n-\topts->committer_email =\n-\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_begin);\n-\n-\treturn 0;\n-}\n-\n int sequencer_continue(struct repository *r, struct replay_opts *opts)\n {\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n@@ -4494,9 +4476,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \tif (read_populate_opts(opts))\n \t\treturn -1;\n \tif (is_rebase_i(opts)) {\n-\t\tif (opts->committer_date_is_author_date && init_committer(opts))\n-\t\t\treturn -1;\n-\n \t\tif ((res = read_populate_todo(r, &todo_list, opts)))\n \t\t\tgoto release_todo_list;\n \n@@ -5391,9 +5370,6 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \n \tres = -1;\n \n-\tif (opts->committer_date_is_author_date && init_committer(opts))\n-\t\tgoto cleanup;\n-\n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n \t\tgoto cleanup;\n \ndiff --git a/sequencer.h b/sequencer.h\nindex b2a501e445..f925e349c5 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -50,8 +50,6 @@ struct replay_opts {\n \n \tint mainline;\n \n-\tchar *committer_name;\n-\tchar *committer_email;\n \tchar *gpg_sign;\n \tenum commit_msg_cleanup_mode default_msg_cleanup;\n \tint explicit_cleanup;\n-- \n2.29.0.583.g8e3ac41d8f\n\n"},{"id":"408210","messageId":"20201023074510.GB2918369@coredump.intra.peff.net","threadId":"54489","inReplyTo":"20201023072630.GA2918369@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T07:45:10Z","receivedAt":"2020-10-23T07:45:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 23, 2020 at 03:26:30AM -0400, Jeff King wrote:\n\n> By the way, I wondered why we needed to do this parsing at all. The\n> patch below does this in a much simpler way. It's a little bit ugly, I\n> think, because we have to call getenv() ourselves. But that's the way\n> fmt_ident() has always worked. We could probably improve that now that\n> it takes a whose_ident flag (before that, it had no idea if we wanted\n> author or committer ident).\n\nI took a brief look at refactoring fmt_ident() to auto-fill from the\nenvironment variables (patch below). It's mostly sensible for\nname/email, because callers pass either the environment variables or\nsome custom-provided value. But for the date, we sometimes pass NULL to\nmean \"use the current time, even if $GIT_*_DATE is set\". I'm not 100%\nsure that isn't a bug, though.\n\n---\n builtin/am.c |  4 +--\n ident.c      | 47 ++++++++++++++++-------------------\n sequencer.c  |  4 +--\n 3 files changed, 23 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 52206bc56b..9f0d1b4859 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1580,9 +1580,7 @@ static void do_commit(const struct am_state *state)\n \t\t\tIDENT_STRICT);\n \n \tif (state->committer_date_is_author_date)\n-\t\tcommitter = fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n-\t\t\t\t      getenv(\"GIT_COMMITTER_EMAIL\"),\n-\t\t\t\t      WANT_COMMITTER_IDENT,\n+\t\tcommitter = fmt_ident(NULL, NULL, WANT_COMMITTER_IDENT,\n \t\t\t\t      state->ignore_date ? NULL\n \t\t\t\t\t\t\t : state->author_date,\n \t\t\t\t      IDENT_STRICT);\ndiff --git a/ident.c b/ident.c\nindex 6aba4b5cb6..7743c1ed05 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -384,6 +384,12 @@ const char *fmt_ident(const char *name, const char *email,\n \tstruct strbuf *ident = &ident_pool[index];\n \tindex = (index + 1) % ARRAY_SIZE(ident_pool);\n \n+\tif (!email) {\n+\t\tif (whose_ident == WANT_AUTHOR_IDENT)\n+\t\t\temail = getenv(\"GIT_AUTHOR_EMAIL\");\n+\t\telse if (whose_ident == WANT_COMMITTER_IDENT)\n+\t\t\temail = getenv(\"GIT_COMMITTER_EMAIL\");\n+\t}\n \tif (!email) {\n \t\tif (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)\n \t\t\temail = git_author_email.buf;\n@@ -405,6 +411,12 @@ const char *fmt_ident(const char *name, const char *email,\n \n \tif (want_name) {\n \t\tint using_default = 0;\n+\t\tif (!name) {\n+\t\t\tif (whose_ident == WANT_AUTHOR_IDENT)\n+\t\t\t\tname = getenv(\"GIT_AUTHOR_NAME\");\n+\t\t\telse if (whose_ident == WANT_COMMITTER_IDENT)\n+\t\t\t\tname = getenv(\"GIT_COMMITTER_NAME\");\n+\t\t}\n \t\tif (!name) {\n \t\t\tif (whose_ident == WANT_AUTHOR_IDENT && git_author_name.len)\n \t\t\t\tname = git_author_name.buf;\n@@ -449,6 +461,12 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tstrbuf_addch(ident, '>');\n \tif (want_date) {\n \t\tstrbuf_addch(ident, ' ');\n+\t\tif (!date_str) {\n+\t\t\tif (whose_ident == WANT_AUTHOR_IDENT)\n+\t\t\t\tdate_str = getenv(\"GIT_AUTHOR_DATE\");\n+\t\t\telse if (whose_ident == WANT_COMMITTER_IDENT)\n+\t\t\t\tdate_str = getenv(\"GIT_COMMITTER_DATE\");\n+\t\t}\n \t\tif (date_str && date_str[0]) {\n \t\t\tif (parse_date(date_str, ident) < 0)\n \t\t\t\tdie(_(\"invalid date format: %s\"), date_str);\n@@ -462,22 +480,7 @@ const char *fmt_ident(const char *name, const char *email,\n \n const char *fmt_name(enum want_ident whose_ident)\n {\n-\tchar *name = NULL;\n-\tchar *email = NULL;\n-\n-\tswitch (whose_ident) {\n-\tcase WANT_BLANK_IDENT:\n-\t\tbreak;\n-\tcase WANT_AUTHOR_IDENT:\n-\t\tname = getenv(\"GIT_AUTHOR_NAME\");\n-\t\temail = getenv(\"GIT_AUTHOR_EMAIL\");\n-\t\tbreak;\n-\tcase WANT_COMMITTER_IDENT:\n-\t\tname = getenv(\"GIT_COMMITTER_NAME\");\n-\t\temail = getenv(\"GIT_COMMITTER_EMAIL\");\n-\t\tbreak;\n-\t}\n-\treturn fmt_ident(name, email, whose_ident, NULL,\n+\treturn fmt_ident(NULL, NULL, whose_ident, NULL,\n \t\t\tIDENT_STRICT | IDENT_NO_DATE);\n }\n \n@@ -487,11 +490,7 @@ const char *git_author_info(int flag)\n \t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \tif (getenv(\"GIT_AUTHOR_EMAIL\"))\n \t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n-\treturn fmt_ident(getenv(\"GIT_AUTHOR_NAME\"),\n-\t\t\t getenv(\"GIT_AUTHOR_EMAIL\"),\n-\t\t\t WANT_AUTHOR_IDENT,\n-\t\t\t getenv(\"GIT_AUTHOR_DATE\"),\n-\t\t\t flag);\n+\treturn fmt_ident(NULL, NULL, WANT_AUTHOR_IDENT, NULL, flag);\n }\n \n const char *git_committer_info(int flag)\n@@ -500,11 +499,7 @@ const char *git_committer_info(int flag)\n \t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \tif (getenv(\"GIT_COMMITTER_EMAIL\"))\n \t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n-\treturn fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n-\t\t\t getenv(\"GIT_COMMITTER_EMAIL\"),\n-\t\t\t WANT_COMMITTER_IDENT,\n-\t\t\t getenv(\"GIT_COMMITTER_DATE\"),\n-\t\t\t flag);\n+\treturn fmt_ident(NULL, NULL, WANT_COMMITTER_IDENT, NULL, flag);\n }\n \n static int ident_is_sufficient(int user_ident_explicitly_given)\ndiff --git a/sequencer.c b/sequencer.c\nindex 07321a7d95..6a8a5c077b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1458,9 +1458,7 @@ static int try_to_commit(struct repository *r,\n \t\t} else {\n \t\t\treset_ident_date();\n \t\t}\n-\t\tcommitter = fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n-\t\t\t\t      getenv(\"GIT_COMMITTER_EMAIL\"),\n-\t\t\t\t      WANT_COMMITTER_IDENT,\n+\t\tcommitter = fmt_ident(NULL, NULL, WANT_COMMITTER_IDENT,\n \t\t\t\t      opts->ignore_date ? NULL : date.buf,\n \t\t\t\t      IDENT_STRICT);\n \t\tstrbuf_release(&date);\n"},{"id":"408218","messageId":"845a14f1-5ce8-2c2f-a847-6d5d1550ba70@gmail.com","threadId":"54489","inReplyTo":"20201023070843.GA2913115@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-23T10:10:18Z","receivedAt":"2020-10-23T10:10:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nThanks for fixing this embarrassing mistake and strengthening the tests, \nthe first three patches look fine to me. We don't check the committer \nname and email in t4150-am.sh which we probably should do (the rebase \ntests were based on those) though that can wait as some of the rebase \ntests are testing the am implementation.\n\nThanks again\n\nPhillip\n\nOn 23/10/2020 08:08, Jeff King wrote:\n> After running \"rebase --committer-date-is-author-date\", we confirm that\n> the committer date is the same as the author date. However, we don't\n> look at any other parts of the committer ident line to make sure we\n> didn't screw them up. And indeed, there are a few bugs here. Depending\n> on the rebase backend in use, we may accidentally use the author email\n> instead of the committer's, or even an empty string.\n> \n> Let's teach our test_ctime_is_atime helper to check the committer name\n> and email, which reveals several failing tests.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>   t/t3436-rebase-more-options.sh | 14 +++++++-------\n>   1 file changed, 7 insertions(+), 7 deletions(-)\n> \n> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\n> index 996e82787e..6f2f49717b 100755\n> --- a/t/t3436-rebase-more-options.sh\n> +++ b/t/t3436-rebase-more-options.sh\n> @@ -65,31 +65,31 @@ test_expect_success '--ignore-whitespace is remembered when continuing' '\n>   '\n>   \n>   test_ctime_is_atime () {\n> -\tgit log $1 --format=%ai >authortime &&\n> -\tgit log $1 --format=%ci >committertime &&\n> +\tgit log $1 --format=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> %ai\" >authortime &&\n> +\tgit log $1 --format=\"%cn <%ce> %ci\" >committertime &&\n>   \ttest_cmp authortime committertime\n>   }\n>   \n> -test_expect_success '--committer-date-is-author-date works with apply backend' '\n> +test_expect_failure '--committer-date-is-author-date works with apply backend' '\n>   \tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n>   \tgit rebase --apply --committer-date-is-author-date HEAD^ &&\n>   \ttest_ctime_is_atime -1\n>   '\n>   \n> -test_expect_success '--committer-date-is-author-date works with merge backend' '\n> +test_expect_failure '--committer-date-is-author-date works with merge backend' '\n>   \tGIT_AUTHOR_DATE=\"@1234 +0300\" git commit --amend --reset-author &&\n>   \tgit rebase -m --committer-date-is-author-date HEAD^ &&\n>   \ttest_ctime_is_atime -1\n>   '\n>   \n> -test_expect_success '--committer-date-is-author-date works with rebase -r' '\n> +test_expect_failure '--committer-date-is-author-date works with rebase -r' '\n>   \tgit checkout side &&\n>   \tGIT_AUTHOR_DATE=\"@1234 +0300\" git merge --no-ff commit3 &&\n>   \tgit rebase -r --root --committer-date-is-author-date &&\n>   \ttest_ctime_is_atime\n>   '\n>   \n> -test_expect_success '--committer-date-is-author-date works when forking merge' '\n> +test_expect_failure '--committer-date-is-author-date works when forking merge' '\n>   \tgit checkout side &&\n>   \tGIT_AUTHOR_DATE=\"@1234 +0300\" git merge --no-ff commit3 &&\n>   \tPATH=\"./test-bin:$PATH\" git rebase -r --root --strategy=test \\\n> @@ -145,7 +145,7 @@ test_expect_success '--reset-author-date works with rebase -r' '\n>   \ttest_atime_is_ignored\n>   '\n>   \n> -test_expect_success '--reset-author-date with --committer-date-is-author-date works' '\n> +test_expect_failure '--reset-author-date with --committer-date-is-author-date works' '\n>   \ttest_must_fail git rebase -m --committer-date-is-author-date \\\n>   \t\t--reset-author-date --onto commit2^^ commit2^ commit3 &&\n>   \tgit checkout --theirs foo &&\n> \n\n"},{"id":"408234","messageId":"9acc1d9f-7646-3ef9-b744-e47c4871873d@gmail.com","threadId":"54489","inReplyTo":"20201023072630.GA2918369@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-23T14:06:01Z","receivedAt":"2020-10-23T14:06:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 23/10/2020 08:26, Jeff King wrote:\n> On Fri, Oct 23, 2020 at 03:09:39AM -0400, Jeff King wrote:\n> \n>> Commit e8cbe2118a (am: stop exporting GIT_COMMITTER_DATE, 2020-08-17)\n>> rewrote the code for setting the committer date to use fmt_ident(),\n>> rather than setting an environment variable and letting commit_tree()\n>> handle it. But it introduced two bugs:\n>>\n>>    - we use the author email string instead of the committer email\n>>\n>>    - when parsing the committer ident, we used the wrong variable to\n>>      compute the length of the email, resulting in it always being a\n>>      zero-length string\n> \n> By the way, I wondered why we needed to do this parsing at all. The\n> patch below does this in a much simpler way. It's a little bit ugly, I\n> think, because we have to call getenv() ourselves. But that's the way\n> fmt_ident() has always worked. We could probably improve that now that\n> it takes a whose_ident flag (before that, it had no idea if we wanted\n> author or committer ident).\n> \n> This is on top of the fixes (but we'd perhaps just want to do those on\n> 'maint' as the minimal fix).\n> \n> -- >8 --\n> Subject: [PATCH 4/3] am, sequencer: stop parsing our own committer ident\n> \n> For the --committer-date-is-author-date option of git-am and git-rebase,\n> we format the committer ident, then re-parse it to find the name and\n> email, and then feed those back to fmt_ident().\n> \n> We can simplify this by handling it all at the time of the fmt_ident()\n> call. We pass in the appropriate getenv() results, and if they're not\n> present, then our WANT_COMMITTER_IDENT flag tells fmt_ident() to fill in\n> the appropriate value from the config. Which is exactly what\n> git_committer_ident() was doing under the hood.\n\nMakes sense and it simplifies the code nicely\n\nThanks\n\nPhillip\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>   builtin/am.c | 19 +++----------------\n>   sequencer.c  | 28 ++--------------------------\n>   sequencer.h  |  2 --\n>   3 files changed, 5 insertions(+), 44 deletions(-)\n> \n> diff --git a/builtin/am.c b/builtin/am.c\n> index 4949535a7f..52206bc56b 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -98,8 +98,6 @@ struct am_state {\n>   \tchar *author_name;\n>   \tchar *author_email;\n>   \tchar *author_date;\n> -\tchar *committer_name;\n> -\tchar *committer_email;\n>   \tchar *msg;\n>   \tsize_t msg_len;\n>   \n> @@ -132,8 +130,6 @@ struct am_state {\n>    */\n>   static void am_state_init(struct am_state *state)\n>   {\n> -\tconst char *committer;\n> -\tstruct ident_split id;\n>   \tint gpgsign;\n>   \n>   \tmemset(state, 0, sizeof(*state));\n> @@ -154,14 +150,6 @@ static void am_state_init(struct am_state *state)\n>   \n>   \tif (!git_config_get_bool(\"commit.gpgsign\", &gpgsign))\n>   \t\tstate->sign_commit = gpgsign ? \"\" : NULL;\n> -\n> -\tcommitter = git_committer_info(IDENT_STRICT);\n> -\tif (split_ident_line(&id, committer, strlen(committer)) < 0)\n> -\t\tdie(_(\"invalid committer: %s\"), committer);\n> -\tstate->committer_name =\n> -\t\txmemdupz(id.name_begin, id.name_end - id.name_begin);\n> -\tstate->committer_email =\n> -\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_begin);\n>   }\n>   \n>   /**\n> @@ -173,8 +161,6 @@ static void am_state_release(struct am_state *state)\n>   \tfree(state->author_name);\n>   \tfree(state->author_email);\n>   \tfree(state->author_date);\n> -\tfree(state->committer_name);\n> -\tfree(state->committer_email);\n>   \tfree(state->msg);\n>   \tstrvec_clear(&state->git_apply_opts);\n>   }\n> @@ -1594,8 +1580,9 @@ static void do_commit(const struct am_state *state)\n>   \t\t\tIDENT_STRICT);\n>   \n>   \tif (state->committer_date_is_author_date)\n> -\t\tcommitter = fmt_ident(state->committer_name,\n> -\t\t\t\t      state->committer_email, WANT_COMMITTER_IDENT,\n> +\t\tcommitter = fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n> +\t\t\t\t      getenv(\"GIT_COMMITTER_EMAIL\"),\n> +\t\t\t\t      WANT_COMMITTER_IDENT,\n>   \t\t\t\t      state->ignore_date ? NULL\n>   \t\t\t\t\t\t\t : state->author_date,\n>   \t\t\t\t      IDENT_STRICT);\n> diff --git a/sequencer.c b/sequencer.c\n> index d76cbded00..07321a7d95 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -314,8 +314,6 @@ int sequencer_remove_state(struct replay_opts *opts)\n>   \t\t}\n>   \t}\n>   \n> -\tfree(opts->committer_name);\n> -\tfree(opts->committer_email);\n>   \tfree(opts->gpg_sign);\n>   \tfree(opts->strategy);\n>   \tfor (i = 0; i < opts->xopts_nr; i++)\n> @@ -1460,8 +1458,8 @@ static int try_to_commit(struct repository *r,\n>   \t\t} else {\n>   \t\t\treset_ident_date();\n>   \t\t}\n> -\t\tcommitter = fmt_ident(opts->committer_name,\n> -\t\t\t\t      opts->committer_email,\n> +\t\tcommitter = fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n> +\t\t\t\t      getenv(\"GIT_COMMITTER_EMAIL\"),\n>   \t\t\t\t      WANT_COMMITTER_IDENT,\n>   \t\t\t\t      opts->ignore_date ? NULL : date.buf,\n>   \t\t\t\t      IDENT_STRICT);\n> @@ -4467,22 +4465,6 @@ static int commit_staged_changes(struct repository *r,\n>   \treturn 0;\n>   }\n>   \n> -static int init_committer(struct replay_opts *opts)\n> -{\n> -\tstruct ident_split id;\n> -\tconst char *committer;\n> -\n> -\tcommitter = git_committer_info(IDENT_STRICT);\n> -\tif (split_ident_line(&id, committer, strlen(committer)) < 0)\n> -\t\treturn error(_(\"invalid committer '%s'\"), committer);\n> -\topts->committer_name =\n> -\t\txmemdupz(id.name_begin, id.name_end - id.name_begin);\n> -\topts->committer_email =\n> -\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_begin);\n> -\n> -\treturn 0;\n> -}\n> -\n>   int sequencer_continue(struct repository *r, struct replay_opts *opts)\n>   {\n>   \tstruct todo_list todo_list = TODO_LIST_INIT;\n> @@ -4494,9 +4476,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n>   \tif (read_populate_opts(opts))\n>   \t\treturn -1;\n>   \tif (is_rebase_i(opts)) {\n> -\t\tif (opts->committer_date_is_author_date && init_committer(opts))\n> -\t\t\treturn -1;\n> -\n>   \t\tif ((res = read_populate_todo(r, &todo_list, opts)))\n>   \t\t\tgoto release_todo_list;\n>   \n> @@ -5391,9 +5370,6 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>   \n>   \tres = -1;\n>   \n> -\tif (opts->committer_date_is_author_date && init_committer(opts))\n> -\t\tgoto cleanup;\n> -\n>   \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n>   \t\tgoto cleanup;\n>   \n> diff --git a/sequencer.h b/sequencer.h\n> index b2a501e445..f925e349c5 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -50,8 +50,6 @@ struct replay_opts {\n>   \n>   \tint mainline;\n>   \n> -\tchar *committer_name;\n> -\tchar *committer_email;\n>   \tchar *gpg_sign;\n>   \tenum commit_msg_cleanup_mode default_msg_cleanup;\n>   \tint explicit_cleanup;\n> \n"},{"id":"408243","messageId":"xmqqmu0dhsoz.fsf@gitster.c.googlers.com","threadId":"54489","inReplyTo":"20201023070747.GA2198273@coredump.intra.peff.net","subject":"Re: committer-date-is-author-date flag removes email in \"Commit\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-23T15:23:40Z","receivedAt":"2020-10-23T15:23:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Oct 23, 2020 at 11:18:51AM +0530, VenomVendor wrote:\n>\n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>> * Create empty repo using `git init`\n>> * Make few commits, at least two\n>> * execute `git log --format=fuller`\n>> * Notice the log, with \"Author\", \"AuthorDate\", \"Commit\", \"CommitDate\"\n>> * Note, \"Commit\"\n>> * execute `git rebase --committer-date-is-author-date HEAD~1`\n>> * execute `git log --format=fuller`\n>> * Note, email from \"Commit\" is empty <>\n>\n> Thanks for a clear report. I was able to easily reproduce the problem.\n> There are actually two related bugs here, and they're both regressions\n> in v2.29.0.\n>\n>   [1/3]: t3436: check --committer-date-is-author-date result more carefully\n>   [2/3]: am: fix broken email with --committer-date-is-author-date\n>   [3/3]: rebase: fix broken email with --committer-date-is-author-date\n\nThanks for taking quick care of this.  It counts as an embarrasing\nbrown-paper-bag bug; it is a bit surprising that nobody noticed it\nwhile the original change was discussed.\n\nI wonder if we even needed to do the original change to begin with\n(stopping to export means not giving information to the hooks), but\nthat is a separate matter.\n\nWill take a look and queue.  Thanks.\n\n>\n>  builtin/am.c                   | 4 ++--\n>  sequencer.c                    | 2 +-\n>  t/t3436-rebase-more-options.sh | 4 ++--\n>  3 files changed, 5 insertions(+), 5 deletions(-)\n>\n> -Peff\n"},{"id":"408262","messageId":"X5MS5l2bbRczcdgF@nand.local","threadId":"54489","inReplyTo":"20201023074510.GB2918369@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-23T17:29:10Z","receivedAt":"2020-10-23T17:29:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 23, 2020 at 03:45:10AM -0400, Jeff King wrote:\n> On Fri, Oct 23, 2020 at 03:26:30AM -0400, Jeff King wrote:\n>\n> > By the way, I wondered why we needed to do this parsing at all. The\n> > patch below does this in a much simpler way. It's a little bit ugly, I\n> > think, because we have to call getenv() ourselves. But that's the way\n> > fmt_ident() has always worked. We could probably improve that now that\n> > it takes a whose_ident flag (before that, it had no idea if we wanted\n> > author or committer ident).\n>\n> I took a brief look at refactoring fmt_ident() to auto-fill from the\n> environment variables (patch below). It's mostly sensible for\n> name/email, because callers pass either the environment variables or\n> some custom-provided value. But for the date, we sometimes pass NULL to\n> mean \"use the current time, even if $GIT_*_DATE is set\". I'm not 100%\n> sure that isn't a bug, though.\n\nThat seems like an area where further investigation would be helpful.\nIn the meantime (especially if this is something that Junio wants to\nqueue and call v2.29.2), I'd be happy with the four patches you posted\n(but waiting on this one below).\n\nThanks for investigating.\n\nThanks,\nTaylor\n"},{"id":"408263","messageId":"58e83ad2-14a2-5ace-c1fd-8224625a0f97@gmail.com","threadId":"54489","inReplyTo":"xmqqmu0dhsoz.fsf@gitster.c.googlers.com","subject":"Re: committer-date-is-author-date flag removes email in \"Commit\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-23T17:32:51Z","receivedAt":"2020-10-23T17:32:57Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 23/10/2020 16:23, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> On Fri, Oct 23, 2020 at 11:18:51AM +0530, VenomVendor wrote:\n>>\n>>> What did you do before the bug happened? (Steps to reproduce your issue)\n>>> * Create empty repo using `git init`\n>>> * Make few commits, at least two\n>>> * execute `git log --format=fuller`\n>>> * Notice the log, with \"Author\", \"AuthorDate\", \"Commit\", \"CommitDate\"\n>>> * Note, \"Commit\"\n>>> * execute `git rebase --committer-date-is-author-date HEAD~1`\n>>> * execute `git log --format=fuller`\n>>> * Note, email from \"Commit\" is empty <>\n>>\n>> Thanks for a clear report. I was able to easily reproduce the problem.\n>> There are actually two related bugs here, and they're both regressions\n>> in v2.29.0.\n>>\n>>    [1/3]: t3436: check --committer-date-is-author-date result more carefully\n>>    [2/3]: am: fix broken email with --committer-date-is-author-date\n>>    [3/3]: rebase: fix broken email with --committer-date-is-author-date\n> \n> Thanks for taking quick care of this.  It counts as an embarrasing\n> brown-paper-bag bug; it is a bit surprising that nobody noticed it\n> while the original change was discussed.\n\nIt is indeed embarrassing. That change only appeared in the last round \nof patches and unfortunately I think most people had stopped looking \nthem it by then as their comments had been addressed in previous rounds.\n\n> I wonder if we even needed to do the original change to begin with\n> (stopping to export means not giving information to the hooks), but\n> that is a separate matter.\n\nI think the main motivation was to stop polluting the environment of \nexec commands\n\nBest Wishes\n\nPhillip\n\n> Will take a look and queue.  Thanks.\n> \n>>\n>>   builtin/am.c                   | 4 ++--\n>>   sequencer.c                    | 2 +-\n>>   t/t3436-rebase-more-options.sh | 4 ++--\n>>   3 files changed, 5 insertions(+), 5 deletions(-)\n>>\n>> -Peff\n\n"},{"id":"408266","messageId":"xmqqh7qkhlgw.fsf@gitster.c.googlers.com","threadId":"54489","inReplyTo":"58e83ad2-14a2-5ace-c1fd-8224625a0f97@gmail.com","subject":"Re: committer-date-is-author-date flag removes email in \"Commit\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-23T17:59:43Z","receivedAt":"2020-10-23T17:59:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>> I wonder if we even needed to do the original change to begin with\n>> (stopping to export means not giving information to the hooks), but\n>> that is a separate matter.\n>\n> I think the main motivation was to stop polluting the environment of\n> exec commands\n\nAs long as the information that used to be exported can be learned\nby the hook script when/if it wants to (how?  read the original\ncommit object with \"git show -s --format=...\"?  How would it know\nwhich one is the original commit?), it is fine.\n\nIf not, a script that does not want the exported environment\nvariable can easily ignore it (just \"unset\" it) but to a script that\nwanted to learn the information that used to be in the environment\nvariable, it's not \"stop polluting\" but \"lose information\", and that\nis where my earlier \"I wonder\" comes from.\n\n\n"},{"id":"408404","messageId":"nycvar.QRO.7.76.6.2010261722230.56@tvgsbejvaqbjf.bet","threadId":"54489","inReplyTo":"20201023074510.GB2918369@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-26T16:25:01Z","receivedAt":"2020-10-26T16:25:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 23 Oct 2020, Jeff King wrote:\n\n> diff --git a/ident.c b/ident.c\n> index 6aba4b5cb6..7743c1ed05 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -384,6 +384,12 @@ const char *fmt_ident(const char *name, const char *email,\n>  \tstruct strbuf *ident = &ident_pool[index];\n>  \tindex = (index + 1) % ARRAY_SIZE(ident_pool);\n>\n> +\tif (!email) {\n> +\t\tif (whose_ident == WANT_AUTHOR_IDENT)\n> +\t\t\temail = getenv(\"GIT_AUTHOR_EMAIL\");\n> +\t\telse if (whose_ident == WANT_COMMITTER_IDENT)\n> +\t\t\temail = getenv(\"GIT_COMMITTER_EMAIL\");\n\nI *guess* that this is a strict improvement, calling `getenv()` much\ncloser to the time the value is actually used (and hence avoiding the\nproblem where pointers returned by `getenv()` get stale due to environment\nchanges).\n\nThanks,\nDscho\n"},{"id":"408423","messageId":"xmqqft60gbci.fsf@gitster.c.googlers.com","threadId":"54489","inReplyTo":"20201023072630.GA2918369@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-26T17:12:45Z","receivedAt":"2020-10-26T17:12:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> By the way, I wondered why we needed to do this parsing at all. The\n> patch below does this in a much simpler way. It's a little bit ugly, I\n> think, because we have to call getenv() ourselves. But that's the way\n> fmt_ident() has always worked. We could probably improve that now that\n> it takes a whose_ident flag (before that, it had no idea if we wanted\n> author or committer ident).\n>\n> This is on top of the fixes (but we'd perhaps just want to do those on\n> 'maint' as the minimal fix).\n\nThis could be the nicest step in the whole series, but let's leave\nit out of the branch meant for 'maint'.\n\nThanks.\n"},{"id":"408488","messageId":"20201027072309.GE3005508@coredump.intra.peff.net","threadId":"54489","inReplyTo":"nycvar.QRO.7.76.6.2010261722230.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-27T07:23:09Z","receivedAt":"2020-10-27T07:23:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 26, 2020 at 05:25:01PM +0100, Johannes Schindelin wrote:\n\n> On Fri, 23 Oct 2020, Jeff King wrote:\n> \n> > diff --git a/ident.c b/ident.c\n> > index 6aba4b5cb6..7743c1ed05 100644\n> > --- a/ident.c\n> > +++ b/ident.c\n> > @@ -384,6 +384,12 @@ const char *fmt_ident(const char *name, const char *email,\n> >  \tstruct strbuf *ident = &ident_pool[index];\n> >  \tindex = (index + 1) % ARRAY_SIZE(ident_pool);\n> >\n> > +\tif (!email) {\n> > +\t\tif (whose_ident == WANT_AUTHOR_IDENT)\n> > +\t\t\temail = getenv(\"GIT_AUTHOR_EMAIL\");\n> > +\t\telse if (whose_ident == WANT_COMMITTER_IDENT)\n> > +\t\t\temail = getenv(\"GIT_COMMITTER_EMAIL\");\n> \n> I *guess* that this is a strict improvement, calling `getenv()` much\n> closer to the time the value is actually used (and hence avoiding the\n> problem where pointers returned by `getenv()` get stale due to environment\n> changes).\n\nI don't think it changes much in practice. Most of the callers are\npassing the values directly in to this function, and there's not much\nthat happens between the function starting and these calls.\n\nThe more worrisome stretch is that we likely call strbuf functions while\nholding on to a getenv() pointer. And those potentially do things like\nxmalloc(), which looks at GIT_ALLOC_LIMIT, etc. But though POSIX\npromises only one getenv() result at a time, we definitely don't adhere\nto that (after all, we routinely pass the results of three separate\ngetenv() calls to fmt_ident!). As you well know, because mingw_getenv()\nhas a circular buffer hack to deal with this. :)\n\nSo it certainly isn't making anything worse, but I'd be surprised if it\nactually helped at all.\n\n-Peff\n"},{"id":"408489","messageId":"20201027072445.GF3005508@coredump.intra.peff.net","threadId":"54489","inReplyTo":"xmqqft60gbci.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-27T07:24:45Z","receivedAt":"2020-10-27T07:24:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 26, 2020 at 10:12:45AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > By the way, I wondered why we needed to do this parsing at all. The\n> > patch below does this in a much simpler way. It's a little bit ugly, I\n> > think, because we have to call getenv() ourselves. But that's the way\n> > fmt_ident() has always worked. We could probably improve that now that\n> > it takes a whose_ident flag (before that, it had no idea if we wanted\n> > author or committer ident).\n> >\n> > This is on top of the fixes (but we'd perhaps just want to do those on\n> > 'maint' as the minimal fix).\n> \n> This could be the nicest step in the whole series, but let's leave\n> it out of the branch meant for 'maint'.\n\nI think that's sensible. Looks like you queued it separately already, so\nthank you. :)\n\n-Peff\n"}]}