From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_NONE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id A50111F461 for ; Sat, 20 Jul 2019 14:56:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725996AbfGTO44 (ORCPT ); Sat, 20 Jul 2019 10:56:56 -0400 Received: from mail-wr1-f66.google.com ([209.85.221.66]:39875 "EHLO mail-wr1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725792AbfGTO44 (ORCPT ); Sat, 20 Jul 2019 10:56:56 -0400 Received: by mail-wr1-f66.google.com with SMTP id x4so34947049wrt.6 for ; Sat, 20 Jul 2019 07:56:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=reply-to:subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=JPMa9rqHAfmDgDCUegqBeuWtufAoa1DG2cKShZWMFhI=; b=jPSIUA/LFD3RH0qlTTQi9U5S6SiivGos++g8zAeLw6YxM7zn5B4Z9IhhWYWD+528la kKpQHsirFvpDsM6hYyR/DxREImAKFpqfUHS4ilJdrCPIeT6fJfmdbJkXs9wPJJ95Ixig AAntFpo5c5EuZU4v6zrWPDyOBXcjamtE5YjKYtaiUvEL7CCSzkCe9yT2+WO2qLv15Wt8 FHDTflDrocKPHZMIaZfHbzyfaHsQYT2LJx8x+KJpnsUAPnhxDj1xGMv0rMA48ggwhDTG opMAOWi4w4eLANO0fiXasb+0aZA8RvHAD2m2LwrOf9sU3GsBqPxs/jVyvYt3w363iyHv 9bxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:reply-to:subject:to:cc:references:from :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=JPMa9rqHAfmDgDCUegqBeuWtufAoa1DG2cKShZWMFhI=; b=JMnQwXllwDh+CGuU8Tevx7MG3RGHHKDT3VaYHdUZPvmXpCV9MkzpyWbJjvP4nvas4X rzca6F5miM/uU90nNFNNM/tWiDcqFq2fXYu86XKp54QWVc5Xxux9gAk/pkQ+f7U1i5kT rbC5mVe/v1QYgPHi2n49JkzCgDU84/ftvGsU68+abpk2+55zVgxtUS5VErk9lauDmItQ 30lFSEATDlAmrjbo8B8PyQF40GskkK/lnEwGLxOnIwxyAkxaak6rfcy0bfc/oOat0Cvo tKqgrdwKhYp54lMrg2MBdaQCs498nvzdvRH/EftYCgPb3HTLiHSXGlMlkPotuS55dKBm ZKpA== X-Gm-Message-State: APjAAAX8Ts6JWDwTyxC8lgmxEg+QFUO1otzDG5xvhlQOBd6PoiqiLqUx NgHC+dRCYXPADaqSxjwCW/8= X-Google-Smtp-Source: APXvYqxiQc2tFaLbq2k9rtMqp877vMbFsyLbuA9hjgEvAMHhFYeJ6tOLQidOlHkkJF5zIkJtu89Yfg== X-Received: by 2002:adf:f3d1:: with SMTP id g17mr33138917wrp.38.1563634612723; Sat, 20 Jul 2019 07:56:52 -0700 (PDT) Received: from [192.168.2.240] (host-89-242-178-164.as13285.net. [89.242.178.164]) by smtp.gmail.com with ESMTPSA id e6sm34237269wrw.23.2019.07.20.07.56.51 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Sat, 20 Jul 2019 07:56:52 -0700 (PDT) Reply-To: phillip.wood@dunelm.org.uk Subject: Re: [GSoC][PATCH v2 2/2] rebase -i: support --committer-date-is-author-date To: Rohit Ashiwal Cc: Dscho , Git Mailing List , Junio , Martin , Thomas , Elijah References: <20190712185357.21211-1-rohit.ashiwal265@gmail.com> <20190718190314.20549-1-rohit.ashiwal265@gmail.com> <20190718190314.20549-3-rohit.ashiwal265@gmail.com> From: Phillip Wood Message-ID: <7f53b294-57f7-77bf-ca0a-e9621227377c@gmail.com> Date: Sat, 20 Jul 2019 15:56:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190718190314.20549-3-rohit.ashiwal265@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi Rohit It's good to see this patch reducing the differences between the rebase backends. On 18/07/2019 20:03, Rohit Ashiwal wrote: > rebase am already has this flag to "lie" about the committer date > by changing it to the author date. Let's add the same for > interactive machinery. > > Signed-off-by: Rohit Ashiwal > --- > Documentation/git-rebase.txt | 7 ++-- > builtin/rebase.c | 23 ++++++++++--- > sequencer.c | 46 ++++++++++++++++++++++++- > sequencer.h | 1 + > t/t3422-rebase-incompatible-options.sh | 1 - > t/t3431-rebase-options-compatibility.sh | 21 +++++++++++ > 6 files changed, 90 insertions(+), 9 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index eda52ed824..ddd111de69 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -383,8 +383,12 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`. > See also INCOMPATIBLE OPTIONS below. > > --committer-date-is-author-date:: > + Instead of recording the time the rebased commits are > + created as the committer date, reuse the author date > + as the committer date. This implies --force-rebase. > + > --ignore-date:: > - These flags are passed to 'git am' to easily change the dates > + This flag is passed to 'git am' to change the author date > of the rebased commits (see linkgit:git-am[1]). > + > See also INCOMPATIBLE OPTIONS below. > @@ -522,7 +526,6 @@ INCOMPATIBLE OPTIONS > > The following options: > > - * --committer-date-is-author-date > * --ignore-date > * --whitespace > * -C > diff --git a/builtin/rebase.c b/builtin/rebase.c > index afe376c3fe..c317fbe53c 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -82,6 +82,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; > @@ -113,6 +114,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) > replay.allow_empty_message = opts->allow_empty_message; > 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; > if (opts->strategy_opts) > @@ -467,6 +470,9 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) > OPT_BOOL(0, "autosquash", &opts.autosquash, > N_("move commits that begin with squash!/fixup!")), > OPT_BOOL(0, "signoff", &opts.signoff, N_("sign commits")), > + OPT_BOOL(0, "committer-date-is-author-date", > + &opts.committer_date_is_author_date, > + N_("make committer date match author date")), I guess it's good to do this for completeness but does rebase--preserver-merges.sh support --committer-date-is-author-date? It is the only caller of rebase--interactive I think so would be the only user of this code. > OPT_BIT('v', "verbose", &opts.flags, > N_("display a diffstat of what changed upstream"), > REBASE_NO_QUIET | REBASE_VERBOSE | REBASE_DIFFSTAT), > @@ -534,6 +540,9 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) > warning(_("--[no-]rebase-cousins has no effect without " > "--rebase-merges")); > > + if (opts.committer_date_is_author_date) > + opts.flags |= REBASE_FORCE; > + > return !!run_rebase_interactive(&opts, command); > } > > @@ -972,6 +981,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"); > if (opts->action && !strcmp("continue", opts->action)) { > argv_array_push(&am.args, "--resolved"); > argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg); > @@ -1419,9 +1430,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"), > @@ -1688,10 +1699,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > state_dir_base, cmd_live_rebase, buf.buf); > } > > + 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")) > options.flags |= REBASE_FORCE; > diff --git a/sequencer.c b/sequencer.c > index a2d7b0925e..a65f01a422 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -147,6 +147,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") > @@ -876,6 +877,18 @@ 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_die(void) > +{ > + char *date; > + > + if (read_author_script(rebase_path_author_script(), > + NULL, NULL, &date, 0)) > + die(_("failed to read author date")); Can we have this return an error please - we try quite hard in the sequencer not to die in library code. > + > + return date; > +} > + > /* Read author-script and return an ident line (author timestamp) */ > static const char *read_author_ident(struct strbuf *buf) > { > @@ -986,10 +999,17 @@ static int run_git_commit(struct repository *r, > > if (res <= 0) > res = error_errno(_("could not read '%s'"), defmsg); > - else > + else { > + if (opts->committer_date_is_author_date) { > + char *date = read_author_date_or_die(); > + setenv("GIT_COMMITTER_DATE", date, 1); > + free(date); > + } > + > res = commit_tree(msg.buf, msg.len, cache_tree_oid, > NULL, &root_commit, author, > opts->gpg_sign); > + } > > strbuf_release(&msg); > strbuf_release(&script); > @@ -1019,6 +1039,11 @@ 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->committer_date_is_author_date) { > + char *date = read_author_date_or_die(); > + argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s", date); > + free(date); > + } It's a shame to be doing this twice is slightly different ways in the same function (and again in try_to_commit() but I don't think that can be avoided as not all callers of run_git_commit() go through try_to_commit()). As I think the child inherits the current environment modified by cmd.env_array we could just call setenv() at the top of the function. It would be worth looking to see if it would be simpler to do the setenv() call in the loop that picks the commits, then we would avoid having to do it in do_merge() and try_to_commit() separately. > if (defmsg) > argv_array_pushl(&cmd.args, "-F", defmsg, NULL); > else if (!(flags & EDIT_MSG)) > @@ -1467,6 +1492,12 @@ static int try_to_commit(struct repository *r, > > reset_ident_date(); > > + if (opts->committer_date_is_author_date) { > + char *date = read_author_date_or_die(); > + setenv("GIT_COMMITTER_DATE", date, 1); > + free(date); > + } > + > if (commit_tree_extended(msg->buf, msg->len, &tree, parents, > oid, author, opts->gpg_sign, extra)) { > res = error(_("failed to write commit object")); > @@ -2538,6 +2569,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; This is safe as we don't save the state of allow_ff for rebases so it wont be overridden later. It would be an idea to add to the checks in the assert() at the beginning of pick_commits() no we have another option that implies --force-rebase. Best Wishes Phillip > + opts->committer_date_is_author_date = 1; > + } > + > if (file_exists(rebase_path_reschedule_failed_exec())) > opts->reschedule_failed_exec = 1; > > @@ -2620,6 +2656,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, > write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign); > if (opts->signoff) > write_file(rebase_path_signoff(), "--signoff\n"); > + 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", ""); > > @@ -3437,6 +3475,12 @@ 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->committer_date_is_author_date) { > + char *date = read_author_date_or_die(); > + argv_array_pushf(&cmd.env_array, > + "GIT_COMMITTER_DATE=%s", date); > + free(date); > + } > > /* Add the tips to be merged */ > for (j = to_merge; j; j = j->next) > diff --git a/sequencer.h b/sequencer.h > index 303047a133..0cfe184fc2 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -44,6 +44,7 @@ struct replay_opts { > int quiet; > int reschedule_failed_exec; > int ignore_whitespace; > + 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 4342f79eea..7402f7e3da 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/t3431-rebase-options-compatibility.sh b/t/t3431-rebase-options-compatibility.sh > index f38ae6f5fc..c3f7f6d5d0 100755 > --- a/t/t3431-rebase-options-compatibility.sh > +++ b/t/t3431-rebase-options-compatibility.sh > @@ -7,6 +7,9 @@ test_description='tests to ensure compatibility between am and interactive backe > > . ./test-lib.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 outputs. It was done intentionally because > # --ignore-whitespace both the backends fall short of optimal > @@ -63,4 +66,22 @@ test_expect_success '--ignore-whitespace works with interactive backend' ' > test_cmp expect file > ' > > +test_expect_success '--committer-date-is-author-date works with am backend' ' > + git rebase -f HEAD^ && > + git rebase --committer-date-is-author-date HEAD^ && > + git cat-file commit HEAD | sed -e "/^\$/q" >head && > + sed -ne "/^author /s/.*> //p" head >authortime && > + sed -ne "/^committer /s/.*> //p" head >committertime && > + test_cmp authortime committertime > +' > + > +test_expect_success '--committer-date-is-author-date works with interactive backend' ' > + git rebase -f HEAD^ && > + git rebase -i --committer-date-is-author-date HEAD^ && > + git cat-file commit HEAD | sed -e "/^\$/q" >head && > + sed -ne "/^author /s/.*> //p" head >authortime && > + sed -ne "/^committer /s/.*> //p" head >committertime && > + test_cmp authortime committertime > +' > + > test_done >