Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
>> +static void push_dates(struct child_process *child)
>> +{
>> + time_t now = time(NULL);
>> + struct strbuf date = STRBUF_INIT;
>> +
>> + strbuf_addf(&date, "@%"PRIuMAX, (uintmax_t)now);
>> + argv_array_pushf(&child->args, "--date=%s", date.buf);
>
> it doesn't matter but it might have been nicer to set both dates the
> same way in the environment.
> + argv_array_pushf(&child->env_array, "GIT_COMMITTER_DATE=%s", date.buf);
We can see that this date string lacks timezone information, which
would likely fall back to whatever timezone the user is in. Is that
what we want? I am guessing it is, as we are dealing with "now"
timestamp, but wanted to double check.
>> + if (opts->ignore_date) {
>> + if (!author)
>> + BUG("ignore-date can only be used with "
>> + "rebase, which must set the author "
>> + "before committing the tree");
>> + ignore_author_date(&author);
>
> Is this leaking the old author? I'd rather see
>
> tmp_author = ignore_author_date(author);
> free(author);
> author = tmp_author;
Or make sure ignore_author_date() does not leak the original, when
it rewrites its parameter.
But I have a larger question at the higher design level. Why are we
passing a single string "author" around, instead of parsed and split
fields, like <name, email, timestamp, tz> tuple? That would allow us
to replace only the time part a lot more easily. Would it make the
other parts of the code more cumbersome (I didn't check---and if
that is the case, then that is a valid reason why we want to stick
to the current "a single string 'author' keeps the necessary info
for the 4-tuple" design).
>> + }
>> res = commit_tree(msg.buf, msg.len, cache_tree_oid,
>> NULL, &root_commit, author,
>> opts->gpg_sign);
>> + }
>> strbuf_release(&msg);
>> strbuf_release(&script);
>> @@ -1053,6 +1087,8 @@ 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->ignore_date)
>> + push_dates(&cmd);
>> if (defmsg)
>> argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
>> else if (!(flags & EDIT_MSG))
>> @@ -1515,6 +1551,11 @@ static int try_to_commit(struct repository *r,
>> reset_ident_date();
>> + if (opts->ignore_date) {
>> + ignore_author_date(&author);
>> + free(author_to_free);
>
> Where is author_to_free set? We should always free the old author, see
> above.
Or require callers to pass a free()able memory to ignore_author_date()
and have the callee free the original?
