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