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

[Rohit Ashiwal <rohit.ashiwal265@gmail.com>]

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