* [ISSUE] Stop accessing, storing, and sharing the user's time zone @ 2019-12-05 17:14 Nathaniel Manista 2019-12-05 17:31 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 47+ messages in thread From: Nathaniel Manista @ 2019-12-05 17:14 UTC (permalink / raw) To: git [-- 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 --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone 2019-12-05 17:14 [ISSUE] Stop accessing, storing, and sharing the user's time zone Nathaniel Manista @ 2019-12-05 17:31 ` Junio C Hamano 2019-12-05 17:33 ` Randall S. Becker ` (2 subsequent siblings) 3 siblings, 0 replies; 47+ messages in thread From: Junio C Hamano @ 2019-12-05 17:31 UTC (permalink / raw) To: Nathaniel Manista; +Cc: git 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [ISSUE] Stop accessing, storing, and sharing the user's time zone 2019-12-05 17:14 [ISSUE] Stop accessing, storing, and sharing the user's time zone Nathaniel Manista 2019-12-05 17:31 ` Junio C Hamano @ 2019-12-05 17:33 ` Randall S. Becker 2019-12-05 17:43 ` Junio C Hamano 2020-09-30 23:21 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Shengfa Lin 2020-10-13 5:28 ` [WIP v2 0/2] experiment with commit option record-time-zone Shengfa Lin 3 siblings, 1 reply; 47+ messages in thread From: Randall S. Becker @ 2019-12-05 17:33 UTC (permalink / raw) To: 'Nathaniel Manista', git 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone 2019-12-05 17:33 ` Randall S. Becker @ 2019-12-05 17:43 ` Junio C Hamano 2019-12-05 17:53 ` Santiago Torres Arias 2019-12-05 18:00 ` Randall S. Becker 0 siblings, 2 replies; 47+ messages in thread From: Junio C Hamano @ 2019-12-05 17:43 UTC (permalink / raw) To: Randall S. Becker; +Cc: 'Nathaniel Manista', git "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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone 2019-12-05 17:43 ` Junio C Hamano @ 2019-12-05 17:53 ` Santiago Torres Arias 2019-12-05 18:00 ` Randall S. Becker 1 sibling, 0 replies; 47+ messages in thread From: Santiago Torres Arias @ 2019-12-05 17:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Randall S. Becker, 'Nathaniel Manista', git [-- 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 --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [ISSUE] Stop accessing, storing, and sharing the user's time zone 2019-12-05 17:43 ` Junio C Hamano 2019-12-05 17:53 ` Santiago Torres Arias @ 2019-12-05 18:00 ` Randall S. Becker 1 sibling, 0 replies; 47+ messages in thread From: Randall S. Becker @ 2019-12-05 18:00 UTC (permalink / raw) To: 'Junio C Hamano'; +Cc: 'Nathaniel Manista', git 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2019-12-05 17:14 [ISSUE] Stop accessing, storing, and sharing the user's time zone Nathaniel Manista 2019-12-05 17:31 ` Junio C Hamano 2019-12-05 17:33 ` Randall S. Becker @ 2020-09-30 23:21 ` Shengfa Lin 2020-09-30 23:21 ` [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config Shengfa Lin 2020-09-30 23:53 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Junio C Hamano 2020-10-13 5:28 ` [WIP v2 0/2] experiment with commit option record-time-zone Shengfa Lin 3 siblings, 2 replies; 47+ messages in thread From: Shengfa Lin @ 2020-09-30 23:21 UTC (permalink / raw) To: git; +Cc: nathaniel, rsbecker, santiago, gitster, 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:21 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Shengfa Lin @ 2020-09-30 23:21 ` Shengfa Lin 2020-09-30 23:41 ` Junio C Hamano ` (3 more replies) 2020-09-30 23:53 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Junio C Hamano 1 sibling, 4 replies; 47+ messages in thread From: Shengfa Lin @ 2020-09-30 23:21 UTC (permalink / raw) To: git; +Cc: nathaniel, rsbecker, santiago, gitster, 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 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:21 ` [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config Shengfa Lin @ 2020-09-30 23:41 ` Junio C Hamano 2020-10-01 0:17 ` Junio C Hamano ` (2 more replies) 2020-09-30 23:55 ` Junio C Hamano ` (2 subsequent siblings) 3 siblings, 3 replies; 47+ messages in thread From: Junio C Hamano @ 2020-09-30 23:41 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:41 ` Junio C Hamano @ 2020-10-01 0:17 ` Junio C Hamano 2020-10-02 6:07 ` Shengfa Lin 2020-10-01 0:31 ` Junio C Hamano 2020-10-02 6:02 ` Shengfa Lin 2 siblings, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-10-01 0:17 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago 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"? ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-01 0:17 ` Junio C Hamano @ 2020-10-02 6:07 ` Shengfa Lin 0 siblings, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 6:07 UTC (permalink / raw) To: gitster; +Cc: git, nathaniel, rsbecker, santiago, shengfa 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". ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:41 ` Junio C Hamano 2020-10-01 0:17 ` Junio C Hamano @ 2020-10-01 0:31 ` Junio C Hamano 2020-10-01 0:35 ` Junio C Hamano 2020-10-02 6:37 ` Shengfa Lin 2020-10-02 6:02 ` Shengfa Lin 2 siblings, 2 replies; 47+ messages in thread From: Junio C Hamano @ 2020-10-01 0:31 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-01 0:31 ` Junio C Hamano @ 2020-10-01 0:35 ` Junio C Hamano 2020-10-02 6:41 ` Shengfa Lin 2020-10-02 6:37 ` Shengfa Lin 1 sibling, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-10-01 0:35 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-01 0:35 ` Junio C Hamano @ 2020-10-02 6:41 ` Shengfa Lin 2020-10-02 6:46 ` Shengfa Lin 0 siblings, 1 reply; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 6:41 UTC (permalink / raw) To: gitster; +Cc: git, nathaniel, rsbecker, santiago, shengfa 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 ) ' ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-02 6:41 ` Shengfa Lin @ 2020-10-02 6:46 ` Shengfa Lin 0 siblings, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 6:46 UTC (permalink / raw) To: shengfa; +Cc: git, gitster, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-01 0:31 ` Junio C Hamano 2020-10-01 0:35 ` Junio C Hamano @ 2020-10-02 6:37 ` Shengfa Lin 1 sibling, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 6:37 UTC (permalink / raw) To: gitster; +Cc: git, nathaniel, rsbecker, santiago, shengfa 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:41 ` Junio C Hamano 2020-10-01 0:17 ` Junio C Hamano 2020-10-01 0:31 ` Junio C Hamano @ 2020-10-02 6:02 ` Shengfa Lin 2020-10-02 6:15 ` Jonathan Nieder 2 siblings, 1 reply; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 6:02 UTC (permalink / raw) To: gitster; +Cc: git, nathaniel, rsbecker, santiago, shengfa 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-02 6:02 ` Shengfa Lin @ 2020-10-02 6:15 ` Jonathan Nieder 2020-10-02 22:32 ` Shengfa Lin 0 siblings, 1 reply; 47+ messages in thread From: Jonathan Nieder @ 2020-10-02 6:15 UTC (permalink / raw) To: Shengfa Lin; +Cc: gitster, git, nathaniel, rsbecker, santiago 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-02 6:15 ` Jonathan Nieder @ 2020-10-02 22:32 ` Shengfa Lin 2020-10-03 4:57 ` Junio C Hamano 0 siblings, 1 reply; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 22:32 UTC (permalink / raw) To: jrnieder; +Cc: git, gitster, nathaniel, rsbecker, santiago, shengfa 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-02 22:32 ` Shengfa Lin @ 2020-10-03 4:57 ` Junio C Hamano 0 siblings, 0 replies; 47+ messages in thread From: Junio C Hamano @ 2020-10-03 4:57 UTC (permalink / raw) To: Shengfa Lin; +Cc: jrnieder, git, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:21 ` [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config Shengfa Lin 2020-09-30 23:41 ` Junio C Hamano @ 2020-09-30 23:55 ` Junio C Hamano 2020-10-02 6:51 ` Shengfa Lin 2020-10-01 0:05 ` Junio C Hamano 2020-10-01 2:44 ` Jonathan Nieder 3 siblings, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-09-30 23:55 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:55 ` Junio C Hamano @ 2020-10-02 6:51 ` Shengfa Lin 0 siblings, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 6:51 UTC (permalink / raw) To: gitster; +Cc: git, nathaniel, rsbecker, santiago, shengfa 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:21 ` [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config Shengfa Lin 2020-09-30 23:41 ` Junio C Hamano 2020-09-30 23:55 ` Junio C Hamano @ 2020-10-01 0:05 ` Junio C Hamano 2020-10-01 2:44 ` Jonathan Nieder 3 siblings, 0 replies; 47+ messages in thread From: Junio C Hamano @ 2020-10-01 0:05 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-09-30 23:21 ` [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config Shengfa Lin ` (2 preceding siblings ...) 2020-10-01 0:05 ` Junio C Hamano @ 2020-10-01 2:44 ` Jonathan Nieder 2020-10-02 21:17 ` Shengfa Lin 3 siblings, 1 reply; 47+ messages in thread From: Jonathan Nieder @ 2020-10-01 2:44 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago, gitster 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 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config 2020-10-01 2:44 ` Jonathan Nieder @ 2020-10-02 21:17 ` Shengfa Lin 0 siblings, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 21:17 UTC (permalink / raw) To: jrnieder; +Cc: git, gitster, nathaniel, rsbecker, santiago, shengfa 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-09-30 23:21 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Shengfa Lin 2020-09-30 23:21 ` [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config Shengfa Lin @ 2020-09-30 23:53 ` Junio C Hamano 2020-10-01 2:17 ` Junio C Hamano 2020-10-02 21:23 ` Shengfa Lin 1 sibling, 2 replies; 47+ messages in thread From: Junio C Hamano @ 2020-09-30 23:53 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago 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(). ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-09-30 23:53 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Junio C Hamano @ 2020-10-01 2:17 ` Junio C Hamano 2020-10-01 3:43 ` Jonathan Nieder 2020-10-02 21:42 ` Shengfa Lin 2020-10-02 21:23 ` Shengfa Lin 1 sibling, 2 replies; 47+ messages in thread From: Junio C Hamano @ 2020-10-01 2:17 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-01 2:17 ` Junio C Hamano @ 2020-10-01 3:43 ` Jonathan Nieder 2020-10-01 15:48 ` Junio C Hamano ` (2 more replies) 2020-10-02 21:42 ` Shengfa Lin 1 sibling, 3 replies; 47+ messages in thread From: Jonathan Nieder @ 2020-10-01 3:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: Shengfa Lin, git, nathaniel, rsbecker, santiago 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-01 3:43 ` Jonathan Nieder @ 2020-10-01 15:48 ` Junio C Hamano 2020-10-08 19:49 ` Junio C Hamano 2020-10-02 21:56 ` Shengfa Lin 2020-10-03 19:53 ` brian m. carlson 2 siblings, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-10-01 15:48 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Shengfa Lin, git, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-01 15:48 ` Junio C Hamano @ 2020-10-08 19:49 ` Junio C Hamano [not found] ` <CAEOYnASgxCE5NjhoSgDwyQyAmdLhw5UyFq_Fu==8q7y6uXGz6w@mail.gmail.com> 0 siblings, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-10-08 19:49 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Shengfa Lin, git, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
[parent not found: <CAEOYnASgxCE5NjhoSgDwyQyAmdLhw5UyFq_Fu==8q7y6uXGz6w@mail.gmail.com>]
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone [not found] ` <CAEOYnASgxCE5NjhoSgDwyQyAmdLhw5UyFq_Fu==8q7y6uXGz6w@mail.gmail.com> @ 2020-10-09 16:48 ` Junio C Hamano 0 siblings, 0 replies; 47+ messages in thread From: Junio C Hamano @ 2020-10-09 16:48 UTC (permalink / raw) To: Nathaniel Manista; +Cc: git, Jonathan Nieder, Shengfa Lin, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-01 3:43 ` Jonathan Nieder 2020-10-01 15:48 ` Junio C Hamano @ 2020-10-02 21:56 ` Shengfa Lin 2020-10-02 22:06 ` Junio C Hamano 2020-10-03 19:53 ` brian m. carlson 2 siblings, 1 reply; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 21:56 UTC (permalink / raw) To: jrnieder; +Cc: git, gitster, nathaniel, rsbecker, santiago, shengfa 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? ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-02 21:56 ` Shengfa Lin @ 2020-10-02 22:06 ` Junio C Hamano 2020-10-03 3:50 ` Shengfa Lin 0 siblings, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-10-02 22:06 UTC (permalink / raw) To: Shengfa Lin; +Cc: jrnieder, git, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-02 22:06 ` Junio C Hamano @ 2020-10-03 3:50 ` Shengfa Lin 2020-10-03 4:42 ` Junio C Hamano 0 siblings, 1 reply; 47+ messages in thread From: Shengfa Lin @ 2020-10-03 3:50 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, nathaniel, rsbecker, santiago, shengfa 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-03 3:50 ` Shengfa Lin @ 2020-10-03 4:42 ` Junio C Hamano 0 siblings, 0 replies; 47+ messages in thread From: Junio C Hamano @ 2020-10-03 4:42 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, jrnieder, nathaniel, rsbecker, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-01 3:43 ` Jonathan Nieder 2020-10-01 15:48 ` Junio C Hamano 2020-10-02 21:56 ` Shengfa Lin @ 2020-10-03 19:53 ` brian m. carlson 2020-10-03 22:14 ` Junio C Hamano 2 siblings, 1 reply; 47+ messages in thread From: brian m. carlson @ 2020-10-03 19:53 UTC (permalink / raw) To: Jonathan Nieder Cc: Junio C Hamano, Shengfa Lin, git, nathaniel, rsbecker, santiago [-- 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 --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-03 19:53 ` brian m. carlson @ 2020-10-03 22:14 ` Junio C Hamano 0 siblings, 0 replies; 47+ messages in thread From: Junio C Hamano @ 2020-10-03 22:14 UTC (permalink / raw) To: brian m. carlson Cc: Jonathan Nieder, Shengfa Lin, git, nathaniel, rsbecker, santiago "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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-10-01 2:17 ` Junio C Hamano 2020-10-01 3:43 ` Jonathan Nieder @ 2020-10-02 21:42 ` Shengfa Lin 1 sibling, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 21:42 UTC (permalink / raw) To: gitster; +Cc: git, nathaniel, rsbecker, santiago, shengfa 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone 2020-09-30 23:53 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Junio C Hamano 2020-10-01 2:17 ` Junio C Hamano @ 2020-10-02 21:23 ` Shengfa Lin 1 sibling, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-02 21:23 UTC (permalink / raw) To: gitster; +Cc: git, nathaniel, rsbecker, santiago, shengfa 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [WIP v2 0/2] experiment with commit option record-time-zone 2019-12-05 17:14 [ISSUE] Stop accessing, storing, and sharing the user's time zone Nathaniel Manista ` (2 preceding siblings ...) 2020-09-30 23:21 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Shengfa Lin @ 2020-10-13 5:28 ` Shengfa Lin 2020-10-13 5:28 ` [WIP v2 1/2] Adding a record-time-zone command option for commit Shengfa Lin 2020-10-13 5:28 ` [WIP v2 2/2] Demonstrate failing and passing tests Shengfa Lin 3 siblings, 2 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-13 5:28 UTC (permalink / raw) To: git; +Cc: sandals, gitster, jrnieder, nathaniel, rsbecker, santiago, shengfa 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [WIP v2 1/2] Adding a record-time-zone command option for commit 2020-10-13 5:28 ` [WIP v2 0/2] experiment with commit option record-time-zone Shengfa Lin @ 2020-10-13 5:28 ` Shengfa Lin 2020-10-13 20:03 ` Junio C Hamano 2020-10-13 5:28 ` [WIP v2 2/2] Demonstrate failing and passing tests Shengfa Lin 1 sibling, 1 reply; 47+ messages in thread From: Shengfa Lin @ 2020-10-13 5:28 UTC (permalink / raw) To: git; +Cc: sandals, gitster, jrnieder, nathaniel, rsbecker, santiago, shengfa 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, ×tamp, &offset)) + int zero_offset_negative_sign; + if (parse_date_basic(date, ×tamp, &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, ×tamp, &offset)) + if (!parse_date_basic(date, ×tamp, &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, ×tamp, &offset)) { + if (!parse_date_basic(date, ×tamp, &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 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [WIP v2 1/2] Adding a record-time-zone command option for commit 2020-10-13 5:28 ` [WIP v2 1/2] Adding a record-time-zone command option for commit Shengfa Lin @ 2020-10-13 20:03 ` Junio C Hamano 2020-10-21 5:01 ` Shengfa Lin 0 siblings, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-10-13 20:03 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, sandals, jrnieder, nathaniel, rsbecker, santiago 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, ×tamp, &offset)) > + if (!parse_date_basic(date, ×tamp, &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, ×tamp, &offset)) { > + if (!parse_date_basic(date, ×tamp, &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); > } > + ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [WIP v2 1/2] Adding a record-time-zone command option for commit 2020-10-13 20:03 ` Junio C Hamano @ 2020-10-21 5:01 ` Shengfa Lin 2020-10-21 18:55 ` Junio C Hamano 0 siblings, 1 reply; 47+ messages in thread From: Shengfa Lin @ 2020-10-21 5:01 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, nathaniel, rsbecker, sandals, santiago, shengfa 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). ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [WIP v2 1/2] Adding a record-time-zone command option for commit 2020-10-21 5:01 ` Shengfa Lin @ 2020-10-21 18:55 ` Junio C Hamano 2020-10-22 16:27 ` Junio C Hamano 0 siblings, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-10-21 18:55 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, jrnieder, nathaniel, rsbecker, sandals, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [WIP v2 1/2] Adding a record-time-zone command option for commit 2020-10-21 18:55 ` Junio C Hamano @ 2020-10-22 16:27 ` Junio C Hamano 2020-10-26 4:14 ` Shengfa Lin 0 siblings, 1 reply; 47+ messages in thread From: Junio C Hamano @ 2020-10-22 16:27 UTC (permalink / raw) To: Shengfa Lin; +Cc: git, jrnieder, nathaniel, rsbecker, sandals, santiago 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [WIP v2 1/2] Adding a record-time-zone command option for commit 2020-10-22 16:27 ` Junio C Hamano @ 2020-10-26 4:14 ` Shengfa Lin 0 siblings, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-26 4:14 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, nathaniel, rsbecker, sandals, santiago, shengfa >> 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [WIP v2 2/2] Demonstrate failing and passing tests 2020-10-13 5:28 ` [WIP v2 0/2] experiment with commit option record-time-zone Shengfa Lin 2020-10-13 5:28 ` [WIP v2 1/2] Adding a record-time-zone command option for commit Shengfa Lin @ 2020-10-13 5:28 ` Shengfa Lin 1 sibling, 0 replies; 47+ messages in thread From: Shengfa Lin @ 2020-10-13 5:28 UTC (permalink / raw) To: git; +Cc: sandals, gitster, jrnieder, nathaniel, rsbecker, santiago, shengfa 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 ^ permalink raw reply related [flat|nested] 47+ messages in thread
end of thread, other threads:[~2020-10-26 4:14 UTC | newest] Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-05 17:14 [ISSUE] Stop accessing, storing, and sharing the user's time zone Nathaniel Manista 2019-12-05 17:31 ` Junio C Hamano 2019-12-05 17:33 ` Randall S. Becker 2019-12-05 17:43 ` Junio C Hamano 2019-12-05 17:53 ` Santiago Torres Arias 2019-12-05 18:00 ` Randall S. Becker 2020-09-30 23:21 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Shengfa Lin 2020-09-30 23:21 ` [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config Shengfa Lin 2020-09-30 23:41 ` Junio C Hamano 2020-10-01 0:17 ` Junio C Hamano 2020-10-02 6:07 ` Shengfa Lin 2020-10-01 0:31 ` Junio C Hamano 2020-10-01 0:35 ` Junio C Hamano 2020-10-02 6:41 ` Shengfa Lin 2020-10-02 6:46 ` Shengfa Lin 2020-10-02 6:37 ` Shengfa Lin 2020-10-02 6:02 ` Shengfa Lin 2020-10-02 6:15 ` Jonathan Nieder 2020-10-02 22:32 ` Shengfa Lin 2020-10-03 4:57 ` Junio C Hamano 2020-09-30 23:55 ` Junio C Hamano 2020-10-02 6:51 ` Shengfa Lin 2020-10-01 0:05 ` Junio C Hamano 2020-10-01 2:44 ` Jonathan Nieder 2020-10-02 21:17 ` Shengfa Lin 2020-09-30 23:53 ` [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone Junio C Hamano 2020-10-01 2:17 ` Junio C Hamano 2020-10-01 3:43 ` Jonathan Nieder 2020-10-01 15:48 ` Junio C Hamano 2020-10-08 19:49 ` Junio C Hamano [not found] ` <CAEOYnASgxCE5NjhoSgDwyQyAmdLhw5UyFq_Fu==8q7y6uXGz6w@mail.gmail.com> 2020-10-09 16:48 ` Junio C Hamano 2020-10-02 21:56 ` Shengfa Lin 2020-10-02 22:06 ` Junio C Hamano 2020-10-03 3:50 ` Shengfa Lin 2020-10-03 4:42 ` Junio C Hamano 2020-10-03 19:53 ` brian m. carlson 2020-10-03 22:14 ` Junio C Hamano 2020-10-02 21:42 ` Shengfa Lin 2020-10-02 21:23 ` Shengfa Lin 2020-10-13 5:28 ` [WIP v2 0/2] experiment with commit option record-time-zone Shengfa Lin 2020-10-13 5:28 ` [WIP v2 1/2] Adding a record-time-zone command option for commit Shengfa Lin 2020-10-13 20:03 ` Junio C Hamano 2020-10-21 5:01 ` Shengfa Lin 2020-10-21 18:55 ` Junio C Hamano 2020-10-22 16:27 ` Junio C Hamano 2020-10-26 4:14 ` Shengfa Lin 2020-10-13 5:28 ` [WIP v2 2/2] Demonstrate failing and passing tests Shengfa Lin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).