Re: [GSoC][PATCH v2 5/6] rebase -i: support --ignore-date
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Aug 14, 2019, 18:47 UTC
- Message-ID
- <812513a6-bd83-5830-055f-7c0252e5ecd8@gmail.com>
- In-Reply-To
- <xmqqo90t7zhl.fsf@gitster-ct.c.googlers.com>
Hi Junio & Rohit
On 13/08/2019 18:21, Junio C Hamano wrote:
Show 18 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> 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.I think we probably want to use the local timezone as you suggest
Show 24 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).It's a bit of a mess at the moment. There are places where we want a single author data string when calling commit_tree_extended(), and other places where we want to set the name, email and date in the environment when running 'git commit'. For the latter case we use a file which we read and write as the C version just follows what the shell script did. I think carrying round a <name, email, timestamp, tz> tuple would be the sensible way to go and we can build the author string when we need it. Doing that would hopefully eliminate having to read and write the author file so much. I haven't looked at how difficult it would be to change the existing code to do that. We should also really carry the commit message around in a variable and only write it to a file if a pick fails or we are editing the message and running 'git commit'. If we're just using commit_tree_extended() there is no need to be writing the message to a file and then reading it back in again later.
Best Wishes
Phillip
Show 28 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?
>