Hi Phillip
On Sat, 20 Jul 2019 15:56:50 +0100 Phillip Wood <[email protected]>
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
