* committer-date-is-author-date flag removes email in "Commit" @ 2020-10-23 5:48 VenomVendor 2020-10-23 7:07 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: VenomVendor @ 2020-10-23 5:48 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: committer-date-is-author-date flag removes email in "Commit" 2020-10-23 5:48 committer-date-is-author-date flag removes email in "Commit" VenomVendor @ 2020-10-23 7:07 ` Jeff King 2020-10-23 7:08 ` [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully Jeff King ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Jeff King @ 2020-10-23 7:07 UTC (permalink / raw) To: VenomVendor; +Cc: Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully 2020-10-23 7:07 ` Jeff King @ 2020-10-23 7:08 ` Jeff King 2020-10-23 10:10 ` Phillip Wood 2020-10-23 7:09 ` [PATCH 2/3] am: fix broken email with --committer-date-is-author-date Jeff King ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2020-10-23 7:08 UTC (permalink / raw) To: VenomVendor; +Cc: Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully 2020-10-23 7:08 ` [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully Jeff King @ 2020-10-23 10:10 ` Phillip Wood 0 siblings, 0 replies; 17+ messages in thread From: Phillip Wood @ 2020-10-23 10:10 UTC (permalink / raw) To: Jeff King, VenomVendor; +Cc: Junio C Hamano, Phillip Wood, git 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 && > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] am: fix broken email with --committer-date-is-author-date 2020-10-23 7:07 ` Jeff King 2020-10-23 7:08 ` [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully Jeff King @ 2020-10-23 7:09 ` Jeff King 2020-10-23 7:26 ` [PATCH 4/3] am, sequencer: stop parsing our own committer ident Jeff King 2020-10-23 7:10 ` [PATCH 3/3] rebase: fix broken email with --committer-date-is-author-date Jeff King 2020-10-23 15:23 ` committer-date-is-author-date flag removes email in "Commit" Junio C Hamano 3 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2020-10-23 7:09 UTC (permalink / raw) To: VenomVendor; +Cc: Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/3] am, sequencer: stop parsing our own committer ident 2020-10-23 7:09 ` [PATCH 2/3] am: fix broken email with --committer-date-is-author-date Jeff King @ 2020-10-23 7:26 ` Jeff King 2020-10-23 7:45 ` Jeff King ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Jeff King @ 2020-10-23 7:26 UTC (permalink / raw) To: VenomVendor; +Cc: Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident 2020-10-23 7:26 ` [PATCH 4/3] am, sequencer: stop parsing our own committer ident Jeff King @ 2020-10-23 7:45 ` Jeff King 2020-10-23 17:29 ` Taylor Blau 2020-10-26 16:25 ` Johannes Schindelin 2020-10-23 14:06 ` Phillip Wood 2020-10-26 17:12 ` Junio C Hamano 2 siblings, 2 replies; 17+ messages in thread From: Jeff King @ 2020-10-23 7:45 UTC (permalink / raw) To: VenomVendor; +Cc: Junio C Hamano, Phillip Wood, git 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); ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident 2020-10-23 7:45 ` Jeff King @ 2020-10-23 17:29 ` Taylor Blau 2020-10-26 16:25 ` Johannes Schindelin 1 sibling, 0 replies; 17+ messages in thread From: Taylor Blau @ 2020-10-23 17:29 UTC (permalink / raw) To: Jeff King; +Cc: VenomVendor, Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident 2020-10-23 7:45 ` Jeff King 2020-10-23 17:29 ` Taylor Blau @ 2020-10-26 16:25 ` Johannes Schindelin 2020-10-27 7:23 ` Jeff King 1 sibling, 1 reply; 17+ messages in thread From: Johannes Schindelin @ 2020-10-26 16:25 UTC (permalink / raw) To: Jeff King; +Cc: VenomVendor, Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident 2020-10-26 16:25 ` Johannes Schindelin @ 2020-10-27 7:23 ` Jeff King 0 siblings, 0 replies; 17+ messages in thread From: Jeff King @ 2020-10-27 7:23 UTC (permalink / raw) To: Johannes Schindelin; +Cc: VenomVendor, Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident 2020-10-23 7:26 ` [PATCH 4/3] am, sequencer: stop parsing our own committer ident Jeff King 2020-10-23 7:45 ` Jeff King @ 2020-10-23 14:06 ` Phillip Wood 2020-10-26 17:12 ` Junio C Hamano 2 siblings, 0 replies; 17+ messages in thread From: Phillip Wood @ 2020-10-23 14:06 UTC (permalink / raw) To: Jeff King, VenomVendor; +Cc: Junio C Hamano, Phillip Wood, git 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; > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident 2020-10-23 7:26 ` [PATCH 4/3] am, sequencer: stop parsing our own committer ident Jeff King 2020-10-23 7:45 ` Jeff King 2020-10-23 14:06 ` Phillip Wood @ 2020-10-26 17:12 ` Junio C Hamano 2020-10-27 7:24 ` Jeff King 2 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-10-26 17:12 UTC (permalink / raw) To: Jeff King; +Cc: VenomVendor, Phillip Wood, git 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident 2020-10-26 17:12 ` Junio C Hamano @ 2020-10-27 7:24 ` Jeff King 0 siblings, 0 replies; 17+ messages in thread From: Jeff King @ 2020-10-27 7:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: VenomVendor, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] rebase: fix broken email with --committer-date-is-author-date 2020-10-23 7:07 ` Jeff King 2020-10-23 7:08 ` [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully Jeff King 2020-10-23 7:09 ` [PATCH 2/3] am: fix broken email with --committer-date-is-author-date Jeff King @ 2020-10-23 7:10 ` Jeff King 2020-10-23 15:23 ` committer-date-is-author-date flag removes email in "Commit" Junio C Hamano 3 siblings, 0 replies; 17+ messages in thread From: Jeff King @ 2020-10-23 7:10 UTC (permalink / raw) To: VenomVendor; +Cc: Junio C Hamano, Phillip Wood, git 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: committer-date-is-author-date flag removes email in "Commit" 2020-10-23 7:07 ` Jeff King ` (2 preceding siblings ...) 2020-10-23 7:10 ` [PATCH 3/3] rebase: fix broken email with --committer-date-is-author-date Jeff King @ 2020-10-23 15:23 ` Junio C Hamano 2020-10-23 17:32 ` Phillip Wood 3 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-10-23 15:23 UTC (permalink / raw) To: Jeff King; +Cc: VenomVendor, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: committer-date-is-author-date flag removes email in "Commit" 2020-10-23 15:23 ` committer-date-is-author-date flag removes email in "Commit" Junio C Hamano @ 2020-10-23 17:32 ` Phillip Wood 2020-10-23 17:59 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Phillip Wood @ 2020-10-23 17:32 UTC (permalink / raw) To: Junio C Hamano, Jeff King; +Cc: VenomVendor, Phillip Wood, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: committer-date-is-author-date flag removes email in "Commit" 2020-10-23 17:32 ` Phillip Wood @ 2020-10-23 17:59 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2020-10-23 17:59 UTC (permalink / raw) To: Phillip Wood; +Cc: Jeff King, VenomVendor, Phillip Wood, git 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-10-27 7:24 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-23 5:48 committer-date-is-author-date flag removes email in "Commit" VenomVendor 2020-10-23 7:07 ` Jeff King 2020-10-23 7:08 ` [PATCH 1/3] t3436: check --committer-date-is-author-date result more carefully Jeff King 2020-10-23 10:10 ` Phillip Wood 2020-10-23 7:09 ` [PATCH 2/3] am: fix broken email with --committer-date-is-author-date Jeff King 2020-10-23 7:26 ` [PATCH 4/3] am, sequencer: stop parsing our own committer ident Jeff King 2020-10-23 7:45 ` Jeff King 2020-10-23 17:29 ` Taylor Blau 2020-10-26 16:25 ` Johannes Schindelin 2020-10-27 7:23 ` Jeff King 2020-10-23 14:06 ` Phillip Wood 2020-10-26 17:12 ` Junio C Hamano 2020-10-27 7:24 ` Jeff King 2020-10-23 7:10 ` [PATCH 3/3] rebase: fix broken email with --committer-date-is-author-date Jeff King 2020-10-23 15:23 ` committer-date-is-author-date flag removes email in "Commit" Junio C Hamano 2020-10-23 17:32 ` Phillip Wood 2020-10-23 17:59 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).