Re: [GSoC][PATCH v2 5/6] rebase -i: support --ignore-date
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 13, 2019, 17:21 UTC
- Message-ID
- <xmqqo90t7zhl.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <b59f0aa2-1dc3-905a-0094-5f63718dffcf@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
Show 11 quoted lines
>> +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.
Show 12 quoted lines
>> + 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).
Show 24 quoted lines
>> + }
>> 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?