Hi Junio & Rohit
On 13/08/2019 18:21, Junio C Hamano wrote:
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.
I think we probably want to use the local timezone as you suggest
+ 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
+ }
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?
