Hi Elijah On 07/04/2020 16:05, Elijah Newren wrote: > On Tue, Apr 7, 2020 at 7:11 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: >> >> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> >> >> Johanathan Nieder reported that `rebase --committer-date-is-author-date` >> failed with the error "invalid date format: @@2592000 +0000" when the >> backend was switched to use the sequencer [1]. This is because when we >> read the date from the author script file is already prefixed by '@' so >> we do not need to add one. This did not show up in our tests >> because once match_object_header_date() fails to match because of the >> extra '@' parse_date_basic() strips the prefix and a date with more than >> 8 digits is parsed as a number of seconds by match_digit(). >> >> While fixing this I also noticed that we were not setting >> GIT_COMMITTER_DATE in the environment when forking `git merge`. This was >> untested. The tests we did have used commits where the author date was >> the same as the current value $GIT_COMMITTER_DATE in the environment so >> they did not test that we were actually using the author date when >> creating the commit. >> >> As we have already read GIT_AUTHOR_DATE into cmd.env_array we now use >> that rather than re-reading the author-script file. I've moved the code >> that handles opts->ignore date so that all the code setting the child >> environment is together and changed it so we no longer set >> GIT_COMMITTER_DATE twice when --ignore-date is combined with >> --committer-date-is-author-date. >> >> [1] https://lore.kernel.org/git/<20200110231436.GA24315@xxxxxxxxxx> >> >> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> >> --- >> sequencer.c | 68 ++++++++++--------------- >> t/t3433-rebase-options-compatibility.sh | 29 +++++++++-- >> 2 files changed, 52 insertions(+), 45 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index 763ccbbc45..56f6e49289 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -880,17 +880,6 @@ static char *get_author(const char *message) >> return NULL; >> } >> >> -/* Returns a "date" string that needs to be free()'d by the caller */ >> -static char *read_author_date_or_null(void) >> -{ >> - char *date; >> - >> - if (read_author_script(rebase_path_author_script(), >> - NULL, NULL, &date, 0)) >> - return NULL; >> - return date; >> -} >> - >> /* Construct a free()able author string with current time as the author date */ >> static char *ignore_author_date(const char *author) >> { >> @@ -909,16 +898,20 @@ static char *ignore_author_date(const char *author) >> return strbuf_detach(&new_author, NULL); >> } >> >> -static void push_dates(struct child_process *child, int change_committer_date) >> +static const char *author_date_from_env_array(const struct argv_array *env) >> { >> - time_t now = time(NULL); >> - struct strbuf date = STRBUF_INIT; >> + int i; >> + const char *date; >> >> - strbuf_addf(&date, "@%"PRIuMAX, (uintmax_t)now); >> - argv_array_pushf(&child->env_array, "GIT_AUTHOR_DATE=%s", date.buf); >> - if (change_committer_date) >> - argv_array_pushf(&child->env_array, "GIT_COMMITTER_DATE=%s", date.buf); >> - strbuf_release(&date); >> + for (i = 0; i < env->argc; i++) >> + if (skip_prefix(env->argv[i], >> + "GIT_AUTHOR_DATE=", &date)) >> + return date; > > Is there any risk that GIT_AUTHOR_DATE=<somedate> will appear twice in > @child->env_array ? If there is, should we read through the whole > list to make sure we take the last one? read_author_script() errors out if GIT_AUTHOR_DATE is missing or given more than once Best Wishes Phillip > >> + /* >> + * If GIT_AUTHOR_DATE is missing we should have already errored out when >> + * reading the script >> + */ >> + BUG("GIT_AUTHOR_DATE missing from author script"); >> } >> >> static const char staged_changes_advice[] = >> @@ -980,31 +973,19 @@ static int run_git_commit(struct repository *r, >> >> cmd.git_cmd = 1; >> >> - if (opts->committer_date_is_author_date) { >> - int res = -1; >> - struct strbuf datebuf = STRBUF_INIT; >> - char *date = read_author_date_or_null(); >> - >> - if (!date) >> - return -1; >> - >> - strbuf_addf(&datebuf, "@%s", date); >> - res = setenv("GIT_COMMITTER_DATE", >> - opts->ignore_date ? "" : datebuf.buf, 1); >> - >> - strbuf_release(&datebuf); >> - free(date); >> - >> - if (res) >> - return -1; >> - } >> - >> if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) { >> const char *gpg_opt = gpg_sign_opt_quoted(opts); >> >> return error(_(staged_changes_advice), >> gpg_opt, gpg_opt); >> } >> + if (opts->committer_date_is_author_date) >> + argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s", >> + opts->ignore_date ? >> + "" : >> + author_date_from_env_array(&cmd.env_array)); >> + if (opts->ignore_date) >> + argv_array_push(&cmd.env_array, "GIT_AUTHOR_DATE="); >> >> argv_array_push(&cmd.args, "commit"); >> >> @@ -1014,8 +995,6 @@ static int run_git_commit(struct repository *r, >> argv_array_push(&cmd.args, "--amend"); >> if (opts->gpg_sign) >> argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign); >> - if (opts->ignore_date) >> - push_dates(&cmd, opts->committer_date_is_author_date); >> if (defmsg) >> argv_array_pushl(&cmd.args, "-F", defmsg, NULL); >> else if (!(flags & EDIT_MSG)) >> @@ -3615,6 +3594,13 @@ static int do_merge(struct repository *r, >> ret = error(_(staged_changes_advice), gpg_opt, gpg_opt); >> goto leave_merge; >> } >> + if (opts->committer_date_is_author_date) >> + argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s", >> + opts->ignore_date ? >> + "" : >> + author_date_from_env_array(&cmd.env_array)); >> + if (opts->ignore_date) >> + argv_array_push(&cmd.env_array, "GIT_AUTHOR_DATE="); >> >> cmd.git_cmd = 1; >> argv_array_push(&cmd.args, "merge"); >> @@ -3635,8 +3621,6 @@ static int do_merge(struct repository *r, >> argv_array_push(&cmd.args, git_path_merge_msg(r)); >> if (opts->gpg_sign) >> argv_array_push(&cmd.args, opts->gpg_sign); >> - if (opts->ignore_date) >> - push_dates(&cmd, opts->committer_date_is_author_date); >> >> /* Add the tips to be merged */ >> for (j = to_merge; j; j = j->next) >> diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh >> index 132f577fc9..8247d01442 100755 >> --- a/t/t3433-rebase-options-compatibility.sh >> +++ b/t/t3433-rebase-options-compatibility.sh >> @@ -71,15 +71,15 @@ test_expect_success '--ignore-whitespace works with interactive backend' ' >> ' >> >> test_expect_success '--committer-date-is-author-date works with am backend' ' >> - git commit --amend && >> + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && >> git rebase --committer-date-is-author-date HEAD^ && >> git log -1 --pretty="format:%ai" >authortime && >> git log -1 --pretty="format:%ci" >committertime && >> test_cmp authortime committertime >> ' >> >> test_expect_success '--committer-date-is-author-date works with interactive backend' ' >> - git commit --amend && >> + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author && >> git rebase -i --committer-date-is-author-date HEAD^ && >> git log -1 --pretty="format:%ai" >authortime && >> git log -1 --pretty="format:%ci" >committertime && >> @@ -88,13 +88,36 @@ test_expect_success '--committer-date-is-author-date works with interactive back >> >> test_expect_success '--committer-date-is-author-date works with rebase -r' ' >> git checkout side && >> - git merge --no-ff commit3 && >> + GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 && >> git rebase -r --root --committer-date-is-author-date && >> git log --pretty="format:%ai" >authortime && >> git log --pretty="format:%ci" >committertime && >> test_cmp authortime committertime >> ' >> >> +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 && >> + git rebase -r --root --strategy=resolve --committer-date-is-author-date && >> + git log --pretty="format:%ai" >authortime && >> + git log --pretty="format:%ci" >committertime && >> + test_cmp authortime committertime >> + >> +' >> + >> +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' ' >> + git checkout commit2 && >> + GIT_AUTHOR_DATE="@1980 +0000" git commit --amend --only --reset-author && >> + git log -1 --format=%at HEAD >expect && >> + test_must_fail git rebase -i --committer-date-is-author-date \ >> + --onto HEAD^^ HEAD^ && >> + echo resolved > foo && >> + git add foo && >> + git rebase --continue && >> + git log -1 --format=%ct HEAD >actual && >> + test_cmp expect actual >> +' >> + >> # Checking for +0000 in author time is enough since default >> # timezone is UTC, but the timezone used while committing >> # sets to +0530. >> -- >> 2.26.0 >>
