Re: [PATCH] reset cached ident date before creating objects

From: Jeff King <peff@peff.net>
Date: Jul 30, 2016, 02:41 UTC
Message-ID: <20160730024135.oaqtjpo5l2e3dam2@sigill.intra.peff.net>
In-Reply-To: <CACRoPnS2kDRLiY8KX3K4Havh7d1GWy3mUXSiYCzw45BznuwYeA@mail.gmail.com>

On Sat, Jul 30, 2016 at 10:11:56AM +0800, Paul Tan wrote:

Show 17 quoted lines

> > diff --git a/commit.c b/commit.c
> > index 71a360d..7ddbffe 100644
> > --- a/commit.c
> > +++ b/commit.c
> > @@ -1548,6 +1548,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
> >         }
> >
> >         /* Person/date information */
> > +       reset_ident_date();
> >         if (!author)
> >                 author = git_author_info(IDENT_STRICT);
> >         strbuf_addf(&buffer, "author %s\n", author);
> 
> But since builtin/commit.c constructs its author ident string before
> calling the editor and then commit_tree_extended(), this would cause
> the resulting commits to have committer timestamps which differ from
> their author timestamps.

Hrm, yeah. I assumed it would only pass in the author string for things like "-c" or "--amend". But it looks like it unconditionally passes in the author. And it would be slightly difficult to have it pass NULL, because it may actually have _part_ of an author (e.g., "--author" will come up with name and email but not the date), so it has to sometimes combine those bits with things like ident_default_date() itself.

I guess one option would be to commit_tree_extended() to take the broken-down author bits and call fmt_ident() itself. That's what all of the callers are doing (that, or just passing NULL). It would make the interface a bit clunkier, but I think the end result would be more flexible.

I suppose that would be tricky for git-commit, because in addition to passing the result of fmt_ident() to commit_tree_extended(), it wants to take the pieces and put them in the environment for hooks to see. And if the data is available only inside commit_tree_extended(), we don't have it for the hooks.

> So maybe we would have to put reset_ident_date() at the end of the
> function instead, at least after git_committer_info() is called.

Yes, although "reset and end" still feels a bit weird to me.

I'd almost prefer to just have long-running programs insert resets at strategic points.

-Peff