From: Phillip Wood Date: Fri, 26 Jun 2020 14:38:38 GMT Subject: Re: [PATCH v5 4/5] rebase -i: support --ignore-date Message-ID: In-Reply-To: <20200626140854.GC12504@danh.dev> Hi Danh On 26/06/2020 15:09, Đoàn Trần Công Danh wrote: > On 2020-06-26 10:55:27+0100, Phillip Wood wrote: >> From: Phillip Wood >> >> Rebase is implemented with two different backends - 'apply' and >> 'merge' each of which support a different set of options. In >> particular the apply backend supports a number of options implemented >> by 'git am' that are not implemented in the merge backend. This means >> that the available options are different depending on which backend is >> used which is confusing. This patch adds support for the --ignore-date >> option to the merge backend. This option uses the current time as the >> author date rather than reusing the original author date when >> rewriting commits. We take care to handle the combination of >> --ignore-date and --committer-date-is-author-date in the same way as >> the apply backend. >> >> Original-patch-by: Rohit Ashiwal >> Signed-off-by: Phillip Wood >> --- >> Documentation/git-rebase.txt | 7 +++-- >> builtin/rebase.c | 13 +++++--- >> sequencer.c | 50 ++++++++++++++++++++++++++++-- >> sequencer.h | 1 + >> t/t3436-rebase-more-options.sh | 56 ++++++++++++++++++++++++++++++++++ >> 5 files changed, 117 insertions(+), 10 deletions(-) >> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt >> index dfa70263e6..e2717e20e6 100644 >> --- a/Documentation/git-rebase.txt >> +++ b/Documentation/git-rebase.txt >> @@ -450,8 +450,9 @@ See also INCOMPATIBLE OPTIONS below. >> date. This option implies --force-rebase. >> >> --ignore-date:: >> - This flag is passed to 'git am' to change the author date >> - of each rebased commit (see linkgit:git-am[1]). >> + Instead of using the author date of the original commit, use >> + the current time as the author date of the rebased commit. This >> + option implies `--force-rebase`. >> + >> See also INCOMPATIBLE OPTIONS below. >> >> @@ -589,7 +590,6 @@ INCOMPATIBLE OPTIONS >> The following options: >> >> * --apply >> - * --ignore-date >> * --whitespace >> * -C >> >> @@ -617,6 +617,7 @@ In addition, the following pairs of options are incompatible: >> * --preserve-merges and --empty= >> * --preserve-merges and --ignore-whitespace >> * --preserve-merges and --committer-date-is-author-date >> + * --preserve-merges and --ignore-date >> * --keep-base and --onto >> * --keep-base and --root >> >> diff --git a/builtin/rebase.c b/builtin/rebase.c >> index a7c3d5c92b..890dd4c588 100644 >> --- a/builtin/rebase.c >> +++ b/builtin/rebase.c >> @@ -89,6 +89,7 @@ struct rebase_options { >> char *gpg_sign_opt; >> int autostash; >> int committer_date_is_author_date; >> + int ignore_date; >> char *cmd; >> int allow_empty_message; >> int rebase_merges, rebase_cousins; >> @@ -127,6 +128,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) >> replay.reschedule_failed_exec = opts->reschedule_failed_exec; >> replay.committer_date_is_author_date = >> opts->committer_date_is_author_date; >> + replay.ignore_date = opts->ignore_date; >> replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt); >> replay.strategy = opts->strategy; >> >> @@ -1503,8 +1505,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >> 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_BOOL(0, "ignore-date", &options.ignore_date, >> + "ignore author date and use current date"), > > Nit: The options' description is subjected to l10n. > > s/".*"/N_(&)/ Well spotted, thanks >> OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"), >> N_("passed to 'git apply'"), 0), >> OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace, >> @@ -1797,13 +1799,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) >> + if (options.committer_date_is_author_date || options.ignore_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, "--ignore-date") || >> - !strcmp(option, "--whitespace=fix") || >> + if (!strcmp(option, "--whitespace=fix") || >> !strcmp(option, "--whitespace=strip")) >> allow_preemptive_ff = 0; >> else if (skip_prefix(option, "-C", &p)) { >> @@ -1862,6 +1863,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >> if (options.committer_date_is_author_date) >> argv_array_push(&options.git_am_opts, >> "--committer-date-is-author-date"); >> + if (options.ignore_date) >> + argv_array_push(&options.git_am_opts, "--ignore-date"); >> } else if (ignore_whitespace) { >> string_list_append (&strategy_options, >> "ignore-space-change"); >> diff --git a/sequencer.c b/sequencer.c >> index 29f6d1bc39..f8e1e38623 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -150,6 +150,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete") >> */ >> 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_ignore_date, "rebase-merge/ignore_date") >> 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") >> @@ -889,6 +890,24 @@ static const char *author_date_from_env_array(const struct argv_array *env) >> BUG("GIT_AUTHOR_DATE missing from author script"); >> } >> >> +/* Construct a free()able author string with current time as the author date */ >> +static char *ignore_author_date(const char *author) >> +{ >> + int len = strlen(author); >> + struct ident_split ident; >> + struct strbuf new_author = STRBUF_INIT; >> + >> + if (split_ident_line(&ident, author, len) < 0) { >> + error(_("malformed ident line '%s'"), author); >> + return NULL; >> + } >> + >> + len = ident.mail_end - ident.name_begin + 1; >> + strbuf_addf(&new_author, "%.*s ", len, ident.name_begin); > > I wonder if we can do this instead: > > strbuf_add(&new_author, ident.name_begin, len); There is a space at the end of the format string, which I think is needed to separate the name and email from the date we add with datestamp() below Best Wishes Phillip > Quick skim through the code and nothing crashes when make test with: > > CFLAGS="-fstack-clash-protection -D_FORTIFY_SOURCE=2" > > I think it's OK to be changed. > >> + datestamp(&new_author); >> + return strbuf_detach(&new_author, NULL); >> +} >> + >