Hi dscho
On 29/05/2020 03:52, Johannes Schindelin wrote:
> Hi Phillip,
>
> On Wed, 27 May 2020, Phillip Wood wrote:
>
>> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>>
>> As part of the on-going effort to retire the apply rebase backend teach
>> the merge backend how to handle --committer-date-is-author-date.
>
> I forgot to mention this in my reply to the first patch in this series: it
> strikes me as a mis-characterization that the `apply` backend is about to
> be retired. What _is_ true is that the `merge` backend is the default
> backend, and what is even more true is that it is cumbersome if certain
> `rebase` options cannot be combined because they are only implemented by
> one of both rebase backends.
I'm not trying to imply that the apply backend is about to be retired
but it is the long term plan to get rid of it isn't it?
>
>>
>> Original-patch-by: Rohit Ashiwal <rohit.ashiwal265@xxxxxxxxx>
>> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>> ---
>> Documentation/git-rebase.txt | 10 +++--
>> builtin/rebase.c | 16 ++++---
>> sequencer.c | 59 +++++++++++++++++++++++++-
>> sequencer.h | 1 +
>> t/t3422-rebase-incompatible-options.sh | 1 -
>> t/t3436-rebase-more-options.sh | 56 ++++++++++++++++++++++++
>> 6 files changed, 133 insertions(+), 10 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 5d8e117276..357cd6acf3 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -89,6 +89,7 @@ struct rebase_options {
>> int ignore_whitespace;
>> char *gpg_sign_opt;
>> int autostash;
>> + int committer_date_is_author_date;
>> char *cmd;
>> int allow_empty_message;
>> int rebase_merges, rebase_cousins;
>> @@ -126,6 +127,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>> replay.quiet = !(opts->flags & REBASE_NO_QUIET);
>> replay.verbose = opts->flags & REBASE_VERBOSE;
>> replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>> + replay.committer_date_is_author_date =
>> + opts->committer_date_is_author_date;
>> replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
>> replay.strategy = opts->strategy;
>>
>> @@ -1003,6 +1006,8 @@ static int run_am(struct rebase_options *opts)
>>
>> if (opts->ignore_whitespace)
>> argv_array_push(&am.args, "--ignore-whitespace");
>> + if (opts->committer_date_is_author_date)
>> + argv_array_push(&opts->git_am_opts, "--committer-date-is-author-date");
>
> As before, I would rather see this in `cmd__rebase()` rahn in `run_am()`.
Right I'll take a look at that
>> if (opts->action && !strcmp("continue", opts->action)) {
>> argv_array_push(&am.args, "--resolved");
>> argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
>> @@ -1507,9 +1512,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>> PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
>> OPT_BOOL(0, "signoff", &options.signoff,
>> N_("add a Signed-off-by: line to each commit")),
>> - OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
>> - &options.git_am_opts, NULL,
>> - N_("passed to 'git am'"), PARSE_OPT_NOARG),
>> + OPT_BOOL(0, "committer-date-is-author-date",
>> + &options.committer_date_is_author_date,
>> + N_("make committer date match author date")),
>> OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts, NULL,
>> N_("passed to 'git am'"), PARSE_OPT_NOARG),
>> OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
>> @@ -1804,11 +1809,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>> options.autosquash) {
>> allow_preemptive_ff = 0;
>> }
>> + if (options.committer_date_is_author_date)
>> + options.flags |= REBASE_FORCE;
>>
>> for (i = 0; i < options.git_am_opts.argc; i++) {
>> const char *option = options.git_am_opts.argv[i], *p;
>> - if (!strcmp(option, "--committer-date-is-author-date") ||
>> - !strcmp(option, "--ignore-date") ||
>> + if (!strcmp(option, "--ignore-date") ||
>> !strcmp(option, "--whitespace=fix") ||
>> !strcmp(option, "--whitespace=strip"))
>> allow_preemptive_ff = 0;
>> diff --git a/sequencer.c b/sequencer.c
>> index 6fd2674632..8826c6325b 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -149,6 +149,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
>> * command-line.
>> */
>> static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
>> +static GIT_PATH_FUNC(rebase_path_cdate_is_adate, "rebase-merge/cdate_is_adate")
>> static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
>> static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
>> static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
>> @@ -872,6 +873,22 @@ static char *get_author(const char *message)
>> return NULL;
>> }
>>
>> +static const char *author_date_from_env_array(const struct argv_array *env)
>> +{
>> + int i;
>> + const char *date;
>> +
>> + for (i = 0; i < env->argc; i++)
>> + if (skip_prefix(env->argv[i],
>> + "GIT_AUTHOR_DATE=", &date))
>> + return date;
>> + /*
>> + * 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[] =
>> N_("you have staged changes in your working tree\n"
>> "If these changes are meant to be squashed into the previous commit, run:\n"
>> @@ -938,6 +955,10 @@ static int run_git_commit(struct repository *r,
>> gpg_opt, gpg_opt);
>> }
>>
>> + if (opts->committer_date_is_author_date)
>> + argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s",
>> + author_date_from_env_array(&cmd.env_array));
>> +
>> argv_array_push(&cmd.args, "commit");
>>
>> if (!(flags & VERIFY_MSG))
>> @@ -1349,6 +1370,30 @@ static int try_to_commit(struct repository *r,
>> commit_list_insert(current_head, &parents);
>> }
>>
>> + if (opts->committer_date_is_author_date) {
>> + int len = strlen(author);
>
> Given that `len` is used only once, is this local variable really
> necessary? You could just as well pass `(int)strlen(author)` in the
> `split_ident_line()` call below.
I wonder if it makes the `split_ident_line()` line too long, I'll take a
look.
>> + struct ident_split ident;
>> + struct strbuf date = STRBUF_INIT;
>> +
>> + if (split_ident_line(&ident, author, len) < 0) {
>> + res = error(_("malformed ident line '%s'"), author);
>> + goto out;
>> + }
>> + if (!ident.date_begin) {
>> + res = error(_("corrupted author without date information"));
>> + goto out;
>> + }
>> +
>> + strbuf_addf(&date, "@%.*s %.*s",
>> + (int)(ident.date_end - ident.date_begin), ident.date_begin,
>> + (int)(ident.tz_end - ident.tz_begin), ident.tz_begin);
>> + res = setenv("GIT_COMMITTER_DATE", date.buf, 1);
>> + strbuf_release(&date);
>
> It might be easier to read if it was written this way:
>
> if (split_ident_line(...) < 0)
> res = ...
> else if (!ident.date_begin)
> res = ...
> else {
> strbuf_addf(...)
> res = ...
> strbuf_release(&date);
> }
>
> if (res)
> goto out;
I'm not sure. The current style of "if there's an error goto out" makes
the control flow clear without having to reason ones way through a
series of else ifs
>> +
>> + if (res)
>> + goto out;
>> + }
>> +
>> if (write_index_as_tree(&tree, r->index, r->index_file, 0, NULL)) {
>> res = error(_("git write-tree failed to write a tree"));
>> goto out;
>> @@ -2532,6 +2577,11 @@ static int read_populate_opts(struct replay_opts *opts)
>> opts->signoff = 1;
>> }
>>
>> + if (file_exists(rebase_path_cdate_is_adate())) {
>> + opts->allow_ff = 0;
>> + opts->committer_date_is_author_date = 1;
>> + }
>> +
>> if (file_exists(rebase_path_reschedule_failed_exec()))
>> opts->reschedule_failed_exec = 1;
>>
>> @@ -2622,6 +2672,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
>> write_file(rebase_path_drop_redundant_commits(), "%s", "");
>> if (opts->keep_redundant_commits)
>> write_file(rebase_path_keep_redundant_commits(), "%s", "");
>> + if (opts->committer_date_is_author_date)
>> + write_file(rebase_path_cdate_is_adate(), "%s", "");
>> if (opts->reschedule_failed_exec)
>> write_file(rebase_path_reschedule_failed_exec(), "%s", "");
>>
>> @@ -3542,6 +3594,10 @@ static int do_merge(struct repository *r,
>> goto leave_merge;
>> }
>>
>> + if (opts->committer_date_is_author_date)
>> + argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s",
>> + author_date_from_env_array(&cmd.env_array));
>> +
>> cmd.git_cmd = 1;
>> argv_array_push(&cmd.args, "merge");
>> argv_array_push(&cmd.args, "-s");
>> @@ -3819,7 +3875,8 @@ static int pick_commits(struct repository *r,
>> setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>> if (opts->allow_ff)
>> assert(!(opts->signoff || opts->no_commit ||
>> - opts->record_origin || opts->edit));
>> + opts->record_origin || opts->edit ||
>> + opts->committer_date_is_author_date));
>> if (read_and_refresh_cache(r, opts))
>> return -1;
>>
>> diff --git a/sequencer.h b/sequencer.h
>> index 0bee85093e..4ab94119ae 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -45,6 +45,7 @@ struct replay_opts {
>> int verbose;
>> int quiet;
>> int reschedule_failed_exec;
>> + int committer_date_is_author_date;
>>
>> int mainline;
>>
>> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
>> index 55ca46786d..c8234062c6 100755
>> --- a/t/t3422-rebase-incompatible-options.sh
>> +++ b/t/t3422-rebase-incompatible-options.sh
>> @@ -61,7 +61,6 @@ test_rebase_am_only () {
>> }
>>
>> test_rebase_am_only --whitespace=fix
>> -test_rebase_am_only --committer-date-is-author-date
>> test_rebase_am_only -C4
>>
>> test_expect_success REBASE_P '--preserve-merges incompatible with --signoff' '
>> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
>> index fb5e747e86..7e68fb9753 100755
>> --- a/t/t3436-rebase-more-options.sh
>> +++ b/t/t3436-rebase-more-options.sh
>> @@ -9,6 +9,9 @@ test_description='tests to ensure compatibility between am and interactive backe
>>
>> . "$TEST_DIRECTORY"/lib-rebase.sh
>>
>> +GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
>> +export GIT_AUTHOR_DATE
>> +
>> # This is a special case in which both am and interactive backends
>> # provide the same output. It was done intentionally because
>> # both the backends fall short of optimal behaviour.
>> @@ -28,8 +31,13 @@ test_expect_success 'setup' '
>> EOF
>> git commit -am "update file" &&
>> git tag side &&
>> + test_commit commit1 foo foo1 &&
>> + test_commit commit2 foo foo2 &&
>> + test_commit commit3 foo foo3 &&
>>
>> git checkout --orphan master &&
>> + git rm --cached foo &&
>> + rm foo &&
>> sed -e "s/^|//" >file <<-\EOF &&
>> |line 1
>> | line 2
>> @@ -78,6 +86,54 @@ test_expect_success '--ignore-whitespace is remembered when continuing' '
>> test_cmp expect file
>> '
>>
>> +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^ &&
>> + git log -1 --pretty=%ai >authortime &&
>> + git log -1 --pretty=%ci >committertime &&
>> + test_cmp authortime committertime
>
> This seems to be a repeated pattern. To dry it up, it would make sense to
> do this:
>
> test_ctime_is_atime () {
> git log -1 --pretty=%ai >authortime &&
> git log -1 --pretty=%ci >committertime &&
> test_cmp authortime committertime
> }
Good suggestion (though we need to parametrize the -1 so we can use it
with the rebase --root cases)
Thanks
Phillip
>
> Ciao,
> Dscho
>
>> +'
>> +
>> +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^ &&
>> + git log -1 --pretty=%ai >authortime &&
>> + git log -1 --pretty=%ci >committertime &&
>> + test_cmp authortime committertime
>> +'
>> +
>> +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 &&
>> + git log --pretty=%ai >authortime &&
>> + git log --pretty=%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=%ai >authortime &&
>> + git log --pretty=%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 -m --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
>> +'
>> +
>> # This must be the last test in this file
>> test_expect_success '$EDITOR and friends are unchanged' '
>> test_editor_unchanged
>> --
>> 2.26.2
>>
>>
