GIT_COMMITTER_* and reflog

GIT_COMMITTER_* and reflog

[Luke Dashjr]

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).

Is there a need to support this override for the reflog?
Is there any reason it can't be changed to use GIT_REFLOG_* instead?

Note that the reflog does NOT pull the name/date from the commit object 
itself, but rather calls git_committer_info() to get a new name/date for 
reflog purposes. So it does not appear to be intentionally designed to use 
the commit's date.

Thanks for your thoughts,

Luke

P.S. If the reflog date can be made useful like this, maybe I will finish a 
minor patch to add reflog-date to the pretty formatting format so it can be 
viewed nicely... ;)

Re: GIT_COMMITTER_* and reflog

[Jonathan Nieder]

Hi Luke,

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?

Thanks and hope that helps,
Jonathan

Re: GIT_COMMITTER_* and reflog

[Luke Dashjr]

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}.

Luke

Re: GIT_COMMITTER_* and reflog

[Philip Oakley]

copying Dscho,

On 26/10/2019 03:43, Luke Dashjr wrote:
> 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.
This looks to have a strong similarity to the Git-for-Windows patches 
which are regularly rebased on top of this here upstream Git.

Dscho uses the `garden shears` script [1] to help update the patch set, 
in conjunction with the 'updated' --rebase-merges option (replaces the 
deprecated --preserve-merges).

> 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}.

-- 
Philip

[1] https://github.com/git-for-windows/build-extra/blob/master/shears.sh

Re: GIT_COMMITTER_* and reflog

[Junio C Hamano]

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().

Re: GIT_COMMITTER_* and reflog

[Jeff King]

On Sat, Oct 26, 2019 at 04:34:22PM +0900, Junio C Hamano wrote:

> 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().

FWIW, I was about to write a similar response. If people really wanted a
separate reflog ident, I could see introducing $GIT_REFLOG_NAME, etc.
But the current date handling just seems buggy (and an unintended
consequence of reusing the existing ident code).

If somebody wants to pursue a patch, I suspect the solution is probably
something like this (totally untested):

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

Re: GIT_COMMITTER_* and reflog

[Junio C Hamano]

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

Re: GIT_COMMITTER_* and reflog

[Phillip Wood]

Hi Junio/Peff

On 27/10/2019 12:20, Junio C Hamano wrote:
> 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 ;-)

It would allow for more robust am/rebase --ignore-date tests as we would 
know what date to expect rather than just checking the timezone.

Best Wishes

Phillip

> 
> 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

Re: GIT_COMMITTER_* and reflog

[Jeff King]

On Sun, Oct 27, 2019 at 09:20:24PM +0900, Junio C Hamano wrote:

> 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 ;-)

Yeah, I agree datestamp() should respect $GIT_TEST_DATE_NOW. This whole
topic is not something for the -rc period, I would think.

-Peff