GIT_COMMITTER_* and reflog

> Luke Dashjr wrote:
> > It appears the reflog currently allows its log data (name and date) to be
> > overridden by the GIT_COMMITTER_* environment variables. At least for my
> > workflow, this kinda breaks the reflog (as I regularly set
> > GIT_COMMITTER_DATE to produce deterministic commit objects).
>
> Can you say more about this?  What is the workflow this is part of?  Can
> you describe a sequence of steps and how you are affected during those
> steps?

> > Is there a need to support this override for the reflog?
>
> Yes.
>
> > Is there any reason it can't be changed to use GIT_REFLOG_* instead?
>
> Would a new GIT_REFLOG_* set of envvars that overrides GIT_COMMITTER_*
> work for you?  If I understand correctly, you could set
> GIT_REFLOG_NAME and GIT_REFLOG_EMAIL to an appropriate identity, but
> you wouldn't have a good value to put in GIT_REFLOG_DATE.
>
> If GIT_COMMITTER_{NAME,EMAIL} were used when writing reflogs but
> GIT_COMMITTER_DATE weren't, would that help with your workflow?

> On Saturday 26 October 2019 02:20:39 Jonathan Nieder wrote:
>> Luke Dashjr wrote:
>>> It appears the reflog currently allows its log data (name and date) to be
>>> overridden by the GIT_COMMITTER_* environment variables. At least for my
>>> workflow, this kinda breaks the reflog (as I regularly set
>>> GIT_COMMITTER_DATE to produce deterministic commit objects).
>> Can you say more about this?  What is the workflow this is part of?  Can
>> you describe a sequence of steps and how you are affected during those
>> steps?
> I maintain a bleeding-edge variant of a more stable project, which is
> constantly being rebased on the latest stable version.

> To make this easier, I
> use a Perl script to generate the bleeding-edge version's git branches:
>      https://github.com/bitcoinknots/assemble-deriv
> It uses GIT_COMMITTER_DATE to ensure that I can repeatedly generate the branch
> until everything merges successfully, without polluting the repository with
> hundreds of merge commits each attempt. (Which would be annoying, since I
> literally never prune.)
>
> Because git's reflog also uses GIT_COMMITTER_DATE, my reflogs (HEAD in
> particular) get polluted with incorrect timestamps during this process.
>
>>> Is there a need to support this override for the reflog?
>> Yes.
>>
>>> Is there any reason it can't be changed to use GIT_REFLOG_* instead?
>> Would a new GIT_REFLOG_* set of envvars that overrides GIT_COMMITTER_*
>> work for you?  If I understand correctly, you could set
>> GIT_REFLOG_NAME and GIT_REFLOG_EMAIL to an appropriate identity, but
>> you wouldn't have a good value to put in GIT_REFLOG_DATE.
>>
>> If GIT_COMMITTER_{NAME,EMAIL} were used when writing reflogs but
>> GIT_COMMITTER_DATE weren't, would that help with your workflow?
> Yes, it's really only GIT_COMMITTER_DATE that's messing me up personally.
> I never use GIT_COMMITTER_{NAME,EMAIL}.

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > If GIT_COMMITTER_{NAME,EMAIL} were used when writing reflogs but
> > GIT_COMMITTER_DATE weren't, would that help with your workflow?
> 
> Thanks for a thoughtful response.
> 
> My knee-jerk reaction is that it probably was a design bug that came
> out of laziness that we used the usual mechanism to obtain the
> committer date when deciding the timestamp we leave in reflog
> entries.  Given that we say master@{6.hours.ago} etc., we should
> base the timestamp on something that is coherent with what the end
> users would give us, e.g. "6.hours.ago".  IOW, we should be using
> the wallclock time without paying attention to GIT_COMMITTER_DATE,
> i.e. date.c::get_time().

>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d60767ab73..2ebf2feeb8 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1658,7 +1658,10 @@ static int files_log_ref_write(struct files_ref_store *refs,
>  	if (logfd < 0)
>  		return 0;
>  	result = log_ref_write_fd(logfd, old_oid, new_oid,
> -				  git_committer_info(0), msg);
> +				  fmt_ident(getenv("GIT_COMMITTER_NAME"),
> +					    getenv("GIT_COMMITTER_EMAIL"),
> +					    WANT_COMMITTER_IDENT, NULL, 0),
> +				  msg);
>  	if (result) {
>  		struct strbuf sb = STRBUF_INIT;
>  		int save_errno = errno;
>
> -Peff

> Jeff King <peff@peff.net> writes:
> 
>> If somebody wants to pursue a patch, I suspect the solution is probably
>> something like this (totally untested):
> 
> Looks sensible.  It is very much unsatisfying that datestamp(),
> which is used by fmt_ident() when no date string is given, seems to
> totally bypass date.c::get_time(), which means the framework to give
> fake timestamp via GIT_TEST_DATE_NOW cannot be used to write
> reproducible tests.
> 
> Given that datestamp() is only used by the push certificate and
> fast-import codepaths and nowhere else, I suspect that "fixing" it
> retroactively to honor GIT_TEST_DATE_NOW would not have any negative
> fallout, but that's not something I should be contemplating on
> during the -rc period ;-)

> 
> Thanks.
> 
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index d60767ab73..2ebf2feeb8 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -1658,7 +1658,10 @@ static int files_log_ref_write(struct files_ref_store *refs,
>>   	if (logfd < 0)
>>   		return 0;
>>   	result = log_ref_write_fd(logfd, old_oid, new_oid,
>> -				  git_committer_info(0), msg);
>> +				  fmt_ident(getenv("GIT_COMMITTER_NAME"),
>> +					    getenv("GIT_COMMITTER_EMAIL"),
>> +					    WANT_COMMITTER_IDENT, NULL, 0),
>> +				  msg);
>>   	if (result) {
>>   		struct strbuf sb = STRBUF_INIT;
>>   		int save_errno = errno;
>>
>> -Peff

> Jeff King <peff@peff.net> writes:
> 
> > If somebody wants to pursue a patch, I suspect the solution is probably
> > something like this (totally untested):
> 
> Looks sensible.  It is very much unsatisfying that datestamp(),
> which is used by fmt_ident() when no date string is given, seems to
> totally bypass date.c::get_time(), which means the framework to give
> fake timestamp via GIT_TEST_DATE_NOW cannot be used to write
> reproducible tests.
> 
> Given that datestamp() is only used by the push certificate and
> fast-import codepaths and nowhere else, I suspect that "fixing" it
> retroactively to honor GIT_TEST_DATE_NOW would not have any negative
> fallout, but that's not something I should be contemplating on
> during the -rc period ;-)