Re: [GSoC][PATCH v2 2/2] rebase -i: support --committer-date-is-author-date

[Junio C Hamano <gitster@pobox.com>]

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.