Re: [PATCH v8 2/5] am: stop exporting GIT_COMMITTER_DATE

[Phillip Wood <phillip.wood123@gmail.com>]

On 17/08/2020 20:12, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The implementation of --committer-date-is-author-date exports
>> GIT_COMMITTER_DATE to override the default committer date but does not
>> reset GIT_COMMITTER_DATE in the environment after creating the commit
>> so it is set in the environment of any hooks that get run. We're about
>> to add the same functionality to the sequencer and do not want to have
>> GIT_COMMITTER_DATE set when running hooks or exec commands so lets
>> update commit_tree_extended() to take an explicit committer so we
>> override the default date without setting GIT_COMMITTER_DATE in the
>> environment.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>  builtin/am.c     | 28 +++++++++++++++++++++++-----
>>  builtin/commit.c |  4 ++--
>>  commit.c         | 11 +++++++----
>>  commit.h         |  7 +++----
>>  ident.c          | 24 ++++++++++++++----------
>>  sequencer.c      |  4 ++--
>>  6 files changed, 51 insertions(+), 27 deletions(-)
> 
> Nice.
> 
> Obviously this would affect the environment while am is running, and
> the change is observable by post-applypatch hook.  I am not sure if
> this change-in-behaviour would negatively affect people's hooks, but
> given the large end-user population we have, somebody somewhere will
> get hit.

Yes I did think about that but decided it was probably better to fix it.
At the moment the pre-applypatch gets GIT_COMMITTER_DATE set to the date
of the last commit which does not make much sense. If anyone does need
the committer date in the post-applypatch hook they can look at HEAD. We
should perhaps mention that in the release notes

>> diff --git a/ident.c b/ident.c
>> index e666ee4e59..7cbf223350 100644
>> --- a/ident.c
>> +++ b/ident.c
>> @@ -361,11 +361,15 @@ N_("\n"
>>  const char *fmt_ident(const char *name, const char *email,
>>  		      enum want_ident whose_ident, const char *date_str, int flag)
>>  {
>> -	static struct strbuf ident = STRBUF_INIT;
>> +	static int index;
>> +	static struct strbuf ident_pool[2] = { STRBUF_INIT, STRBUF_INIT };
>>  	int strict = (flag & IDENT_STRICT);
>>  	int want_date = !(flag & IDENT_NO_DATE);
>>  	int want_name = !(flag & IDENT_NO_NAME);
>>  
>> +	struct strbuf *ident = &ident_pool[index];
>> +	index = (index + 1) % ARRAY_SIZE(ident_pool);
> 
> 2-element rotating buffer because we happen to care at most two
> idents at the same time, author's and committer's?
> 
> How many callers of fmt_ident() do we have these days?  I wonder if
> we can introduce a new API that lets/forces the caller to prepare a
> strbuf and migrate the current callers of this function to it, of if
> it is too large a churn for the purpose of this series.

After this series is applied there are the following callers

blame.c:207: ident = fmt_ident("Not Committed Yet", "not.committed.yet",
builtin/am.c:1591: author = fmt_ident(state->author_name,
state->author_email,
builtin/am.c:1597: committer = fmt_ident(state->committer_name,
builtin/commit.c:638: strbuf_addstr(author_ident, fmt_ident(name, email,
WANT_AUTHOR_IDENT, date,
ident.c:466: return fmt_ident(name, email, whose_ident, NULL,
ident.c:476: return fmt_ident(getenv("GIT_AUTHOR_NAME"),
ident.c:489: return fmt_ident(getenv("GIT_COMMITTER_NAME"),
sequencer.c:1461: committer = fmt_ident(opts->committer_name,
sequencer.c:1481: author = fmt_ident(name, email, WANT_AUTHOR_IDENT, NULL,

In blame.c we'd have to add an strbuf to pass in,

The caller in builtin/commit.c would obviously be easy to convert as it
is stuffing the result into an strbuf already.

For am and the sequencer fmt_ident() is in a function which is called
from a loop and it is convenient not to have to worry about passing an
strbuf around or allocating and freeing it on each function call

The callers in ident (fmt_name(), git_author_info() and
git_committer_info()) return the string so they would need their own
strbuf or have to be changed so the caller passed one in. There are
quite a few callers of those functions and they seem to either
immediately call split_ident_line() or duplicate the returned string
(mostly the latter).

So it would be a bit of work to do it but not an enormous amount
(assuming we don't change the signature of git_author_info() etc in
ident.c, although given the pattern of callers it might be worth doing
that if they are mostly duplicating the returned string anyway)

I'm going to be off line for 10-14 days from the beginning of next week,
I could take a look at it after that, or we could leave it for a future
improvement - what do you think?

Best Wishes

Phillip


> 
> Thanks.
>