[ISSUE] Stop accessing, storing, and sharing the user's time zone

[ISSUE] Stop accessing, storing, and sharing the user's time zone

[Nathaniel Manista]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

(This is also filed at https://bugs.chromium.org/p/git/issues/detail?id=43.)

Affected Version: All? This has been bothering me at least a year.

What steps will reproduce the problem?
1. Author a commit.
2. "git log --pretty=fuller"

What is the expected output?
The log will display that the timestamps of the commit have both the
author time and committer time in UTC. Internally no part of the
commit will have stored any time zone information and when the commit
is shared with others no information about where the user was in the
world at the time of the commit will be obtainable from it.

What do you see instead?
Authoring and sharing a commit by default exposes the user's time zone.

Additional context:
"commit --date=YYYY-MM-DDThh:mm:ss+0000" suffices to put the author
time in UTC but not the commit time in UTC. But the user shouldn't
have to pass a flag at all.

Where the user is in the world is PII that git ought not to record and
make available as part of the user's software engineering (make
available to colleagues, in the case of proprietary development, and
make available to the world, in the case of open source). Git should
entirely stop accessing, recording, and sharing the user's time zone,
full stop. Failing that, git should by default stop accessing,
recording, and sharing the user's time zone, but if individual users
want to have their time zones on their commits, they can opt into it.
Failing that, users should be able to add a .gitconfig line to ensure
that all author timestamps, all committer timestamps, and any other
information are in UTC.

(Thanks much,
-Nathaniel)

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4849 bytes --]

Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone

[Junio C Hamano]

Nathaniel Manista <nathaniel@google.com> writes:

> Authoring and sharing a commit by default exposes the user's time zone.

s/exposes/stores/ and that is a feature.  You are free to run 

    $ TZ=GMT git commit

if you wanted to opt out of the feature, but this has been the
default since day one and people expect Git to behave this way.

RE: [ISSUE] Stop accessing, storing, and sharing the user's time zone

[Randall S. Becker]

On December 5, 2019 12:14 PM, Nathaniel Manista wrote:
> (This is also filed at https://bugs.chromium.org/p/git/issues/detail?id=43.)

Getting permission denied trying to access this.

> Affected Version: All? This has been bothering me at least a year.
> 
> What steps will reproduce the problem?
> 1. Author a commit.
> 2. "git log --pretty=fuller"
> 
> What is the expected output?
> The log will display that the timestamps of the commit have both the
> author time and committer time in UTC. Internally no part of the
> commit will have stored any time zone information and when the commit
> is shared with others no information about where the user was in the
> world at the time of the commit will be obtainable from it.
> 
> What do you see instead?
> Authoring and sharing a commit by default exposes the user's time zone.
> 
> Additional context:
> "commit --date=YYYY-MM-DDThh:mm:ss+0000" suffices to put the author
> time in UTC but not the commit time in UTC. But the user shouldn't
> have to pass a flag at all.
> 
> Where the user is in the world is PII that git ought not to record and
> make available as part of the user's software engineering (make
> available to colleagues, in the case of proprietary development, and
> make available to the world, in the case of open source). Git should
> entirely stop accessing, recording, and sharing the user's time zone,
> full stop. Failing that, git should by default stop accessing,
> recording, and sharing the user's time zone, but if individual users
> want to have their time zones on their commits, they can opt into it.
> Failing that, users should be able to add a .gitconfig line to ensure
> that all author timestamps, all committer timestamps, and any other
> information are in UTC.

My position has been UTC as the standard in all cases with storing LCT as an optional only. I like the opt-in concept. I currently am running a repository located at UTC+2, with developers at UTC-5. It is driving us a bit wonky. I would rather see only UTC.

Regards,
Randall

Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone

[Junio C Hamano]

"Randall S. Becker" <rsbecker@nexbridge.com> writes:

> .... I currently am
> running a repository located at UTC+2, with developers at
> UTC-5. It is driving us a bit wonky. I would rather see only UTC.

If "seeing" is the primary reason (i.e. you want to compare times
your people worked on their commits), you can always do that on the
display side (e.g. "git log --date=local").  It is not a good excuse
to advocate for information loss.

I also feel that TZ being PII is not particularly a brilliant
argument against recording TZ---of course it is PII, so are the
committer e-mail address and the committer name.  Those who want to
hide can hide but in order to keep track of provenance who did what
when, we do record them.

As you can guess from the above reasoning, I am not fundamentally
opposed to introducing user.tz to complement existing user.name and
user.email configuration variables.

Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone

[Santiago Torres Arias]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Thu, Dec 05, 2019 at 09:43:54AM -0800, Junio C Hamano wrote:
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> > .... I currently am
> > running a repository located at UTC+2, with developers at
> > UTC-5. It is driving us a bit wonky. I would rather see only UTC.
>
> I also feel that TZ being PII is not particularly a brilliant
> argument against recording TZ---of course it is PII, so are the
> committer e-mail address and the committer name.  Those who want to
> hide can hide but in order to keep track of provenance who did what
> when, we do record them.

This is exactly what I had found confusing from the original post.
> 
> As you can guess from the above reasoning, I am not fundamentally
> opposed to introducing user.tz to complement existing user.name and
> user.email configuration variables.

This sounds like a small delta that would probably please everyone. For
now, using the --date argument on git commit allows you to also pass a
timezone:

    git commit --date="$(TZ=PST date)"

Which you could easily alias...

Thanks,
-Santiago

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [ISSUE] Stop accessing, storing, and sharing the user's time zone

[Randall S. Becker]

On December 5, 2019 12:44 PM, Junio C Hamano wrote:
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> > .... I currently am
> > running a repository located at UTC+2, with developers at UTC-5. It is
> > driving us a bit wonky. I would rather see only UTC.
> 
> If "seeing" is the primary reason (i.e. you want to compare times your
people
> worked on their commits), you can always do that on the display side (e.g.
> "git log --date=local").  It is not a good excuse to advocate for
information
> loss.
> 
> I also feel that TZ being PII is not particularly a brilliant argument
against
> recording TZ---of course it is PII, so are the committer e-mail address
and the
> committer name.  Those who want to hide can hide but in order to keep
> track of provenance who did what when, we do record them.
> 
> As you can guess from the above reasoning, I am not fundamentally opposed
> to introducing user.tz to complement existing user.name and user.email
> configuration variables.

I rather like this idea. I'm going to look into it when I'm in the
upside-down toward the end of the month.

Cheers,
Randall

[RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Shengfa Lin]

Discussed with Jonathan and Junio regarding whether we should support
letting user specify timezone or restricted to UTC only. There was no
defnite conclusion. I think it's a good idea to start with this
implementation which might result in the change landing or e.g. a
documentation change.

The patch add checking for user.hideTimezone and change environment
variable TZ to UTC in the beginning of cmd_commit if it's true. As a
result, when calculating timezone offset in datestamp(date.c) would result in 0. 

Shengfa Lin (1):
  hideTimezone: add a user.hideTimezone config

 Documentation/config/user.txt   |  4 ++++
 builtin/commit.c                |  5 +++++
 t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+)
 create mode 100755 t/t7527-commit-hide-timezone.sh

-- 
2.28.0.709.gb0816b6eb0-goog

[RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Users requested hiding location in the world from source control
trail. This is an implementation to read user.hideTimezone in
cmd_commit and set timezone to UTC if it's true.

Added a brief explanation of the new field in Documentation
and added tests for true/false and reset-author

Signed-off-by: Shengfa Lin <shengfa@google.com>
---
 Documentation/config/user.txt   |  4 ++++
 builtin/commit.c                |  5 +++++
 t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+)
 create mode 100755 t/t7527-commit-hide-timezone.sh

diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index 59aec7c3ae..bd6813f527 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -36,3 +36,7 @@ user.signingKey::
 	commit, you can override the default selection with this variable.
 	This option is passed unchanged to gpg's --local-user parameter,
 	so you may specify a key using any method that gpg supports.
+
+user.hideTimezone::
+  Override TZ to UTC for Git commits to hide user's timezone in commit
+  date
diff --git a/builtin/commit.c b/builtin/commit.c
index 42b964e0ca..fb1cbb8a39 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
 	s.colopts = 0;
 
+  git_config(git_default_config, NULL);
+  int hide_timezone = 0;
+  if (!git_config_get_bool("user.hideTimezone", &hide_timezone)  && hide_timezone)
+    setenv("TZ", "UTC", 1);
+
 	if (get_oid("HEAD", &oid))
 		current_head = NULL;
 	else {
diff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh
new file mode 100755
index 0000000000..41ed9c27da
--- /dev/null
+++ b/t/t7527-commit-hide-timezone.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='git-commit can override local timezone setting by reading user.hideTimezone from config'
+
+. ./test-lib.sh
+
+test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' '
+        git config user.hideTimezone false &&
+        echo test1 >> file &&
+        git add file &&
+        # unset GIT_AUTHOR_DATE from test_tick
+        unset GIT_AUTHOR_DATE &&
+        TZ=Europe/Istanbul git commit -m initial &&
+        git log -1 > output &&
+        grep "Date: .* +0300" output
+'
+
+test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' '
+        git config user.hideTimezone true &&
+        git commit --amend --reset-author &&
+        git log -1 > output &&
+        grep "Date: .* +0000" output
+'
+
+test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' '
+        git config user.hideTimezone true &&
+        echo test2 >> file &&
+        git add file &&
+        # TZ setting corresponding to -0600 or -0500 depending on DST
+        # unset GIT_AUTHOR_DATE from test_tick
+        unset GIT_AUTHOR_DATE &&
+        TZ=America/Chicago git commit -m test2 &&
+        git log -1 > output &&
+        grep "Date: .* +0000" output
+'
+
+test_done
-- 
2.28.0.709.gb0816b6eb0-goog

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

> Users requested hiding location in the world from source control
> trail. This is an implementation to read user.hideTimezone in
> cmd_commit and set timezone to UTC if it's true.
>
> Added a brief explanation of the new field in Documentation
> and added tests for true/false and reset-author
>
> Signed-off-by: Shengfa Lin <shengfa@google.com>
> ---
>  Documentation/config/user.txt   |  4 ++++
>  builtin/commit.c                |  5 +++++

There are many ways other than running "git commit" for a commit to
be created, including but not limited to "git merge", "git rebase",
"git pull" (with or without "--rebase").

> +user.hideTimezone::
> +  Override TZ to UTC for Git commits to hide user's timezone in commit
> +  date

One level of indentation in this codebase is a single HT.

Unterminated sentence.

A configuration _without_ command line option that overrides it is
highly frowned upon.  I do not see a reason why this must be such a
configuration.  If anything, a feature like this should first start
as a command line option, and only after it proves its usefulness,
a new configuration for convenience should be added.

This only affects "git commit" and no other command (which I think
is a mistake), yet is placed in the "user.*" namespace?  That does
not make any sense.  I can sort-of understand if it were called say
"commit.useUTC" though.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 42b964e0ca..fb1cbb8a39 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
>  	s.colopts = 0;
>  
> +  git_config(git_default_config, NULL);

Declaration after statement is not tolerated in this codebase.

One level of indentation in this codebase is a single HT.

> +  int hide_timezone = 0;

Unnecessary initialization.

> +  if (!git_config_get_bool("user.hideTimezone", &hide_timezone)  && hide_timezone)

Overlong line.  Double-SP before &&

> +    setenv("TZ", "UTC", 1);
> +
>  	if (get_oid("HEAD", &oid))
>  		current_head = NULL;
>  	else {
> diff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh
> new file mode 100755
> index 0000000000..41ed9c27da

Let's not waste a test number for just a single test or two.  Can't
we roll this into 

> --- /dev/null
> +++ b/t/t7527-commit-hide-timezone.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +test_description='git-commit can override local timezone setting by reading user.hideTimezone from config'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' '
> +        git config user.hideTimezone false &&
> +        echo test1 >> file &&

Style.  Documentation/CodingGuidelines

 - Redirection operators should be written with space before, but no
   space after them.  In other words, write 'echo test >"$file"'
   instead of 'echo test> $file' or 'echo test > $file'. ...

> +        git add file &&
> +        # unset GIT_AUTHOR_DATE from test_tick
> +        unset GIT_AUTHOR_DATE &&
> +        TZ=Europe/Istanbul git commit -m initial &&
> +        git log -1 > output &&
> +        grep "Date: .* +0300" output

Do they not have DST over there, and is it guaranteed that they will
never have one?  Would we see this test fail about half of the year,
when timezone rules change over there in some future year?  After
all, they changed in 2016 last time, which is fairly recent.

This test attempts to establish the baseline, but I do not think it
is a good idea.  I think it is better *not* to unset GIT_AUTHOR_DATE
like this.  Instead, make sure it is set to some timestamp in some
timezone that is not UTC, and the timezone of the resulting commit
author date is in that timezone.  But that must have already been
done in basic tests on "git commit" that we honor the environment
variable, no?  Which means there is no need to add yet another extra
baseline test here.

> +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' '
> +        git config user.hideTimezone true &&
> +        git commit --amend --reset-author &&
> +        git log -1 > output &&
> +        grep "Date: .* +0000" output

This one IS interesting, but keep the GIT_AUTHOR_DATE set and
exported.  As long as that is from a timezone different from UTC, we
are testing what we want to test here.

> +'
> +
> +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' '
> +        git config user.hideTimezone true &&
> +        echo test2 >> file &&
> +        git add file &&
> +        # TZ setting corresponding to -0600 or -0500 depending on DST
> +        # unset GIT_AUTHOR_DATE from test_tick
> +        unset GIT_AUTHOR_DATE &&
> +        TZ=America/Chicago git commit -m test2 &&

This one is a borderline meh, compared to a test with explicit
GIT_AUTHOR_DATE getting overridden by the configuration.  It is not
all that wrong, but I do not see a point in adding cycles to the
already big testsuite.

> +        git log -1 > output &&
> +        grep "Date: .* +0000" output
> +'
> +
> +test_done

Thanks.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

> Discussed with Jonathan and Junio regarding whether we should support
> letting user specify timezone or restricted to UTC only. There was no
> defnite conclusion.

I do not think was involved in that part of the discussion to
consider UTC vs custom zone, though.

What I said is that git () { TZ=UTC command git "$@" } is simple
enough that I doubt it is worth the engineering effort to either
read configuration file early (which is tricky) so that setenv() can
be done early in cmd_main() and/or audit the codebase widely enough
(which is time consuming) to make sure that we pay attention to the
configuration variable in all codepaths that matter to do the
setenv().

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

> Users requested hiding location in the world from source control
> trail. This is an implementation to read user.hideTimezone in
> cmd_commit and set timezone to UTC if it's true.

Not a very good proposed log message, that sounds as if "it is
what 'Users' requested, so it must be a worthwhile thing to do",
which is not the line of thinking to go by.




>
> Added a brief explanation of the new field in Documentation
> and added tests for true/false and reset-author
>
> Signed-off-by: Shengfa Lin <shengfa@google.com>
> ---
>  Documentation/config/user.txt   |  4 ++++
>  builtin/commit.c                |  5 +++++
>  t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
>  create mode 100755 t/t7527-commit-hide-timezone.sh
>
> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> index 59aec7c3ae..bd6813f527 100644
> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -36,3 +36,7 @@ user.signingKey::
>  	commit, you can override the default selection with this variable.
>  	This option is passed unchanged to gpg's --local-user parameter,
>  	so you may specify a key using any method that gpg supports.
> +
> +user.hideTimezone::
> +  Override TZ to UTC for Git commits to hide user's timezone in commit
> +  date
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 42b964e0ca..fb1cbb8a39 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
>  	s.colopts = 0;
>  
> +  git_config(git_default_config, NULL);
> +  int hide_timezone = 0;
> +  if (!git_config_get_bool("user.hideTimezone", &hide_timezone)  && hide_timezone)
> +    setenv("TZ", "UTC", 1);
> +
>  	if (get_oid("HEAD", &oid))
>  		current_head = NULL;
>  	else {
> diff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh
> new file mode 100755
> index 0000000000..41ed9c27da
> --- /dev/null
> +++ b/t/t7527-commit-hide-timezone.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +test_description='git-commit can override local timezone setting by reading user.hideTimezone from config'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' '
> +        git config user.hideTimezone false &&
> +        echo test1 >> file &&
> +        git add file &&
> +        # unset GIT_AUTHOR_DATE from test_tick
> +        unset GIT_AUTHOR_DATE &&
> +        TZ=Europe/Istanbul git commit -m initial &&
> +        git log -1 > output &&
> +        grep "Date: .* +0300" output
> +'
> +
> +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' '
> +        git config user.hideTimezone true &&
> +        git commit --amend --reset-author &&
> +        git log -1 > output &&
> +        grep "Date: .* +0000" output
> +'
> +
> +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' '
> +        git config user.hideTimezone true &&
> +        echo test2 >> file &&
> +        git add file &&
> +        # TZ setting corresponding to -0600 or -0500 depending on DST
> +        # unset GIT_AUTHOR_DATE from test_tick
> +        unset GIT_AUTHOR_DATE &&
> +        TZ=America/Chicago git commit -m test2 &&
> +        git log -1 > output &&
> +        grep "Date: .* +0000" output
> +'
> +
> +test_done

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

I won't repeat what I said in the other review message, but since I
forgot to comment on the log message...

> Users requested hiding location in the world from source control
> trail. This is an implementation to read user.hideTimezone in
> cmd_commit and set timezone to UTC if it's true.
>
> Added a brief explanation of the new field in Documentation
> and added tests for true/false and reset-author

Not a very good proposed log message, that sounds as if "it is
what 'Users' requested, so it must be a worthwhile thing to do",
which is not the line of thinking to go by.

The convention we follow in the commit log messages is to:

 - first explain that the current system does not do X (in present
   tense, so we do NOT say "previously we did not do X"), then

 - explain why doing X would be a good thing, and finally

 - give an order to the codebase to start doing X.


Perhaps

    Many places in Git record the timezone of the actor when a
    timestamp is recorded, including the committer and author
    timestamps in a commit object and the tagger timestamp in a tag
    object.  Some people however prefer to "lie" about where they
    actually are.

    They _could_ just say "export TZ=UTC" and be done with it, but
    the method would not easily allow them to pretend to be in the
    UTC timezone only with Git, while revealing their true timezone
    to other activities (e.g. sending e-mail?).

    Introduce user.hideTimeZone configuration variable, which can be
    optionally set to 'true' to pretend to Git as if the user has
    exported environment variable TZ with the value UTC.

Thanks.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> new file mode 100755
>> index 0000000000..41ed9c27da
>
> Let's not waste a test number for just a single test or two.  Can't
> we roll this into 

... an existing test for "git commit"?

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' '
>> +        git config user.hideTimezone true &&
>> +        git commit --amend --reset-author &&
>> +        git log -1 > output &&
>> +        grep "Date: .* +0000" output
>
> This one IS interesting, but keep the GIT_AUTHOR_DATE set and
> exported.  As long as that is from a timezone different from UTC, we
> are testing what we want to test here.

Note.

Once GIT_AUTHOR_DATE and friends are set fully including timezone,
we won't even read TZ because there is no need to.  But you can do
something along these lines:

	test_config user.hideTimeZone true &&
	(
		export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&
		git commit ... &&
		git show -s --format='%aI' >output &&
		echo 2020-09-13T15:26:40+03:00 >expect &&
		...

I think (haven't actually tested) "git commit --date=<datestring>" option
is handled the same way, i.e. comparing these two would be a way not
to touch the environment variable.

    TZ=UTC-09 git commit --date=@1600000000 ... &&
    TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 ... &&
    git show -s --format='%aI' HEAD~1 >output0 &&
    git show -s --format='%aI' HEAD~0 >output1



    

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 	test_config user.hideTimeZone true &&
> 	(
> 		export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&
> 		git commit ... &&
> 		git show -s --format='%aI' >output &&
> 		echo 2020-09-13T15:26:40+03:00 >expect &&

Oops.  The sample date and zone must be adjusted for the values
exported above.  I expect that we'd need to do

		...
		echo 2020-09-13T12:26:40+00:00 >expect &&
		test_cmp expect output

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> What I said is that git () { TZ=UTC command git "$@" } is simple
> enough that I doubt it is worth the engineering effort to either
> read configuration file early (which is tricky) so that setenv() can
> be done early in cmd_main() and/or audit the codebase widely enough
> (which is time consuming) to make sure that we pay attention to the
> configuration variable in all codepaths that matter to do the
> setenv().

Regarding the implementation, the posted patch to "git commit" uses
the "futz with TZ early inside the git process" approach, but I
think we should not do so for another reason.  Our setenv() may not
be early enough---before the code that decides to call a setenv()
is run, there are many things that are outside your control (like
the tracing subsystem, repository discovery, etc.) will run, and if
any of them does something that triggers tzset() to be called, it
will be done with the value of TZ the process started with, and our
setenv() that happens much later won't have any effect to it.

Another thing is that setting TZ ourselves would affect hooks and
other programs we spawn as subprocess, which may or may not be
desired, depending on the reason why the user is forcing UTC.

All code that create object headers like committer, author and
tagger lines use the same git_author_info() and git_committer_info()
API, I think, which eventually goes to fmt_ident(), and they produce
a pair <number of seconds since epoch, offset>.  This gives us an
entirely different opening to tackle this issue from, because the
"number of seconds since epoch" part won't change the value no
matter what timezone you are in.

You can let the existing code produce its natural result and then
when the "force UTC" flag is set, override the offset part to +0000
if and only if the timezone was obtained from the current
environment (this if-and-only-if is necessary, because you do not
want to rewrite and force UTC when you run "git commit --amend"
without the "--reset-author" option to update a commit that was
created somewhere else to UTC).  That way, we do not have to futz
with TZ environment or tzset.

Just something to think about.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Jonathan Nieder]

Hi,

Shengfa Lin wrote:

>  Documentation/config/user.txt   |  4 ++++
>  builtin/commit.c                |  5 +++++
>  t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
>  create mode 100755 t/t7527-commit-hide-timezone.sh

Thanks for this discussion starter.  I'm interested to see what we end
up with.

To summarize the thread so far:

Nathaniel Manista, who we can credit with a `Reported-by` line,
reports that (mildly paraphrased):

	Authoring and sharing a commit by default exposes the user's
	time zone.

	"gt commit --date=YYYY-MM-DDThh:mm:ss+0000" suffices to put
	the author time in UTC (though not the commit time in UTC).
	But the user shouldn't have to pass a flag at all.

	Where the user is in the world is private information that git
	ought not to record and make available as part of the user's
	software engineering (make available to colleagues, in the
	case of proprietary development, and make available to the
	world, in the case of open source). Git should entirely stop
	accessing, recording, and sharing the user's time zone.

On the other hand, various others have mentioned some beneficial
aspects of recording the timezone --- for example, it makes it easier
to make sense of the chronology in a program's development.  What
seems uncontroversial is that users should have control over the time
zone used (as they already do, via the TZ environment variable).

In response to the suggestion of a "[user] timeZone" setting,
Nathaniel suggests

	That sounds like a great first step and like something that
	wouldn't ruffle anyone's feathers while proving the value of
	ignoring time zone information. 🙂

There is more to discuss in this design space --- let's see where the
patch leads us.

> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -36,3 +36,7 @@ user.signingKey::
>  	commit, you can override the default selection with this variable.
>  	This option is passed unchanged to gpg's --local-user parameter,
>  	so you may specify a key using any method that gpg supports.
> +
> +user.hideTimezone::
> +  Override TZ to UTC for Git commits to hide user's timezone in commit
> +  date

To me, this seems less appealing than being able to set the time zone.
By setting the time zone to match others in the same project, I would
be able to blend in without sharing information about my travels.  If
I use the hideTimezone setting instead, then I may stick out as the
only contributor using UTC.

UTC is the default timezone in various development environments so
this is not a big deal, but it seems enough reason to prefer the
fuller-control approach.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 42b964e0ca..fb1cbb8a39 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
>  	s.colopts = 0;
>  
> +  git_config(git_default_config, NULL);
> +  int hide_timezone = 0;
> +  if (!git_config_get_bool("user.hideTimezone", &hide_timezone)  && hide_timezone)
> +    setenv("TZ", "UTC", 1);

Like Junio mentioned, this affects "git commit" but not other commands
that record the current date with the local timezone.

The fundamental tool to exercise that machinery is

	$ git var GIT_AUTHOR_IDENT
	Jonathan Nieder <jrnieder@gmail.com> 1601517809 -0700

so I suppose I'd be interested in seeing that exercised in tests.

Looking at the implementation, I find fmt_ident in ident.c, which
calls ident_default_date(), which calls date.c's datestamp, which uses
localtime_r to get the timezone.  localtime_r on all platforms I know
of calls tzset, though apparently[*] it is not required to.

The unfortunate thing about these APIs is that there's no way to pass
in a timezone from a string instead of from the environment.  This
means that passing through the environment as above is the only
reasonable way to do it, but that would have the unfortunate result
of changing the output of commands like "git log --date=local" that
are about writing dates to the terminal instead of storing them.

So I'd be tempted to do something targetted like this:

-- 8< --
diff --git i/date.c w/date.c
index f9ea807b3a9..658ba1a9a45 100644
--- i/date.c
+++ w/date.c
@@ -5,6 +5,7 @@
  */
 
 #include "cache.h"
+#include "config.h"
 
 /*
  * This is like mktime, but without normalization of tm_wday and tm_yday.
@@ -998,6 +999,20 @@ void datestamp(struct strbuf *out)
 	time_t now;
 	int offset;
 	struct tm tm = { 0 };
+	const char *env_tz;
+	char *config_tz = NULL;
+	int restore_tz = 0;
+
+	if (!git_config_get_string("user.timezone", &config_tz)) {
+		env_tz = getenv("TZ");
+		if (env_tz)
+			env_tz = xstrdup(env_tz);
+		if (!env_tz || strcmp(env_tz, config_tz)) {
+			restore_tz = 1;
+			setenv("TZ", config_tz, 1);
+			tzset();
+		}
+	}
 
 	time(&now);
 
@@ -1005,6 +1020,14 @@ void datestamp(struct strbuf *out)
 	offset /= 60;
 
 	date_string(now, offset, out);
+
+	if (restore_tz) {
+		if (env_tz)
+			setenv("TZ", env_tz, 1);
+		else
+			unsetenv("TZ");
+	}
+	free(config_tz);
 }
 
 /*
-- >8 --

Looking over callers, who would this affect?  There are three callers:

 fast-import.c::parse_ident:
	Used to handle ident string "now".  That seems in keeping with
	the intent here, and fast-import does respect some other
	configuration though only affecting storage.  Seems fine.sensible.

 ident.c::ident_default_date:
	Used to produce author and committer timestamps and timestamps for
	reflog entries.  That's the goal; good.

 send-pack.c::generate_push_cert:
	Used for the timestamp sent to the server in a signed push
	certificate.  Also good.

So I think this does the right thing, plus it retains the
user-friendly feature of being able to *display* timestamps in their
local timezone.

Now let's talk through the downsides:

It's complex.  The performance isn't likely to be bad when
user.timezone is not set, which is nice, but it still is messier than
I'd like to see.

It's specific to Git, but a user might want to disable recording their
timezone in *all* collaboration tools (mail clients, newsreaders,
etc), not just Git.  So I would find it more compelling if there were
a common convention shared across tools for setting your exposed
timezone, just like the EMAIL envvar for setting your exposed email
address.

Thoughts?

Thanks,
Jonathan

[*] https://pubs.opengroup.org/onlinepubs/9699919799/functions/localtime.html

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Jonathan Nieder]

Junio C Hamano wrote:

>                                                Our setenv() may not
> be early enough---before the code that decides to call a setenv()
> is run, there are many things that are outside your control (like
> the tracing subsystem, repository discovery, etc.) will run, and if
> any of them does something that triggers tzset() to be called, it
> will be done with the value of TZ the process started with, and our
> setenv() that happens much later won't have any effect to it.

I thought about this before, but in fact it's okay: when calling a
function like localtime() (though not localetime_r() --- see my other
reply), tzset() is called each time so it is able to reflect any
updates to the TZ envvar from the interim.

[....]
> You can let the existing code produce its natural result and then
> when the "force UTC" flag is set, override the offset part to +0000
> if and only if the timezone was obtained from the current
> environment (this if-and-only-if is necessary, because you do not
> want to rewrite and force UTC when you run "git commit --amend"
> without the "--reset-author" option to update a commit that was
> created somewhere else to UTC).  That way, we do not have to futz
> with TZ environment or tzset.

Yes, I think this is simpler and nicer than the proposal in my other
reply.

In addition to not having to futz with TZ, I think I like the
semantics better.  The motivation that started this thread was not so
much "I want to set a custom timezone to blend in" but rather "why are
we recording the timezone at all here?"  In that context, it makes
sense to me to have a setting such as

	core.recordTimeZone

that I can turn *off* to say that I don't think datestamp() callers
should consider the timezone to be information worth recording (and
instead they should write +0000).  To me that seems a little simpler
to understand than user.hideTimezone since this focuses on turning
some functionality off (recording of the time zone) instead of turning
on a new stealth mode.

Thanks,
Jonathan

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> In addition to not having to futz with TZ, I think I like the
> semantics better.  The motivation that started this thread was not so
> much "I want to set a custom timezone to blend in" but rather "why are
> we recording the timezone at all here?"  In that context, it makes
> sense to me to have a setting such as
>
> 	core.recordTimeZone
>
> that I can turn *off* to say that I don't think datestamp() callers
> should consider the timezone to be information worth recording (and
> instead they should write +0000).  To me that seems a little simpler
> to understand than user.hideTimezone since this focuses on turning
> some functionality off (recording of the time zone) instead of turning
> on a new stealth mode.

Hmph.  It is a valid way to look at the issue, I guess.

Thanks for an input.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Thanks for the comments.

Junio C Hamano <gitster@pobox.com> writes:

>> Users requested hiding location in the world from source control
>> trail. This is an implementation to read user.hideTimezone in
>> cmd_commit and set timezone to UTC if it's true.
>>
>> Added a brief explanation of the new field in Documentation
>> and added tests for true/false and reset-author
>>
>> Signed-off-by: Shengfa Lin <shengfa@google.com>
>> ---
>>  Documentation/config/user.txt   |  4 ++++
>>  builtin/commit.c                |  5 +++++
>
> There are many ways other than running "git commit" for a commit to
> be created, including but not limited to "git merge", "git rebase",
> "git pull" (with or without "--rebase").

I overlooked on this.

>> +user.hideTimezone::
>> +  Override TZ to UTC for Git commits to hide user's timezone in commit
>> +  date
>
> One level of indentation in this codebase is a single HT.
>
> Unterminated sentence.

What does HT stands for? I will change the indentation to 8 spaces.

> This only affects "git commit" and no other command (which I think
> is a mistake), yet is placed in the "user.*" namespace?  That does
> not make any sense.  I can sort-of understand if it were called say
> "commit.useUTC" though.

I don't think this is a recommendation to implement commit.useUTC instead
since it makes more sense to support more than just the commit command.

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 42b964e0ca..fb1cbb8a39 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
>>  	s.colopts = 0;
>>  
>> +  git_config(git_default_config, NULL);
>
>Declaration after statement is not tolerated in this codebase.

If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler
catches this as an error?

>> +  int hide_timezone = 0;
>
> Unnecessary initialization.
>> +  if (!git_config_get_bool("user.hideTimezone", &hide_timezone)  && hide_timezone)
>

Is it unnecessary because I am checking the return value from git_config_get_bool so
that the uninitialized value won't be used?

>> +        git add file &&
>> +        # unset GIT_AUTHOR_DATE from test_tick
>> +        unset GIT_AUTHOR_DATE &&
>> +        TZ=Europe/Istanbul git commit -m initial &&
>> +        git log -1 > output &&
>> +        grep "Date: .* +0300" output
>
> Do they not have DST over there, and is it guaranteed that they will
> never have one?  Would we see this test fail about half of the year,
> when timezone rules change over there in some future year?  After
> all, they changed in 2016 last time, which is fairly recent.

I looked up the timezones and find one without DST without considering
the possibility of timezone rules change which would cause the test
to fail.

> This test attempts to establish the baseline, but I do not think it
> is a good idea.  I think it is better *not* to unset GIT_AUTHOR_DATE
> like this.  Instead, make sure it is set to some timestamp in some
> timezone that is not UTC, and the timezone of the resulting commit
> author date is in that timezone.  But that must have already been
> done in basic tests on "git commit" that we honor the environment
> variable, no?  Which means there is no need to add yet another extra
> baseline test here.
>

I am not sure if this test has already been done in commit basic tests.
Will remove this test.

>> +'
>> +
>> +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' '
>> +        git config user.hideTimezone true &&
>> +        echo test2 >> file &&
>> +        git add file &&
>> +        # TZ setting corresponding to -0600 or -0500 depending on DST
>> +        # unset GIT_AUTHOR_DATE from test_tick
>> +        unset GIT_AUTHOR_DATE &&
>> +        TZ=America/Chicago git commit -m test2 &&
>
> This one is a borderline meh, compared to a test with explicit
> GIT_AUTHOR_DATE getting overridden by the configuration.  It is not
> all that wrong, but I do not see a point in adding cycles to the
> already big testsuite.
>

Will remove this one as well.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Junio C Hamano <gitster@pobox.com> writes:

>>> new file mode 100755
>>> index 0000000000..41ed9c27da
>>
>> Let's not waste a test number for just a single test or two.  Can't
>> we roll this into 
>
> ... an existing test for "git commit"?

Will move into an existing test for "git commit".

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Jonathan Nieder]

Hi,

Shengfa Lin wrote:
> Thanks for the comments.

>>> +user.hideTimezone::
>>> +  Override TZ to UTC for Git commits to hide user's timezone in commit
>>> +  date
>>
>> One level of indentation in this codebase is a single HT.
>>
>> Unterminated sentence.
>
> What does HT stands for? I will change the indentation to 8 spaces.

HT means "horizontal tab", like might be shown with "man ascii".

Git uses tabs for indentation.  This file is documentation instead of
source so clang-format doesn't know about it, but I might as well
mention anyway: if you run "make style", then clang-format will give
some suggestions around formatting.  The configuration for that is not
yet perfect so you can take its suggestions with a grain of salt, but
they should get you in the right direction.

[...]
>>> --- a/builtin/commit.c
>>> +++ b/builtin/commit.c
>>> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>>  	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
>>>  	s.colopts = 0;
>>>  
>>> +  git_config(git_default_config, NULL);
>>
>> Declaration after statement is not tolerated in this codebase.
>
> If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler
> catches this as an error?

Yes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement.

>>> +  int hide_timezone = 0;
>>
>> Unnecessary initialization.
>>
>>> +  if (!git_config_get_bool("user.hideTimezone", &hide_timezone)  && hide_timezone)
>
> Is it unnecessary because I am checking the return value from git_config_get_bool so
> that the uninitialized value won't be used?

By leaving it uninitialized, you can help avoid the reader wondering
whether there is some code path where the default value is used.

[...]
>>             Instead, make sure it is set to some timestamp in some
>> timezone that is not UTC, and the timezone of the resulting commit
>> author date is in that timezone.  But that must have already been
>> done in basic tests on "git commit" that we honor the environment
>> variable, no?  Which means there is no need to add yet another extra
>> baseline test here.
>
> I am not sure if this test has already been done in commit basic tests.
> Will remove this test.

Let's see: *checks with "git grep -e TZ -- t"*.

Looks like t0006 tests various aspects of TZ handling pretty well and
t1100 includes of test using TZ with commit-tree (good).

Thanks and hope that helps,
Jonathan

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Junio C Hamano <gitster@pobox.com> writes:

> 
> I think (haven't actually tested) "git commit --date=<datestring>" option
> is handled the same way, i.e. comparing these two would be a way not
> to touch the environment variable.
> 
>     TZ=UTC-09 git commit --date=@1600000000 ... &&
>     TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 ... &&
>     git show -s --format='%aI' HEAD~1 >output0 &&
>     git show -s --format='%aI' HEAD~0 >output1

Like this?

test_expect_fail '...' '
         echo t1 >file &&
         git add file &&
         TZ=UTC-09 git commit --date=@1600000000 -m "t1" &&
         echo t2 >>file &&
         git add file &&
         TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 -m "t2" &&
         git show -s --format='%aI' HEAD~1 >output0 &&
         git show -s --format='%aI' HEAD~0 >output1 &&
         test_cmp output0 output1
'

I tested it.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Junio C Hamano <gitster@pobox.com> writes:

>> 	test_config user.hideTimeZone true &&
>> 	(
>> 		export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&
>> 		git commit ... &&
>> 		git show -s --format='%aI' >output &&
>> 		echo 2020-09-13T15:26:40+03:00 >expect &&
>
> Oops.  The sample date and zone must be adjusted for the values
> exported above.  I expect that we'd need to do
> 
> 		...
> 		echo 2020-09-13T12:26:40+00:00 >expect &&
> 		test_cmp expect output

Tested the following:

test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true' '
        test_config user.hideTimezone true &&
        (
                echo test2 >file &&
                git add file &&
                export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&
                git commit -m "test2" &&
                git show -s --format='%aI' >output &&
                echo 2020-09-13T12:26:40+00:00 >expect &&
                test_cmp output expect
        )
'

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Shengfa Lin <shengfa@google.com> writes:

>>> 	test_config user.hideTimeZone true &&
>>> 	(
>>> 		export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&
>>> 		git commit ... &&
>>> 		git show -s --format='%aI' >output &&
>>> 		echo 2020-09-13T15:26:40+03:00 >expect &&
>>
>> Oops.  The sample date and zone must be adjusted for the values
>> exported above.  I expect that we'd need to do
>> 
>> 		...
>> 		echo 2020-09-13T12:26:40+00:00 >expect &&
>> 		test_cmp expect output
>
> Tested the following:
> 
> test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true' '
>         test_config user.hideTimezone true &&
>         (
>                 echo test2 >file &&
>                 git add file &&
>                 export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&
>                 git commit -m "test2" &&
>                 git show -s --format='%aI' >output &&
>                 echo 2020-09-13T12:26:40+00:00 >expect &&
>                 test_cmp output expect
>         )
> '

I assume the opening and closing parentheses means the environment variables are only effective
within like an environment scope. Please correct if wrong.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Junio C Hamano <gitster@pobox.com> writes:

>> Users requested hiding location in the world from source control
>> trail. This is an implementation to read user.hideTimezone in
>> cmd_commit and set timezone to UTC if it's true.
>
> Not a very good proposed log message, that sounds as if "it is
> what 'Users' requested, so it must be a worthwhile thing to do",
> which is not the line of thinking to go by.

That's a good point. Users requested does not necessarily equivalent
to  worthwhile thing to do. Thanks for pointing it out.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Like Junio mentioned, this affects "git commit" but not other commands
> that record the current date with the local timezone.
> 
> The fundamental tool to exercise that machinery is
> 
> 	$ git var GIT_AUTHOR_IDENT
> 	Jonathan Nieder <jrnieder@gmail.com> 1601517809 -0700
> 
> so I suppose I'd be interested in seeing that exercised in tests.

Next patch should include a test for git var.

> [...]
> 
> The unfortunate thing about these APIs is that there's no way to pass
> in a timezone from a string instead of from the environment.  This
> means that passing through the environment as above is the only
> reasonable way to do it, but that would have the unfortunate result
> of changing the output of commands like "git log --date=local" that
> are about writing dates to the terminal instead of storing them.
> 

The TZ should apply for write but not read.

> [...]
> Looking over callers, who would this affect?  There are three callers:
> 
>  fast-import.c::parse_ident:
> 	Used to handle ident string "now".  That seems in keeping with
> 	the intent here, and fast-import does respect some other
> 	configuration though only affecting storage.  Seems fine.sensible.
> 
>  ident.c::ident_default_date:
> 	Used to produce author and committer timestamps and timestamps for
> 	reflog entries.  That's the goal; good.
> 
>  send-pack.c::generate_push_cert:
> 	Used for the timestamp sent to the server in a signed push
> 	certificate.  Also good.
> 

Thanks for the analysis here. I was wondering whether the TZ should
affect these callers.

> So I think this does the right thing, plus it retains the
> user-friendly feature of being able to *display* timestamps in their
> local timezone.
> 
> Now let's talk through the downsides:
> 
> It's complex.  The performance isn't likely to be bad when
> user.timezone is not set, which is nice, but it still is messier than
> I'd like to see.
> 

+1.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Shengfa Lin]

Junio C Hamano <gitster@pobox.com> writes:

Shengfa Lin <shengfa@google.com> writes:

>> Discussed with Jonathan and Junio regarding whether we should support
>> letting user specify timezone or restricted to UTC only. There was no
>> defnite conclusion.
>
> I do not think was involved in that part of the discussion to
> consider UTC vs custom zone, though.
> 
> What I said is that git () { TZ=UTC command git "$@" } is simple
> enough that I doubt it is worth the engineering effort to either
> read configuration file early (which is tricky) so that setenv() can
> be done early in cmd_main() and/or audit the codebase widely enough
> (which is time consuming) to make sure that we pay attention to the
> configuration variable in all codepaths that matter to do the
> setenv().

Thanks for clarifying.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Shengfa Lin]

Junio C Hamano <gitster@pobox.com> writes:

>> [...]

> Regarding the implementation, the posted patch to "git commit" uses
> the "futz with TZ early inside the git process" approach, but I
> think we should not do so for another reason.  Our setenv() may not
> be early enough---before the code that decides to call a setenv()
> is run, there are many things that are outside your control (like
> the tracing subsystem, repository discovery, etc.) will run, and if
> any of them does something that triggers tzset() to be called, it
> will be done with the value of TZ the process started with, and our
> setenv() that happens much later won't have any effect to it.
> 

Setenv for TZ early in the git process even at the beginning of
cmd_main does not set TZ for things run earlier. In this sense,
user setting TZ themselves would be consistent. On the other
hand, we may not require "things" run earlier to have consistent
TZ depending on whether user time would be exposed.

> Another thing is that setting TZ ourselves would affect hooks and
> other programs we spawn as subprocess, which may or may not be
> desired, depending on the reason why the user is forcing UTC.
> 

Should not setenv for TZ bluntly because of risk of undesired side
effect

> All code that create object headers like committer, author and
> tagger lines use the same git_author_info() and git_committer_info()
> API, I think, which eventually goes to fmt_ident(), and they produce
> a pair <number of seconds since epoch, offset>.  This gives us an
> entirely different opening to tackle this issue from, because the
> "number of seconds since epoch" part won't change the value no
> matter what timezone you are in.
> 

A good candidate for handling in a centralized and controllable
manner.

> You can let the existing code produce its natural result and then
> when the "force UTC" flag is set, override the offset part to +0000
> if and only if the timezone was obtained from the current
> environment (this if-and-only-if is necessary, because you do not

I don't understand "if the timezone was obtained from the current
environment. How do we tell? getenv("TZ")?

> want to rewrite and force UTC when you run "git commit --amend"
> without the "--reset-author" option to update a commit that was
> created somewhere else to UTC).  That way, we do not have to futz
> with TZ environment or tzset.
> 
> Just something to think about.

Does this mean we need to make sure "git commit --amend"
without the "--reset-author" will not trigger regeneration of
timestamp through datestamp in "date.c" but with the
"--reset-author", it will?

Preferable not to futz with TZ environment.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Shengfa Lin]

Jonathan Nieder <jrnieder@gmail.com> writes:

>>                                                Our setenv() may not
>> be early enough---before the code that decides to call a setenv()
>> is run, there are many things that are outside your control (like
>> the tracing subsystem, repository discovery, etc.) will run, and if
>> any of them does something that triggers tzset() to be called, it
>> will be done with the value of TZ the process started with, and our
>> setenv() that happens much later won't have any effect to it.
>
> I thought about this before, but in fact it's okay: when calling a
> function like localtime() (though not localetime_r() --- see my other
> reply), tzset() is called each time so it is able to reflect any
> updates to the TZ envvar from the interim.
> 

My understanding is that it's concerning that setenv may not be early
enough that not all things have the same TZ; thus, it's still possible
to expose the user timezone.

> [....]
> 
> In addition to not having to futz with TZ, I think I like the
> semantics better.  The motivation that started this thread was not so
> much "I want to set a custom timezone to blend in" but rather "why are
> we recording the timezone at all here?"  In that context, it makes
> sense to me to have a setting such as
> 
> 	core.recordTimeZone
> 
> that I can turn *off* to say that I don't think datestamp() callers
> should consider the timezone to be information worth recording (and
> instead they should write +0000).  To me that seems a little simpler
> to understand than user.hideTimezone since this focuses on turning
> some functionality off (recording of the time zone) instead of turning
> on a new stealth mode.
> 
> Thanks,
> Jonathan

+1, simpler to understand.
Using user.hideTimezone because user is
trying to hide timezone sounds a little off; directly instructing the
system is more straight forward.
If we have a setting of "core.recordTimeZone", do we need to make it
as a command option as Junio suggested earlier?

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

>> In addition to not having to futz with TZ, I think I like the
>> semantics better.  The motivation that started this thread was not so
>> much "I want to set a custom timezone to blend in" but rather "why are
>> we recording the timezone at all here?"  In that context, it makes
>> sense to me to have a setting such as
>> 
>> 	core.recordTimeZone
>> 
>> that I can turn *off* to say that I don't think datestamp() callers
>> should consider the timezone to be information worth recording (and
>> instead they should write +0000).  To me that seems a little simpler
>> to understand than user.hideTimezone since this focuses on turning
>> some functionality off (recording of the time zone) instead of turning
>> on a new stealth mode.
>> 
>> Thanks,
>> Jonathan
>
> +1, simpler to understand.

Yup.

> If we have a setting of "core.recordTimeZone", do we need to make it
> as a command option as Junio suggested earlier?

Usually we add command line option --[no-]record-time-zone first
without configuration option when introducing a new features like
this one.  Once the feature proves useful, we'd add a matching
configuration variable for convenience, but leave the command line
option (negative form in this case) so that a configured per-user
default can be overridden as needed.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Shengfa Lin]

Jonathan Nieder <jrnieder@gmail.com> writes:

>>> [...]
>>
>> What does HT stands for? I will change the indentation to 8 spaces.
>
> HT means "horizontal tab", like might be shown with "man ascii".
> 
> Git uses tabs for indentation.  This file is documentation instead of
> source so clang-format doesn't know about it, but I might as well
> mention anyway: if you run "make style", then clang-format will give
> some suggestions around formatting.  The configuration for that is not
> yet perfect so you can take its suggestions with a grain of salt, but
> they should get you in the right direction.
>

Got it, thanks!

>[...]
>>>> --- a/builtin/commit.c
>>>> +++ b/builtin/commit.c
>>>> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>>>  	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
>>>>  	s.colopts = 0;
>>>>  
>>>> +  git_config(git_default_config, NULL);
>>>
>>> Declaration after statement is not tolerated in this codebase.
>>
>> If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler
>> catches this as an error?
>
> Yes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement.
>

Got the error from int hide_timezone = 0; but not
git_config(git_default_config, NULL);.

>>>> +  int hide_timezone = 0;
>>>
>>> Unnecessary initialization.
>>>
>>>> +  if (!git_config_get_bool("user.hideTimezone", &hide_timezone)  && hide_timezone)
>>
>> Is it unnecessary because I am checking the return value from git_config_get_bool so
>> that the uninitialized value won't be used?
>
> By leaving it uninitialized, you can help avoid the reader wondering
> whether there is some code path where the default value is used.
>

I see.

>[...]
>>>             Instead, make sure it is set to some timestamp in some
>>> timezone that is not UTC, and the timezone of the resulting commit
>>> author date is in that timezone.  But that must have already been
>>> done in basic tests on "git commit" that we honor the environment
>>> variable, no?  Which means there is no need to add yet another extra
>>> baseline test here.
>>
>> I am not sure if this test has already been done in commit basic tests.
>> Will remove this test.
>
> Let's see: *checks with "git grep -e TZ -- t"*.
> 
> Looks like t0006 tests various aspects of TZ handling pretty well and
> t1100 includes of test using TZ with commit-tree (good).
> 
> Thanks and hope that helps,
> Jonathan

Thanks! That helps a lot.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Shengfa Lin]

Junio C Hamano <gitster@pobox.com> writes:

>> If we have a setting of "core.recordTimeZone", do we need to make it
>> as a command option as Junio suggested earlier?
>
> Usually we add command line option --[no-]record-time-zone first
> without configuration option when introducing a new features like
> this one.  Once the feature proves useful, we'd add a matching
> configuration variable for convenience, but leave the command line
> option (negative form in this case) so that a configured per-user
> default can be overridden as needed.

Any recommendation as to where to add this command line option?
Should the option be added separately in "git merge", "git rebase",
"git commit", "git pull" and other places as OPT_BOOL?
Or could it be added directly to handle_options in git.c?
If added to git.c, it will only be added it once.
Then an environment variable(such as GIT_RECORD_TIME_ZONE) can be added
to save the option and datestamp in date.c can set offset accordingly.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> If we have a setting of "core.recordTimeZone", do we need to make it
>>> as a command option as Junio suggested earlier?
>>
>> Usually we add command line option --[no-]record-time-zone first
>> without configuration option when introducing a new features like
>> this one.  Once the feature proves useful, we'd add a matching
>> configuration variable for convenience, but leave the command line
>> option (negative form in this case) so that a configured per-user
>> default can be overridden as needed.
>
> Any recommendation as to where to add this command line option?
> Should the option be added separately in "git merge", "git rebase",
> "git commit", "git pull" and other places as OPT_BOOL?

I think that would be most end-user friendly.

> Or could it be added directly to handle_options in git.c?
> If added to git.c, it will only be added it once.

That would be easier for lazy developers, but not as end-user
friendly as giving it to individual commands, I would think.

> Then an environment variable(such as GIT_RECORD_TIME_ZONE) can be added
> to save the option and datestamp in date.c can set offset accordingly.

We prefer not to use new environment variable unless there is a
clear advantage.  Consider GIT_DIR etc. that were added early in
Git's history as historical mistakes.

Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

>>>>> +  git_config(git_default_config, NULL);
>>>>
>>>> Declaration after statement is not tolerated in this codebase.
>>>
>>> If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler
>>> catches this as an error?
>>
>> Yes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement.
>
> Got the error from int hide_timezone = 0; but not
> git_config(git_default_config, NULL);.
> ...
>>>>> +  int hide_timezone = 0;

That is exactly expected.

The problematic sequence was

	...
	s.colopts = 0;

	git_config(git_default_config, NULL);
	int hide_timezone = 0;

The assignment of 0 to s.colopts, which existed before your patch,
was already a statement.  So is a new call to git_config() function
you added.  As the existing "s.colopts = 0" were in a place where a
statement can legally appear, the place you added git_config() call
is also where a statement can legally appear.  There is nothing for
the compiler to complain about.

The declaration of hide_timezone added _after_ that statement is a
different story.  Because the -Wdeclaration-after-statement is about
starting a function (or "a block" in general) with all the necessary
declarations before we write a single statement, and also forbidding
us from adding other declarations after we write a statement.  Since
there are bunch of statements in the same block already, including
the assignment to s.colopts and a call to git_config(), we cannot
add a declaration after that.

Thanks.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

On 2020-10-01 at 03:43:50, Jonathan Nieder wrote:
> Junio C Hamano wrote:
> > You can let the existing code produce its natural result and then
> > when the "force UTC" flag is set, override the offset part to +0000
> > if and only if the timezone was obtained from the current
> > environment (this if-and-only-if is necessary, because you do not
> > want to rewrite and force UTC when you run "git commit --amend"
> > without the "--reset-author" option to update a commit that was
> > created somewhere else to UTC).  That way, we do not have to futz
> > with TZ environment or tzset.
> 
> Yes, I think this is simpler and nicer than the proposal in my other
> reply.

Yeah, I agree this is more desirable.

> In addition to not having to futz with TZ, I think I like the
> semantics better.  The motivation that started this thread was not so
> much "I want to set a custom timezone to blend in" but rather "why are
> we recording the timezone at all here?"  In that context, it makes
> sense to me to have a setting such as
> 
> 	core.recordTimeZone
> 
> that I can turn *off* to say that I don't think datestamp() callers
> should consider the timezone to be information worth recording (and
> instead they should write +0000).  To me that seems a little simpler
> to understand than user.hideTimezone since this focuses on turning
> some functionality off (recording of the time zone) instead of turning
> on a new stealth mode.

I'd like to make one suggestion here, and that's that instead of writing
"+0000" in this case, we write "-0000".  As far as I'm aware, it should
be parsed equivalently but it mirrors RFC 5322:

  Though "-0000" also indicates Universal Time, it is used to indicate
  that the time was generated on a system that may be in a local time
  zone other than Universal Time and that the date-time contains no
  information about the local time zone.

This is exactly my case.  As you can tell from my emails, I'm not
physically located in a UTC timezone, but my system is in UTC and uses
that for timestamps.  I use UTC because I know and work with people from
around the world and it's more convenient to use an objective standard;
my real time zone is unimportant.  That's materially different than
someone who's located in Reykjavík, where we'd want to write +0000,
since they are physically located in a UTC-equivalent timezone.
-- 
brian m. carlson: Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I'd like to make one suggestion here, and that's that instead of writing
> "+0000" in this case, we write "-0000".  As far as I'm aware, it should
> be parsed equivalently but it mirrors RFC 5322:
>
>   Though "-0000" also indicates Universal Time, it is used to indicate
>   that the time was generated on a system that may be in a local time
>   zone other than Universal Time and that the date-time contains no
>   information about the local time zone.
>
> This is exactly my case.  As you can tell from my emails, I'm not
> physically located in a UTC timezone, but my system is in UTC and uses
> that for timestamps.

What you had in the header of the message I am responding to was:

    Date:   Sat, 3 Oct 2020 19:53:25 +0000

I cannot tell if somebody in the middle rewrote -0000 to +0000, or
your original already said +0000 in your message.

> I use UTC because I know and work with people from
> around the world and it's more convenient to use an objective standard;
> my real time zone is unimportant.

It cuts both ways, and there is a small downside, though.  As I
assume most people are not actively hacking on Git in the early
morning say 01am-04am in their local timezone, I may delay my
response to a message from a contributor if I notice that the day
for that contributor is already over and my message will not be read
for some time even if I rushed it.  "real time zone is unimportant"
may or may not be true.  A responder in such a case is actively
inconvenienced by the zone information being hidden [*1*].

Having said that.

> That's materially different than
> someone who's located in Reykjavík, where we'd want to write +0000,
> since they are physically located in a UTC-equivalent timezone.

I agree that it would be a good thing to have a way to say "I am not
telling which zone the timestamp is from", that can be used to
differenciate "This timestamp is in UTC because I am in UTC".


[Footnote]

*1* How much the person feel inconvenienced is very subjective, so
    let's not go into "it's just a small inconvenience---my privacy
    is more valuable" kind of discussion.  The value of privacy is
    subjective in the same way and we shouldn't compare subjective
    things to decide which side must bend their way to accomodate
    the other side.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> In addition to not having to futz with TZ, I think I like the
>> semantics better.  The motivation that started this thread was not so
>> much "I want to set a custom timezone to blend in" but rather "why are
>> we recording the timezone at all here?"  In that context, it makes
>> sense to me to have a setting such as
>>
>> 	core.recordTimeZone
>>
>> that I can turn *off* to say that I don't think datestamp() callers
>> should consider the timezone to be information worth recording (and
>> instead they should write +0000).  To me that seems a little simpler
>> to understand than user.hideTimezone since this focuses on turning
>> some functionality off (recording of the time zone) instead of turning
>> on a new stealth mode.
>
> Hmph.  It is a valid way to look at the issue, I guess.
>
> Thanks for an input.

I do not have a strong opinion on recordTimeZone vs hideTimeZone any
more (the latter, as proposed by Shengfa, is shorter to type ;-),
but I think it is a good idea to keep it in user.* hierarchy.  It
sits next to user.name and user.email and controls how the user ident
is formulated.

Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone

[Junio C Hamano]

Nathaniel Manista <nathaniel@google.com> writes:

> If we say that we would like to eventually live in a world in which a
> user's time zones are not recorded until after that user deliberately opts
> into git recording zir time zones, does that point us toward an eventual
> destination of "users who don't wish to record their time zones don't have
> to do anything to their .gitconfigs, and users who do wish to record their
> time zones only have to write one line in their .gitconfigs"? If that's the
> case, ought that one line that some users choose to write be targeted to be
> "recordTimeZone = true" rather than anything else (particularly rather than
> "hideTimeZone = false")? If all that holds, does
>
> today: introduce recordTimeZone with a default of true and advertise its
> existence so that those users who have feelings one way or the other on the
> matter may explicitly set it to one value or the other
> six months from today: flip recordTimeZone's default from true to false
>
> look like a plausible sketch of how to get to the desired future state?
>
> What am I missing (and 🤞 that my "if"s hold too...)?

Since it is perfectly OK to have a configuration variable whose
default value is 'true', the choice between the world in which times
are by default hidden and the opposite world does not affect which
way the configuration variable should be named and defined at all.

The system could hide zone when there is no user.hideTimeZone
configured, and those who think that the value of giving others a
useful bit of info outweighs the value of hiding their own zone can
set it explicitly to 'false' to expose their zone.

Or the system could record zone when there is no user.recordTimeZone
configured, and you can set it explicitly to 'false' if you think if
it is more valuable to omit your zone from the commit than recording
the zone in which the commit was written.

Either way, we can make the system to "do what the majority of users
want, with an escape hatch to protect monority"; the naming does not
affect it in any way.

The only thing we should consider when naming, therefore, is how
clearly the name conveys the semantics.

And from that point of view, user.hideTimeZone=yes is a mistake.  It
can be mistaken as if we do record but simply choose not to show at
time of the output.  Contrast that with user.recordTimeZone=no which
has no such room for confusion.  If we are not recording, there is
nothing clever to be done when showing.

FWIW, I am not in favor of dropping info by default myself, but that
is outside the scope of this message.

[WIP v2 0/2] experiment with commit option record-time-zone

[Shengfa Lin]

My current understanding is that we should start with providing command
options first and if they prove to be useful, we can progress with
config such as user.recordTimeZone. Also, we wanted to save the
timestamp as -0000 when user specify not to record time zone so to
distinguish with +0000(user from UTC time zone).

Experimenting to support command option --[no-]record-time-zone, currently only
added for commit command; the plan is to add it to other time zone
recording command such as rebase, pull and merge. Documentation change
is also missing.

I added three tests where the first two are failing. I am not sure why
they are failing. I am guessing the datestamp method is not called when
specifying the date or GIT_AUTHOR_DATE. Also, I am not sure the correct
handling of all the show_date calls yet. Anyway, I wanted to share WIP
before delaying longer.


Shengfa Lin (2):
  Adding a record-time-zone command option for commit
  Demonstrate failing and passing tests

 builtin/am.c            |  2 +-
 builtin/blame.c         |  4 +-
 builtin/commit.c        |  3 +-
 builtin/fast-import.c   |  2 +-
 builtin/show-branch.c   |  2 +-
 builtin/tag.c           |  2 +-
 cache.h                 | 10 +++--
 date.c                  | 96 ++++++++++++++++++++++++++++-------------
 http-backend.c          |  2 +-
 pretty.c                |  6 ++-
 ref-filter.c            |  2 +-
 reflog-walk.c           |  2 +-
 refs.c                  |  6 ++-
 sha1-name.c             |  2 +-
 t/helper/test-date.c    |  8 ++--
 t/t7514-commit-patch.sh | 28 ++++++++++++
 16 files changed, 126 insertions(+), 51 deletions(-)

-- 
2.28.0.1011.ga647a8990f-goog

[WIP v2 1/2] Adding a record-time-zone command option for commit

[Shengfa Lin]

Many places in Git record the timezone of actor when a
timestamp is recorded, including the commiter and author
timestamps in a commit object and the tagger timestamp in a tag
object. Some people however prefer not to share where they
actually are.

They _could_ just say "export TZ=UTC" and be done with it,
but the method would not easily allow them to pretend to be
in the UTC timezone only with Git, while revealing their true
timezone to other activities (e.g. sending e-emails?).

Introduce --[no-]record-time-zone for commit command, which can be
specified to disallow Git to record time zone. Timezone will be
recorded by default.

Note that this is a WIP and the test is failing.

Signed-off-by: Shengfa Lin <shengfa@google.com>
---
 builtin/am.c            |  2 +-
 builtin/blame.c         |  4 +-
 builtin/commit.c        |  3 +-
 builtin/fast-import.c   |  2 +-
 builtin/show-branch.c   |  2 +-
 builtin/tag.c           |  2 +-
 cache.h                 | 10 +++--
 date.c                  | 96 ++++++++++++++++++++++++++++-------------
 http-backend.c          |  2 +-
 pretty.c                |  6 ++-
 ref-filter.c            |  2 +-
 reflog-walk.c           |  2 +-
 refs.c                  |  6 ++-
 sha1-name.c             |  2 +-
 t/helper/test-date.c    |  8 ++--
 t/t7514-commit-patch.sh |  9 ++++
 16 files changed, 107 insertions(+), 51 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 2c7673f74e..059cc5fce7 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -884,7 +884,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
 			if (tz > 0)
 				tz2 = -tz2;
 
-			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, DATE_MODE(RFC2822)));
+			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, NULL, DATE_MODE(RFC2822)));
 		} else if (starts_with(sb.buf, "# ")) {
 			continue;
 		} else {
diff --git a/builtin/blame.c b/builtin/blame.c
index bb0f29300e..8205f01868 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -306,7 +306,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 		size_t time_width;
 		int tz;
 		tz = atoi(tz_str);
-		time_str = show_date(time, tz, &blame_date_mode);
+		time_str = show_date(time, tz, NULL, &blame_date_mode);
 		strbuf_addstr(&time_buf, time_str);
 		/*
 		 * Add space paddings to time_buf to display a fixed width
@@ -1002,7 +1002,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
 		break;
 	case DATE_STRFTIME:
-		blame_date_width = strlen(show_date(0, 0, &blame_date_mode)) + 1; /* add the null */
+		blame_date_width = strlen(show_date(0, 0, NULL, &blame_date_mode)) + 1; /* add the null */
 		break;
 	}
 	blame_date_width -= 1; /* strip the null */
diff --git a/builtin/commit.c b/builtin/commit.c
index 1dfd799ec5..ee3ca2c7ac 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1547,7 +1547,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 				N_("ok to record an empty change")),
 		OPT_HIDDEN_BOOL(0, "allow-empty-message", &allow_empty_message,
 				N_("ok to record a change with an empty message")),
-
+                OPT_BOOL(0, "record-time-zone", &record_time_zone, N_("record user timezone")),
 		OPT_END()
 	};
 
@@ -1580,6 +1580,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
 					  prefix, current_head, &s);
+
 	if (verbose == -1)
 		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
 
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 1bf50a73dc..5aebe70670 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -327,7 +327,7 @@ static void write_crash_report(const char *err)
 	fprintf(rpt, "fast-import crash report:\n");
 	fprintf(rpt, "    fast-import process: %"PRIuMAX"\n", (uintmax_t) getpid());
 	fprintf(rpt, "    parent process     : %"PRIuMAX"\n", (uintmax_t) getppid());
-	fprintf(rpt, "    at %s\n", show_date(time(NULL), 0, DATE_MODE(ISO8601)));
+	fprintf(rpt, "    at %s\n", show_date(time(NULL), 0, NULL, DATE_MODE(ISO8601)));
 	fputc('\n', rpt);
 
 	fputs("fatal: ", rpt);
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d6d2dabeca..e4210c06ab 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -777,7 +777,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			else
 				msg++;
 			reflog_msg[i] = xstrfmt("(%s) %s",
-						show_date(timestamp, tz,
+						show_date(timestamp, tz, NULL,
 							  DATE_MODE(RELATIVE)),
 						msg);
 			free(logmsg);
diff --git a/builtin/tag.c b/builtin/tag.c
index ecf011776d..6ee3c7109e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -332,7 +332,7 @@ static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)
 		free(buf);
 
 		if ((c = lookup_commit_reference(the_repository, oid)) != NULL)
-			strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
+			strbuf_addf(sb, ", %s", show_date(c->date, 0, NULL, DATE_MODE(SHORT)));
 		break;
 	case OBJ_TREE:
 		strbuf_addstr(sb, "tree object");
diff --git a/cache.h b/cache.h
index c0072d43b1..84f9e88649 100644
--- a/cache.h
+++ b/cache.h
@@ -1625,17 +1625,18 @@ struct date_mode {
 /*
  * Convenience helper for passing a constant type, like:
  *
- *   show_date(t, tz, DATE_MODE(NORMAL));
+ *   show_date(t, tz, NULL, DATE_MODE(NORMAL));
  */
 #define DATE_MODE(t) date_mode_from_type(DATE_##t)
 struct date_mode *date_mode_from_type(enum date_mode_type type);
 
-const char *show_date(timestamp_t time, int timezone, const struct date_mode *mode);
+const char *show_date(timestamp_t time, int timezone, int *sign, const struct date_mode *mode);
 void show_date_relative(timestamp_t time, struct strbuf *timebuf);
 void show_date_human(timestamp_t time, int tz, const struct timeval *now,
 			struct strbuf *timebuf);
 int parse_date(const char *date, struct strbuf *out);
-int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset);
+int parse_date_basic(const char *date, timestamp_t *timestamp,
+                     int *offset, int *zero_offset_negative_sign);
 int parse_expiry_date(const char *date, timestamp_t *timestamp);
 void datestamp(struct strbuf *out);
 #define approxidate(s) approxidate_careful((s), NULL)
@@ -1979,4 +1980,7 @@ int print_sha1_ellipsis(void);
 /* Return 1 if the file is empty or does not exists, 0 otherwise. */
 int is_empty_or_missing_file(const char *filename);
 
+/* date.c */
+extern int record_time_zone;
+
 #endif /* CACHE_H */
diff --git a/date.c b/date.c
index f9ea807b3a..f32ab51f2b 100644
--- a/date.c
+++ b/date.c
@@ -6,6 +6,8 @@
 
 #include "cache.h"
 
+int record_time_zone = 1;
+
 /*
  * This is like mktime, but without normalization of tm_wday and tm_yday.
  */
@@ -213,7 +215,21 @@ struct date_mode *date_mode_from_type(enum date_mode_type type)
 	return &mode;
 }
 
-static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, struct tm *human_tm, int human_tz, int local)
+static int negative_zero(int tz, int *sign)
+{
+        return !tz && sign && (*sign) == '-';
+}
+
+static const char *tz_fmt(int tz, int *sign)
+{
+        if (!negative_zero(tz, sign))
+                return " %+05d";
+        else
+                return " -%04d";
+}
+
+
+static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, int *sign, struct tm *human_tm, int human_tz, int local)
 {
 	struct {
 		unsigned int	year:1,
@@ -277,10 +293,10 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm
 		strbuf_addf(buf, " %d", tm->tm_year + 1900);
 
 	if (!hide.tz)
-		strbuf_addf(buf, " %+05d", tz);
+                strbuf_addf(buf, tz_fmt(tz, sign), tz);
 }
 
-const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
+const char *show_date(timestamp_t time, int tz, int *signp, const struct date_mode *mode)
 {
 	struct tm *tm;
 	struct tm tmbuf = { 0 };
@@ -331,15 +347,18 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 	if (mode->type == DATE_SHORT)
 		strbuf_addf(&timebuf, "%04d-%02d-%02d", tm->tm_year + 1900,
 				tm->tm_mon + 1, tm->tm_mday);
-	else if (mode->type == DATE_ISO8601)
-		strbuf_addf(&timebuf, "%04d-%02d-%02d %02d:%02d:%02d %+05d",
+	else if (mode->type == DATE_ISO8601) {
+		strbuf_addf(&timebuf, "%04d-%02d-%02d %02d:%02d:%02d",
 				tm->tm_year + 1900,
 				tm->tm_mon + 1,
 				tm->tm_mday,
-				tm->tm_hour, tm->tm_min, tm->tm_sec,
-				tz);
+				tm->tm_hour, tm->tm_min, tm->tm_sec);
+		strbuf_addf(&timebuf, tz_fmt(tz, signp), tz);
+        }
 	else if (mode->type == DATE_ISO8601_STRICT) {
 		char sign = (tz >= 0) ? '+' : '-';
+                if (negative_zero(tz, signp))
+                        sign = '-';
 		tz = abs(tz);
 		strbuf_addf(&timebuf, "%04d-%02d-%02dT%02d:%02d:%02d%c%02d:%02d",
 				tm->tm_year + 1900,
@@ -347,16 +366,18 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 				tm->tm_mday,
 				tm->tm_hour, tm->tm_min, tm->tm_sec,
 				sign, tz / 100, tz % 100);
-	} else if (mode->type == DATE_RFC2822)
-		strbuf_addf(&timebuf, "%.3s, %d %.3s %d %02d:%02d:%02d %+05d",
+	} else if (mode->type == DATE_RFC2822) {
+		strbuf_addf(&timebuf, "%.3s, %d %.3s %d %02d:%02d:%02d",
 			weekday_names[tm->tm_wday], tm->tm_mday,
 			month_names[tm->tm_mon], tm->tm_year + 1900,
-			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
+			tm->tm_hour, tm->tm_min, tm->tm_sec);
+		strbuf_addf(&timebuf, tz_fmt(tz, signp), tz);
+        }
 	else if (mode->type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
 				!mode->local);
 	else
-		show_date_normal(&timebuf, time, tm, tz, &human_tm, human_tz, mode->local);
+		show_date_normal(&timebuf, time, tm, tz, signp, &human_tm, human_tz, mode->local);
 	return timebuf.buf;
 }
 
@@ -786,14 +807,16 @@ static int match_tz(const char *date, int *offp)
 	return end - date;
 }
 
-static void date_string(timestamp_t date, int offset, struct strbuf *buf)
+static void date_string(timestamp_t date, int offset,
+                        int zero_offset_negative_sign, struct strbuf *buf)
 {
 	int sign = '+';
+        if (offset < 0) {
+                offset = -offset;
+                sign = '-';
+        } else if (!offset && zero_offset_negative_sign)
+                sign = '-';
 
-	if (offset < 0) {
-		offset = -offset;
-		sign = '-';
-	}
 	strbuf_addf(buf, "%"PRItime" %c%02d%02d", date, sign, offset / 60, offset % 60);
 }
 
@@ -826,17 +849,21 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in
 
 /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822
    (i.e. English) day/month names, and it doesn't work correctly with %z. */
-int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
+int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset, int *zero_offset_negative_sign)
 {
 	struct tm tm;
 	int tm_gmt;
 	timestamp_t dummy_timestamp;
 	int dummy_offset;
+        int dummy_zero_offset_negative_sign;
+        int negative_sign;
 
 	if (!timestamp)
 		timestamp = &dummy_timestamp;
 	if (!offset)
 		offset = &dummy_offset;
+        if (!zero_offset_negative_sign)
+                zero_offset_negative_sign = &dummy_zero_offset_negative_sign;
 
 	memset(&tm, 0, sizeof(tm));
 	tm.tm_year = -1;
@@ -848,6 +875,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
 	tm.tm_sec = -1;
 	*offset = -1;
 	tm_gmt = 0;
+        negative_sign = 0;
 
 	if (*date == '@' &&
 	    !match_object_header_date(date + 1, timestamp, offset))
@@ -864,9 +892,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
 			match = match_alpha(date, &tm, offset);
 		else if (isdigit(c))
 			match = match_digit(date, &tm, offset, &tm_gmt);
-		else if ((c == '-' || c == '+') && isdigit(date[1]))
+		else if ((c == '-' || c == '+') && isdigit(date[1])) {
 			match = match_tz(date, offset);
-
+                        if (c == '-')
+                                negative_sign = 1;
+                }
 		if (!match) {
 			/* BAD CRAP */
 			match = 1;
@@ -895,6 +925,9 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
 
 	if (!tm_gmt)
 		*timestamp -= *offset * 60;
+
+        *zero_offset_negative_sign = (!(*offset) && negative_sign);
+
 	return 0; /* success */
 }
 
@@ -924,9 +957,10 @@ int parse_date(const char *date, struct strbuf *result)
 {
 	timestamp_t timestamp;
 	int offset;
-	if (parse_date_basic(date, &timestamp, &offset))
+        int zero_offset_negative_sign;
+	if (parse_date_basic(date, &timestamp, &offset, &zero_offset_negative_sign))
 		return -1;
-	date_string(timestamp, offset, result);
+	date_string(timestamp, offset, zero_offset_negative_sign, result);
 	return 0;
 }
 
@@ -996,15 +1030,16 @@ void parse_date_format(const char *format, struct date_mode *mode)
 void datestamp(struct strbuf *out)
 {
 	time_t now;
-	int offset;
-	struct tm tm = { 0 };
+	int offset = 0;
 
-	time(&now);
+        time(&now);
+        if (record_time_zone) {
+          struct tm tm = { 0 };
+          offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
+          offset /= 60;
+        }
 
-	offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
-	offset /= 60;
-
-	date_string(now, offset, out);
+	date_string(now, offset, !record_time_zone, out);
 }
 
 /*
@@ -1330,7 +1365,7 @@ timestamp_t approxidate_relative(const char *date)
 	int offset;
 	int errors = 0;
 
-	if (!parse_date_basic(date, &timestamp, &offset))
+	if (!parse_date_basic(date, &timestamp, &offset, NULL))
 		return timestamp;
 
 	get_time(&tv);
@@ -1346,7 +1381,7 @@ timestamp_t approxidate_careful(const char *date, int *error_ret)
 	if (!error_ret)
 		error_ret = &dummy;
 
-	if (!parse_date_basic(date, &timestamp, &offset)) {
+	if (!parse_date_basic(date, &timestamp, &offset, NULL)) {
 		*error_ret = 0;
 		return timestamp;
 	}
@@ -1371,3 +1406,4 @@ int date_overflows(timestamp_t t)
 	sys = t;
 	return t != sys || (t < 1) != (sys < 1);
 }
+
diff --git a/http-backend.c b/http-backend.c
index a03b4bae22..644c4c7af6 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -97,7 +97,7 @@ static void hdr_int(struct strbuf *hdr, const char *name, uintmax_t value)
 
 static void hdr_date(struct strbuf *hdr, const char *name, timestamp_t when)
 {
-	const char *value = show_date(when, 0, DATE_MODE(RFC2822));
+	const char *value = show_date(when, 0, NULL, DATE_MODE(RFC2822));
 	hdr_str(hdr, name, value);
 }
 
diff --git a/pretty.c b/pretty.c
index 7a7708a0ea..9b86e4ec12 100644
--- a/pretty.c
+++ b/pretty.c
@@ -416,6 +416,7 @@ const char *show_ident_date(const struct ident_split *ident,
 {
 	timestamp_t date = 0;
 	long tz = 0;
+        int sign = '+';
 
 	if (ident->date_begin && ident->date_end)
 		date = parse_timestamp(ident->date_begin, NULL, 10);
@@ -426,8 +427,11 @@ const char *show_ident_date(const struct ident_split *ident,
 			tz = strtol(ident->tz_begin, NULL, 10);
 		if (tz >= INT_MAX || tz <= INT_MIN)
 			tz = 0;
+                else if (!tz && ident->tz_begin &&
+                         (*ident->tz_begin) == '-')
+                        sign = '-';
 	}
-	return show_date(date, tz, mode);
+	return show_date(date, tz, &sign, mode);
 }
 
 void pp_user_info(struct pretty_print_context *pp,
diff --git a/ref-filter.c b/ref-filter.c
index c62f6b4822..85bf338c99 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1136,7 +1136,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 	tz = strtol(zone, NULL, 10);
 	if ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)
 		goto bad;
-	v->s = xstrdup(show_date(timestamp, tz, &date_mode));
+	v->s = xstrdup(show_date(timestamp, tz, NULL, &date_mode));
 	v->value = timestamp;
 	return;
  bad:
diff --git a/reflog-walk.c b/reflog-walk.c
index 3a25b27d8f..2a9c7cadad 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -224,7 +224,7 @@ void get_reflog_selector(struct strbuf *sb,
 	if (commit_reflog->selector == SELECTOR_DATE ||
 	    (commit_reflog->selector == SELECTOR_NONE && force_date)) {
 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
-		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
+		strbuf_addstr(sb, show_date(info->timestamp, info->tz, NULL, dmode));
 	} else {
 		strbuf_addf(sb, "%d", commit_reflog->reflogs->nr
 			    - 2 - commit_reflog->recno);
diff --git a/refs.c b/refs.c
index fa01153151..6cb056a2d1 100644
--- a/refs.c
+++ b/refs.c
@@ -890,13 +890,15 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
 			oidcpy(cb->oid, noid);
 			if (!oideq(&cb->ooid, noid))
 				warning(_("log for ref %s has gap after %s"),
-					cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
+					cb->refname, 
+                                        show_date(cb->date, cb->tz,
+                                                  NULL, DATE_MODE(RFC2822)));
 		}
 		else if (cb->date == cb->at_time)
 			oidcpy(cb->oid, noid);
 		else if (!oideq(noid, cb->oid))
 			warning(_("log for ref %s unexpectedly ended on %s"),
-				cb->refname, show_date(cb->date, cb->tz,
+				cb->refname, show_date(cb->date, cb->tz, NULL,
 						       DATE_MODE(RFC2822)));
 		oidcpy(&cb->ooid, ooid);
 		oidcpy(&cb->noid, noid);
diff --git a/sha1-name.c b/sha1-name.c
index 0b23b86ceb..9362c20803 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -915,7 +915,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 				if (!(flags & GET_OID_QUIETLY)) {
 					warning(_("log for '%.*s' only goes back to %s"),
 						len, str,
-						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
+						show_date(co_time, co_tz, NULL, DATE_MODE(RFC2822)));
 				}
 			} else {
 				if (flags & GET_OID_QUIETLY) {
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 099eff4f0f..cf8d4574b7 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -28,7 +28,7 @@ static void show_human_dates(const char **argv)
 {
 	for (; *argv; argv++) {
 		time_t t = atoi(*argv);
-		printf("%s -> %s\n", *argv, show_date(t, 0, DATE_MODE(HUMAN)));
+		printf("%s -> %s\n", *argv, show_date(t, 0, NULL, DATE_MODE(HUMAN)));
 	}
 }
 
@@ -51,7 +51,7 @@ static void show_dates(const char **argv, const char *format)
 			arg++;
 		tz = atoi(arg);
 
-		printf("%s -> %s\n", *argv, show_date(t, tz, &mode));
+		printf("%s -> %s\n", *argv, show_date(t, tz, NULL, &mode));
 	}
 }
 
@@ -67,7 +67,7 @@ static void parse_dates(const char **argv)
 		parse_date(*argv, &result);
 		if (sscanf(result.buf, "%"PRItime" %d", &t, &tz) == 2)
 			printf("%s -> %s\n",
-			       *argv, show_date(t, tz, DATE_MODE(ISO8601)));
+			       *argv, show_date(t, tz, NULL, DATE_MODE(ISO8601)));
 		else
 			printf("%s -> bad\n", *argv);
 	}
@@ -79,7 +79,7 @@ static void parse_approxidate(const char **argv)
 	for (; *argv; argv++) {
 		timestamp_t t;
 		t = approxidate_relative(*argv);
-		printf("%s -> %s\n", *argv, show_date(t, 0, DATE_MODE(ISO8601)));
+		printf("%s -> %s\n", *argv, show_date(t, 0, NULL, DATE_MODE(ISO8601)));
 	}
 }
 
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index 998a2103c7..3ba1ff4f81 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -31,4 +31,13 @@ test_expect_success 'edit hunk "commit --dry-run -p -m message"' '
 	test -r editor_was_started
 '
 
+test_expect_success 'commit date shows timezone offset -0000 when no-record-time-zone is specified' '
+        echo test >>file &&
+        git add file &&
+        TZ=UTC-09 git commit --date=@1600000000 -m "test" --no-record-time-zone &&
+        git show -s --format='%aI' >output &&
+        echo 2020-09-13T12:26:40-00:00 >expect &&
+        test_cmp output expect
+'
+
 test_done
-- 
2.28.0.1011.ga647a8990f-goog

[WIP v2 2/2] Demonstrate failing and passing tests

[Shengfa Lin]

date=@... and GIT_AUTHOR_DATE=@... tests are failing
while unsetting GIT_AUTHOR_DATE passes

Signed-off-by: Shengfa Lin <shengfa@google.com>
---
 t/t7514-commit-patch.sh | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index 3ba1ff4f81..b87d29f020 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -31,13 +31,32 @@ test_expect_success 'edit hunk "commit --dry-run -p -m message"' '
 	test -r editor_was_started
 '
 
-test_expect_success 'commit date shows timezone offset -0000 when no-record-time-zone is specified' '
-        echo test >>file &&
+test_expect_success 'commit with --date shows timezone offset -0000 when no-record-time-zone is specified' '
+        echo test1 >>file &&
         git add file &&
-        TZ=UTC-09 git commit --date=@1600000000 -m "test" --no-record-time-zone &&
+        TZ=UTC-09 git commit --date=@1600000000 -m "test1" --no-record-time-zone &&
         git show -s --format='%aI' >output &&
         echo 2020-09-13T12:26:40-00:00 >expect &&
         test_cmp output expect
 '
 
+test_expect_success 'commit with GIT_AUTHOR_DATE shows timezone offset -0000 when no-record-time-zone is specified' '
+        echo test2 >>file &&
+        git add file &&
+        export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&
+        git commit -m "test2" --no-record-time-zone &&
+        git show -s --format='%aI' >output &&
+        echo 2020-09-13T12:26:40-00:00 >expect &&
+        test_cmp output expect
+'
+
+test_expect_success 'commit with unset GIT_AUTHOR_DATE shows timezone offset -0000 when no-record-time-zone is specified' '
+        echo test2 >>file &&
+        git add file &&
+        unset GIT_AUTHOR_DATE &&
+        TZ=UTC-09 git commit -m "test2" --no-record-time-zone &&
+        git show -s --format='%aI' >output &&
+        grep "\-00:00" output
+'
+
 test_done
-- 
2.28.0.1011.ga647a8990f-goog

Re: [WIP v2 1/2] Adding a record-time-zone command option for commit

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

> Many places in Git record the timezone of actor when a
> timestamp is recorded, including the commiter and author
> timestamps in a commit object and the tagger timestamp in a tag
> object. Some people however prefer not to share where they
> actually are.
>
> They _could_ just say "export TZ=UTC" and be done with it,
> but the method would not easily allow them to pretend to be
> in the UTC timezone only with Git, while revealing their true
> timezone to other activities (e.g. sending e-emails?).
>
> Introduce --[no-]record-time-zone for commit command, which can be
> specified to disallow Git to record time zone. Timezone will be
> recorded by default.
>
> Note that this is a WIP and the test is failing.

And there is no outline of the design decision made so far, so it is
hard to give useful review comments.

It does not help that the patch is distracting by turning tabs to
spaces and breaking alingment X-<.

> diff --git a/builtin/am.c b/builtin/am.c
> index 2c7673f74e..059cc5fce7 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -884,7 +884,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
>  			if (tz > 0)
>  				tz2 = -tz2;
>  
> -			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, DATE_MODE(RFC2822)));
> +			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, NULL, DATE_MODE(RFC2822)));

For example, it appears that tweaking "show_date()" API function
seems to be a central part of the design, as there are so many
instances like this change in the patch.  If the proposed log
message mentioned, even briefly, what the extra parameter added to
the API function meant (especially what NULL means there) upfront,
then readers can coast the part that added NULL there, like these
ones, and concentrate on the parts of this patch that matter, which
presumably uses something more interesting than NULL instead.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 1dfd799ec5..ee3ca2c7ac 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1547,7 +1547,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  				N_("ok to record an empty change")),
>  		OPT_HIDDEN_BOOL(0, "allow-empty-message", &allow_empty_message,
>  				N_("ok to record a change with an empty message")),
> -
> +                OPT_BOOL(0, "record-time-zone", &record_time_zone, N_("record user timezone")),

Our code indents with HT; get used to the style early and your
patches won't distract reviewers as much, leading to more quality
reviews and suggestions.

Likewise.  The record_time_zone global variable seems to play a
crucial role in this change, but without preparing readers by
mentioning where it is defined, what normal/default value it takes,
and who potentially touches it, in the proposed log message, it
makes reading the change harder than necessary.

A system-wide global like this is usually defined in environment.c,
by the way.  Look for say trust_executable_bit and mimic where it 
is defined and declared.

>  		OPT_END()
>  	};
>  
> @@ -1580,6 +1580,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>  					  builtin_commit_usage,
>  					  prefix, current_head, &s);
> +
>  	if (verbose == -1)
>  		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
>  

Distraction.

> +static int negative_zero(int tz, int *sign)
> +{
> +        return !tz && sign && (*sign) == '-';
> +}
> +
> +static const char *tz_fmt(int tz, int *sign)
> +{
> +        if (!negative_zero(tz, sign))
> +                return " %+05d";
> +        else
> +                return " -%04d";
> +}
> +
> +
> +static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, int *sign, struct tm *human_tm, int human_tz, int local)
>  {
>  	struct {
>  		unsigned int	year:1,
> @@ -277,10 +293,10 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm
>  		strbuf_addf(buf, " %d", tm->tm_year + 1900);
>  
>  	if (!hide.tz)
> -		strbuf_addf(buf, " %+05d", tz);
> +                strbuf_addf(buf, tz_fmt(tz, sign), tz);
>  }
>  
> -const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
> +const char *show_date(timestamp_t time, int tz, int *signp, const struct date_mode *mode)

With its type, we can tell easily that sign is a pointer, so no need
for 'p' (we do not have modep, either, next door).  More important
is if 'sign' is a good name that conveys what the parameter (which
is almost always NULL) means.  Without any introductory text, it is
hard to tell and offer recommendations.

> @@ -826,17 +849,21 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in
>  
>  /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822
>     (i.e. English) day/month names, and it doesn't work correctly with %z. */
> -int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
> +int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset, int *zero_offset_negative_sign)
>  {
>  	struct tm tm;
>  	int tm_gmt;
>  	timestamp_t dummy_timestamp;
>  	int dummy_offset;
> +        int dummy_zero_offset_negative_sign;
> +        int negative_sign;
>  	if (!timestamp)
>  		timestamp = &dummy_timestamp;
>  	if (!offset)
>  		offset = &dummy_offset;

I see no need for the extra dummy_zero_offset_negative_sign variable.
I can guess this mimics "if (!offset) offset = &dummy_offset" without
much thought---a big difference is that after calling match_tz() to
fill *offset, the code needs to obtain the value match_tz() parsed
to decide if it needs to do the mktime() dance to guess the current
zone offset, and also needs to read *offset to adjust *timestamp
the function returns.

The zero_offset_negative_sign pointer that specifies the location to
optionally return a bit of info is *ONLY* used once at the very end
of the function, so 

> +        if (!zero_offset_negative_sign)
> +                zero_offset_negative_sign = &dummy_zero_offset_negative_sign;

there is absolutely no need for the dummy variable or this
assignment, especially since the patch adds negative_sign variable
that always exists, and ...

>  	memset(&tm, 0, sizeof(tm));
>  	tm.tm_year = -1;
> @@ -848,6 +875,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>  	tm.tm_sec = -1;
>  	*offset = -1;
>  	tm_gmt = 0;
> +        negative_sign = 0;
>  
>  	if (*date == '@' &&
>  	    !match_object_header_date(date + 1, timestamp, offset))
> @@ -864,9 +892,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>  			match = match_alpha(date, &tm, offset);
>  		else if (isdigit(c))
>  			match = match_digit(date, &tm, offset, &tm_gmt);
> -		else if ((c == '-' || c == '+') && isdigit(date[1]))
> +		else if ((c == '-' || c == '+') && isdigit(date[1])) {
>  			match = match_tz(date, offset);
> -
> +                        if (c == '-')
> +                                negative_sign = 1;
> +                }

... is usable.

>  		if (!match) {
>  			/* BAD CRAP */
>  			match = 1;
> @@ -895,6 +925,9 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>  
>  	if (!tm_gmt)
>  		*timestamp -= *offset * 60;
> +
> +        *zero_offset_negative_sign = (!(*offset) && negative_sign);
> +

The only change needed for this optional extra bit return is to
make sure that the assignment happens only when it was requested by
the caller, i.e.

	if (zero_offset_negative_sign)
		*zero_offset_negative_sign = ...

Having said all that, quite honestly, I do not know if this is going
in the right direction.  Specifically, I do not offhand see why we
need such a huge surgery to date.c just to touch datestamp() and
date_string().

I may be totally off, partly due to lack of explanation of how the
patch attempts to achieve its goal, but wouldn't it be just the
matter of touching the single callsite of datestamp() in ident.c, so
that after it gets git_default_date string filled, null out the last
5 bytes in it with "-0000" if record_tz is off?

> @@ -996,15 +1030,16 @@ void parse_date_format(const char *format, struct date_mode *mode)
>  void datestamp(struct strbuf *out)
>  {
>  	time_t now;
> -	int offset;
> -	struct tm tm = { 0 };
> +	int offset = 0;
>  
> -	time(&now);
> +        time(&now);
> +        if (record_time_zone) {
> +          struct tm tm = { 0 };
> +          offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
> +          offset /= 60;
> +        }
>  
> -	offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
> -	offset /= 60;
> -
> -	date_string(now, offset, out);
> +	date_string(now, offset, !record_time_zone, out);
>  }
>  
>  /*
> @@ -1330,7 +1365,7 @@ timestamp_t approxidate_relative(const char *date)
>  	int offset;
>  	int errors = 0;
>  
> -	if (!parse_date_basic(date, &timestamp, &offset))
> +	if (!parse_date_basic(date, &timestamp, &offset, NULL))
>  		return timestamp;
>  
>  	get_time(&tv);
> @@ -1346,7 +1381,7 @@ timestamp_t approxidate_careful(const char *date, int *error_ret)
>  	if (!error_ret)
>  		error_ret = &dummy;
>  
> -	if (!parse_date_basic(date, &timestamp, &offset)) {
> +	if (!parse_date_basic(date, &timestamp, &offset, NULL)) {
>  		*error_ret = 0;
>  		return timestamp;
>  	}
> @@ -1371,3 +1406,4 @@ int date_overflows(timestamp_t t)
>  	sys = t;
>  	return t != sys || (t < 1) != (sys < 1);
>  }
> +

Re: [WIP v2 1/2] Adding a record-time-zone command option for commit

[Shengfa Lin]

Junio C Hamano <gitster@pobox.com> writes:

>> Many places in Git record the timezone of actor when a
>> timestamp is recorded, including the commiter and author
>> timestamps in a commit object and the tagger timestamp in a tag
>> object. Some people however prefer not to share where they
>> actually are.
>>
>> They _could_ just say "export TZ=UTC" and be done with it,
>> but the method would not easily allow them to pretend to be
>> in the UTC timezone only with Git, while revealing their true
>> timezone to other activities (e.g. sending e-emails?).
>>
>> Introduce --[no-]record-time-zone for commit command, which can be
>> specified to disallow Git to record time zone. Timezone will be
>> recorded by default.
>>
>> Note that this is a WIP and the test is failing.
>
> And there is no outline of the design decision made so far, so it is
> hard to give useful review comments.

Thanks for the comments and sorry for not describing the design.
I will add it here.

First, I would like to use a "global" variable to keep track of whether
record-time-zone is set and default to true. Then in various places such
as commit, pull, merge and rebase; we can add command option that can
modify this value.

Then in datestamp in date.c, we can check this value; offset would be
initialized to 0 and only be set if record_time_zone is true. Additionally,
date_string from the same file would take an extra argument to indicate if
we want to use nagative sign for zero offset. Then the timestamp along with
sign and 4 digits offset would be stored in "git_default_date" as buf
"1603255519 -0000". I think of this as the "encoding" step.

Initially, I thought this would be sufficient to show "-0000" in commit log
message. However, I found that the show_date function is used for "decoding";
converting timestamp and tz to more readable format. Then I realize the
function won't distinguish between +0 and -0 as it only takes in a tz as
argument. As a result, I added the sign pointer as an argument; the reason for
pointer being there are many call sites for the show_date function but I am not sure
if they all need to display in the new format(-0000). I used NULL to denote
not sure and just do whatever they were doing before. For show_ident_date in
pretty.c, I have extracted the sign in ident tz and pass it into show_date.
Then added helper functions in date.c to print either %+05d or -%04d depending
on tz and sign pointer.

In fmt_ident(ident.c), we are calling parse_date which calls parse_date_basic
and used the parameters it parsed to call date_string again, so I modified
parse_date_basic to parse the sign as well.

> It does not help that the patch is distracting by turning tabs to
> spaces and breaking alingment X-<.

I was assuming 1 HT is 8 spaces. Then after doing :set list in vim, I think
1 HT is actually ctrl + I. Is this correct?

>> diff --git a/builtin/am.c b/builtin/am.c
>> index 2c7673f74e..059cc5fce7 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -884,7 +884,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
>>  			if (tz > 0)
>>  				tz2 = -tz2;
>>  
>> -			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, DATE_MODE(RFC2822)));
>> +			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, NULL, DATE_MODE(RFC2822)));
>
> For example, it appears that tweaking "show_date()" API function
> seems to be a central part of the design, as there are so many
> instances like this change in the patch.  If the proposed log
> message mentioned, even briefly, what the extra parameter added to
> the API function meant (especially what NULL means there) upfront,
> then readers can coast the part that added NULL there, like these
> ones, and concentrate on the parts of this patch that matter, which
> presumably uses something more interesting than NULL instead.

Added above, the extra parameter is the decoded sign if not NULL.

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 1dfd799ec5..ee3ca2c7ac 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1547,7 +1547,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  				N_("ok to record an empty change")),
>>  		OPT_HIDDEN_BOOL(0, "allow-empty-message", &allow_empty_message,
>>  				N_("ok to record a change with an empty message")),
>> -
>> +                OPT_BOOL(0, "record-time-zone", &record_time_zone, N_("record user timezone")),
>
> Our code indents with HT; get used to the style early and your
> patches won't distract reviewers as much, leading to more quality
> reviews and suggestions.

My bad, I didn't realize I was doing it wrong all along, I was thinking that I was
sometimes missing spaces.
I use vim, is there any easy way to ensure that indent is with HT?
I was using 4 tabs since each tab is 2 spaces. Should I type in ctrl + I
instead?

> Likewise.  The record_time_zone global variable seems to play a
> crucial role in this change, but without preparing readers by
> mentioning where it is defined, what normal/default value it takes,
> and who potentially touches it, in the proposed log message, it
> makes reading the change harder than necessary.
> 
> A system-wide global like this is usually defined in environment.c,
> by the way.  Look for say trust_executable_bit and mimic where it 
> is defined and declared.

Will move to environment.c.

>>  		OPT_END()
>>  	};
>>  
>> @@ -1580,6 +1580,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>>  					  builtin_commit_usage,
>>  					  prefix, current_head, &s);
>> +
>>  	if (verbose == -1)
>>  		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
>>  
>
> Distraction.

Will remove.

>> +static int negative_zero(int tz, int *sign)
>> +{
>> +        return !tz && sign && (*sign) == '-';
>> +}
>> +
>> +static const char *tz_fmt(int tz, int *sign)
>> +{
>> +        if (!negative_zero(tz, sign))
>> +                return " %+05d";
>> +        else
>> +                return " -%04d";
>> +}
>> +
>> +
>> +static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, int *sign, struct tm *human_tm, int human_tz, int local)
>>  {
>>  	struct {
>>  		unsigned int	year:1,
>> @@ -277,10 +293,10 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm
>>  		strbuf_addf(buf, " %d", tm->tm_year + 1900);
>>  
>>  	if (!hide.tz)
>> -		strbuf_addf(buf, " %+05d", tz);
>> +                strbuf_addf(buf, tz_fmt(tz, sign), tz);
>>  }
>>  
>> -const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
>> +const char *show_date(timestamp_t time, int tz, int *signp, const struct date_mode *mode)
>
> With its type, we can tell easily that sign is a pointer, so no need
> for 'p' (we do not have modep, either, next door).  More important
> is if 'sign' is a good name that conveys what the parameter (which
> is almost always NULL) means.  Without any introductory text, it is
> hard to tell and offer recommendations.

The reason I used signp instead of sign here is because there is a variable with
name sign used in the function. Regarding "good name", maybe sign_hint is more appropriate.

>> @@ -826,17 +849,21 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in
>>  
>>  /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822
>>     (i.e. English) day/month names, and it doesn't work correctly with %z. */
>> -int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>> +int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset, int *zero_offset_negative_sign)
>>  {
>>  	struct tm tm;
>>  	int tm_gmt;
>>  	timestamp_t dummy_timestamp;
>>  	int dummy_offset;
>> +        int dummy_zero_offset_negative_sign;
>> +        int negative_sign;
>>  	if (!timestamp)
>>  		timestamp = &dummy_timestamp;
>>  	if (!offset)
>>  		offset = &dummy_offset;
>
> I see no need for the extra dummy_zero_offset_negative_sign variable.
> I can guess this mimics "if (!offset) offset = &dummy_offset" without
> much thought---a big difference is that after calling match_tz() to
> fill *offset, the code needs to obtain the value match_tz() parsed
> to decide if it needs to do the mktime() dance to guess the current
> zone offset, and also needs to read *offset to adjust *timestamp
> the function returns.
> 
> The zero_offset_negative_sign pointer that specifies the location to
> optionally return a bit of info is *ONLY* used once at the very end
> of the function, so 
>> +        if (!zero_offset_negative_sign)
>> +                zero_offset_negative_sign = &dummy_zero_offset_negative_sign;
>
> there is absolutely no need for the dummy variable or this
> assignment, especially since the patch adds negative_sign variable
> that always exists, and ...
>
>>  	memset(&tm, 0, sizeof(tm));
>>  	tm.tm_year = -1;
>> @@ -848,6 +875,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>>  	tm.tm_sec = -1;
>>  	*offset = -1;
>>  	tm_gmt = 0;
>> +        negative_sign = 0;
>>  
>>  	if (*date == '@' &&
>>  	    !match_object_header_date(date + 1, timestamp, offset))
>> @@ -864,9 +892,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>>  			match = match_alpha(date, &tm, offset);
>>  		else if (isdigit(c))
>>  			match = match_digit(date, &tm, offset, &tm_gmt);
>> -		else if ((c == '-' || c == '+') && isdigit(date[1]))
>> +		else if ((c == '-' || c == '+') && isdigit(date[1])) {
>>  			match = match_tz(date, offset);
>> -
>> +                        if (c == '-')
>> +                                negative_sign = 1;
>> +                }
>
>... is usable.

Got it, thanks for pointing out.

>>  		if (!match) {
>>  			/* BAD CRAP */
>>  			match = 1;
>> @@ -895,6 +925,9 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>>  
>>  	if (!tm_gmt)
>>  		*timestamp -= *offset * 60;
>> +
>> +        *zero_offset_negative_sign = (!(*offset) && negative_sign);
>> +
>
> The only change needed for this optional extra bit return is to
> make sure that the assignment happens only when it was requested by
> the caller, i.e.
>
> 	if (zero_offset_negative_sign)
> 		*zero_offset_negative_sign = ...
>
> Having said all that, quite honestly, I do not know if this is going
> in the right direction.  Specifically, I do not offhand see why we
> need such a huge surgery to date.c just to touch datestamp() and
> date_string().
> 
> I may be totally off, partly due to lack of explanation of how the
> patch attempts to achieve its goal, but wouldn't it be just the
> matter of touching the single callsite of datestamp() in ident.c, so
> that after it gets git_default_date string filled, null out the last
> 5 bytes in it with "-0000" if record_tz is off?
>

Changing parse_date_basic because it's called by parse_date in fmt_ident(ident.c).

Re: [WIP v2 1/2] Adding a record-time-zone command option for commit

[Junio C Hamano]

Shengfa Lin <shengfa@google.com> writes:

> Thanks for the comments and sorry for not describing the design.
> I will add it here.

Thanks.  Please do not forget to add it to the updated patch, too.
That's where it matters most---you do not necessarily have to
explain things to _me_, but you should, to everybody who will read
"git log" in the future in order to understand what we did and why.

> First, I would like to use a "global" variable to keep track of whether
> record-time-zone is set and default to true. Then in various places such
> as commit, pull, merge and rebase; we can add command option that can
> modify this value.
>
> Then in datestamp in date.c, we can check this value; offset would be
> initialized to 0 and only be set if record_time_zone is true. Additionally,
> date_string from the same file would take an extra argument to indicate if
> we want to use nagative sign for zero offset. Then the timestamp along with
> sign and 4 digits offset would be stored in "git_default_date" as buf
> "1603255519 -0000". I think of this as the "encoding" step.

Yes, we could check it in datestamp(), but ... 

> Initially, I thought this would be sufficient to show "-0000" in commit log
> message. However, I found that the show_date function is used for "decoding";
> converting timestamp and tz to more readable format. Then I realize the
> function won't distinguish between +0 and -0 as it only takes in a tz as
> argument. As a result,...

... I would have imagined that you do not have to deal with all
those complications if you don't hook this to such a low level of
the call graph.  That is why I wondered:

>> I may be totally off, ... but wouldn't it be just the
>> matter of touching the single callsite of datestamp() in ident.c, so
>> that after it gets git_default_date string filled, null out the last
>> 5 bytes in it with "-0000" if record_tz is off?

Without any change to datestamp() you made in the patch, the call to
the function from ident.c may give us back a string that ends with
the integer that is the number of seconds since epoch, and sign plus
4 digits, e.g. +0900 or -0800, that would reveal the true timezone.
I would have thought that these five bytes can be replaced with
-0000 under some condition (including "the global is set" which is a
sign that the feature is being used, but not limited to that one---
we may need to make sure the call to ident_default_date() to fill
git_default_date.buf is done on behalf of the user to get a new
timestamp to record the user's activity, not doing something like
"git commit -C <existing commit>").  I do not immediately see a
reason why such a change near the surface level, which does not
disrupt the workings of the code at lower levels, would not work.

Thanks.

Re: [WIP v2 1/2] Adding a record-time-zone command option for commit

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Yes, we could check it in datestamp(), but ... 
>
>> Initially, I thought this would be sufficient to show "-0000" in commit log
>> message. However, I found that the show_date function is used for "decoding";
>> converting timestamp and tz to more readable format. Then I realize the
>> function won't distinguish between +0 and -0 as it only takes in a tz as
>> argument. As a result,...
>
> ... I would have imagined that you do not have to deal with all
> those complications if you don't hook this to such a low level of
> the call graph.  That is why I wondered:
> ...

Let me answer some of my puzzlement myself; that is, I would have
understood the change well if it were explained to me this way, and
if that explanation matched what the patches did ;-)

The topic has two major parts.

The code that prepares the timestamp to be recorded for the current
user, who wants to record an anonymous timezone "-0000", is one (and
the easier) part.  And this part could be done all inside
ident_default_date() without touching anything in date.c; when we
need to call datestamp(), we are getting the current time for the
current user, so we can mask the timezone.

The other part is that we need to read the timestamp from existing
records, and if we choose to distinguish between timestamp in UTC
and timestamp with anonymous timezone, we'd need to devise a way to
encode the anonymous timezone differently.  It is where the extra
bit that says "this bit does not usually mean anything but only when
the offset (which is a signed integer whose valid range is set to
between -2400 to +2400 by date.c::match_tz()) is zero, and this bit
is set, the zone is anonymous" comes in.

	Side note.  I suspect the damage to the callchain can be
	limited much narrower if we didn't add this bit throughout
	the API.  What if we instead pick a number outside the valid
	range of offsets, say -10000, as a sentinel value and passed
	that throughout the code when we want an anonymous zone?

	The functions in the callchain that care about the timezone
	must understand how anonymous zone is encoded anyway, so to
	them it's a matter of using an int plus one bit or using an
	int that can have a special value.  But other functions in
	the callchain whose sole purpose (with respect to the
	timezone information) is to pass it between their caller and
	their callee as an opaque piece of data, using just a single
	integer is much less error prone---the patch does not have
	to touch them at all.

Thanks.

Re: [WIP v2 1/2] Adding a record-time-zone command option for commit

[Shengfa Lin]

>> Yes, we could check it in datestamp(), but ... 
>>
>>> Initially, I thought this would be sufficient to show "-0000" in commit log
>>> message. However, I found that the show_date function is used for "decoding";
>>> converting timestamp and tz to more readable format. Then I realize the
>>> function won't distinguish between +0 and -0 as it only takes in a tz as
>>> argument. As a result,...
>>
>> ... I would have imagined that you do not have to deal with all
>> those complications if you don't hook this to such a low level of
>> the call graph.  That is why I wondered:
>> ...
>
> Let me answer some of my puzzlement myself; that is, I would have
> understood the change well if it were explained to me this way, and
> if that explanation matched what the patches did ;-)

Yes, I agree.

> The topic has two major parts.
> 
> The code that prepares the timestamp to be recorded for the current
> user, who wants to record an anonymous timezone "-0000", is one (and
> the easier) part.  And this part could be done all inside
> ident_default_date() without touching anything in date.c; when we
> need to call datestamp(), we are getting the current time for the
> current user, so we can mask the timezone.

So for this part, there is no need to modify datestamp in dates.c.
We could modify ident_default_date buf after datestamp to set the last
5 bytes to "-0000" using strcpy.

> The other part is that we need to read the timestamp from existing
> records, and if we choose to distinguish between timestamp in UTC
> and timestamp with anonymous timezone, we'd need to devise a way to
> encode the anonymous timezone differently.  It is where the extra
> bit that says "this bit does not usually mean anything but only when
> the offset (which is a signed integer whose valid range is set to
> between -2400 to +2400 by date.c::match_tz()) is zero, and this bit
> is set, the zone is anonymous" comes in.

Yes, that's correct.

> 	Side note.  I suspect the damage to the callchain can be
> 	limited much narrower if we didn't add this bit throughout
> 	the API.  What if we instead pick a number outside the valid
> 	range of offsets, say -10000, as a sentinel value and passed
> 	that throughout the code when we want an anonymous zone?

Good idea.

> 	The functions in the callchain that care about the timezone
> 	must understand how anonymous zone is encoded anyway, so to
> 	them it's a matter of using an int plus one bit or using an
> 	int that can have a special value.  But other functions in
> 	the callchain whose sole purpose (with respect to the
> 	timezone information) is to pass it between their caller and
> 	their callee as an opaque piece of data, using just a single
> 	integer is much less error prone---the patch does not have
> 	to touch them at all.

That would be easier to follow and requires less changes as well.

> Thanks.

Thanks for the clarification. Now I think we have a much better
understanding. I will try to do a better job describing patches
next time.