[GSoC,v2,2/2] rebase -i: support --committer-date-is-author-date
diff mbox series

Message ID 20190718190314.20549-3-rohit.ashiwal265@gmail.com
State New
Headers show
Series
  • rebase -i: support --committer-date-is-author-date
Related show

Commit Message

Rohit Ashiwal July 18, 2019, 7:03 p.m. UTC
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 <rohit.ashiwal265@gmail.com>
---
 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(-)

Comments

Junio C Hamano July 19, 2019, 10:36 p.m. UTC | #1
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

> @@ -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;

This is needed here, because am-opts no longer has the
committer-date-is-author-date passed through with the
parse_options() call in cmd_rebase(), which makes sense.

> diff --git a/sequencer.c b/sequencer.c
> index a2d7b0925e..a65f01a422 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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);
> +			}

Hmph, are we sure that author-script is always available at this
point so that a call to read_author_date_or_die() is safe?  There
are three callers to the run_git_commit() function and I am not sure
if codepaths that reach all of them prepared the input to the
read_author_script() helper.

> @@ -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);
> +	}
>  	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);
> +	}
> +

In the same function, we seem to be grabbing the author ident by
calling get_author(message), where the message is an in-core copy of
a commit object, which suggests me that we may not necessarily be
working with the on-disk information read_author_date_or_die() is
prepared to deal with.  Are we sure we have the needed information
on disk so that read_author_date_or_die() will read the correct
information from the disk?

> @@ -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;
> +			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", "");

These two are propagating the option across rebase invocations, and
they should be correct (as correct as how correctly other options
are propagated ;-)).

> @@ -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);

This codepath does have read_env_script() which reads from the
author script an earlier call to write_author_script() would have
left on disk (at least when do_merge() is called with 'commit'
argument, anyway), so we should be able to read from it (or the
existing code is already buggy---and this patch just makes it
slightly worse by depending on a wrong assumption even more).  OK.
Phillip Wood July 20, 2019, 2:56 p.m. UTC | #2
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 <rohit.ashiwal265@gmail.com>
> ---
>   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 <email> 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
>
Rohit Ashiwal July 23, 2019, 7:57 p.m. UTC | #3
Hi Phillip

On Sat, 20 Jul 2019 15:56:50 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
> [...]
> 
> > @@ -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.

Oh! Yes, I did it for the completeness. Let's add the flag while we
still have that _rebase--interactive_ command hanging out with us.

> [...]
> 
> > +	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.

Yes, we can through an error and continue, but then the user will
see the unchanged author date which is against his / her will but
it will not crash the program at least.

> [...]
> 
> > +	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.

Ok, I'll have to change the code according to what Junio suggested.
Let's see how this area will look after that.

> [...]
> 
> > +		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.

Are you suggesting to modify this assert() call (in pick_commits())?

    if (opts->allow_ff)
        assert(!(opts->signoff || opts->no_commit ||
                opts->record_origin || opts->edit));

Thanks
Rohit
Phillip Wood July 24, 2019, 1:33 p.m. UTC | #4
Hi Rohit

On 23/07/2019 20:57, Rohit Ashiwal wrote:
> Hi Phillip
> 
> On Sat, 20 Jul 2019 15:56:50 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> [...]
>>
>>> @@ -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.
> 
> Oh! Yes, I did it for the completeness. Let's add the flag while we
> still have that _rebase--interactive_ command hanging out with us.
> 
>> [...]
>>
>>> +	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.
> 
> Yes, we can through an error and continue, but then the user will
> see the unchanged author date which is against his / her will but
> it will not crash the program at least.

I dont think it should continue, the error should be propagated up to 
cmd_rebase() (In your patch I think one of the context lines in 
run_git_commit() shows this happening)

> 
>> [...]
>>
>>> +	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.
> 
> Ok, I'll have to change the code according to what Junio suggested.
> Let's see how this area will look after that.
> 
>> [...]
>>
>>> +		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.
> 
> Are you suggesting to modify this assert() call (in pick_commits())?
> 
>      if (opts->allow_ff)
>          assert(!(opts->signoff || opts->no_commit ||
>                  opts->record_origin || opts->edit));

Yes I think it should check for opts->committer_date_is_author_date here

Best Wishes

Phillip

> Thanks
> Rohit
>
Rohit Ashiwal Aug. 2, 2019, 8:57 p.m. UTC | #5
Hi Junio

On Fri, 19 Jul 2019 15:36:15 -0700 Junio C Hamano <gitster@pobox.com> wrote:
>
> [...]
> Hmph, are we sure that author-script is always available at this
> point so that a call to read_author_date_or_die() is safe?  There
> are three callers to the run_git_commit() function and I am not sure
> if codepaths that reach all of them prepared the input to the
> read_author_script() helper.

Functions do_pick_commit() and do_merge() always write author_script
before calling run_git_commit(), so,  we are sure to find it on disk.
Furthermore, commit_staged_changes() only calls run_git_commit() when
rebase is stopped (by merge conflicts or 'edit' commands), so we are
sure that the previous invocation of git must have saved author_script.
In all other cases, we fail as we should.

> [...]
>
> In the same function, we seem to be grabbing the author ident by
> calling get_author(message), where the message is an in-core copy of
> a commit object, which suggests me that we may not necessarily be
> working with the on-disk information read_author_date_or_die() is
> prepared to deal with.  Are we sure we have the needed information
> on disk so that read_author_date_or_die() will read the correct
> information from the disk?

Yes, in that case, we can re-parse the author date and re-set the
env variable while we are in this if branch. I believe that the
information stored on the disk is same as the information retrieved
through get_author(), please confirm or disprove this.

> [...]

With all this knowledge, I'll polish the patch and re-send it.

Thanks
Rohit

Patch
diff mbox series

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")),
 		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"));
+
+	return date;
+}
+
 /* Read author-script and return an ident line (author <email> 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);
+	}
 	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;
+			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