{"thread":{"id":"52389","subject":"[ISSUE] Stop accessing, storing, and sharing the user's time zone","startedAt":"2019-12-05T17:14:36Z","lastAt":"2020-10-26T04:14:49Z","messageCount":47,"participants":["Nathaniel Manista","Junio C Hamano","Randall S. Becker","Santiago Torres Arias","Shengfa Lin","Jonathan Nieder","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"387566","messageId":"CAEOYnAQYMrNAe9s1V-0DVLdL-B_KpHMDP5e=yRnbCkMWdrvFHQ@mail.gmail.com","threadId":"52389","inReplyTo":null,"subject":"[ISSUE] Stop accessing, storing, and sharing the user's time zone","fromName":"Nathaniel Manista","fromEmail":"nathaniel@google.com","sentAt":"2019-12-05T17:14:04Z","receivedAt":"2019-12-05T17:14:36Z","isPatch":false,"sender":{"key":"nathaniel@google.com","avatar":null},"body":"(This is also filed at https://bugs.chromium.org/p/git/issues/detail?id=43.)\n\nAffected Version: All? This has been bothering me at least a year.\n\nWhat steps will reproduce the problem?\n1. Author a commit.\n2. \"git log --pretty=fuller\"\n\nWhat is the expected output?\nThe log will display that the timestamps of the commit have both the\nauthor time and committer time in UTC. Internally no part of the\ncommit will have stored any time zone information and when the commit\nis shared with others no information about where the user was in the\nworld at the time of the commit will be obtainable from it.\n\nWhat do you see instead?\nAuthoring and sharing a commit by default exposes the user's time zone.\n\nAdditional context:\n\"commit --date=YYYY-MM-DDThh:mm:ss+0000\" suffices to put the author\ntime in UTC but not the commit time in UTC. But the user shouldn't\nhave to pass a flag at all.\n\nWhere the user is in the world is PII that git ought not to record and\nmake available as part of the user's software engineering (make\navailable to colleagues, in the case of proprietary development, and\nmake available to the world, in the case of open source). Git should\nentirely stop accessing, recording, and sharing the user's time zone,\nfull stop. Failing that, git should by default stop accessing,\nrecording, and sharing the user's time zone, but if individual users\nwant to have their time zones on their commits, they can opt into it.\nFailing that, users should be able to add a .gitconfig line to ensure\nthat all author timestamps, all committer timestamps, and any other\ninformation are in UTC.\n\n(Thanks much,\n-Nathaniel)\n"},{"id":"387568","messageId":"xmqq1rti1xww.fsf@gitster-ct.c.googlers.com","threadId":"52389","inReplyTo":"CAEOYnAQYMrNAe9s1V-0DVLdL-B_KpHMDP5e=yRnbCkMWdrvFHQ@mail.gmail.com","subject":"Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-05T17:31:59Z","receivedAt":"2019-12-05T17:32:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nathaniel Manista writes:\n\n> Authoring and sharing a commit by default exposes the user's time zone.\n\ns/exposes/stores/ and that is a feature. You are free to run \n\n $ TZ=GMT git commit\n\nif you wanted to opt out of the feature, but this has been the\ndefault since day one and people expect Git to behave this way.\n\n"},{"id":"387569","messageId":"047b01d5ab92$159a8240$40cf86c0$@nexbridge.com","threadId":"52389","inReplyTo":"CAEOYnAQYMrNAe9s1V-0DVLdL-B_KpHMDP5e=yRnbCkMWdrvFHQ@mail.gmail.com","subject":"RE: [ISSUE] Stop accessing, storing, and sharing the user's time zone","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-12-05T17:33:13Z","receivedAt":"2019-12-05T17:33:24Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 5, 2019 12:14 PM, Nathaniel Manista wrote:\n> (This is also filed at https://bugs.chromium.org/p/git/issues/detail?id=43.)\n\nGetting permission denied trying to access this.\n\n> Affected Version: All? This has been bothering me at least a year.\n> \n> What steps will reproduce the problem?\n> 1. Author a commit.\n> 2. \"git log --pretty=fuller\"\n> \n> What is the expected output?\n> The log will display that the timestamps of the commit have both the\n> author time and committer time in UTC. Internally no part of the\n> commit will have stored any time zone information and when the commit\n> is shared with others no information about where the user was in the\n> world at the time of the commit will be obtainable from it.\n> \n> What do you see instead?\n> Authoring and sharing a commit by default exposes the user's time zone.\n> \n> Additional context:\n> \"commit --date=YYYY-MM-DDThh:mm:ss+0000\" suffices to put the author\n> time in UTC but not the commit time in UTC. But the user shouldn't\n> have to pass a flag at all.\n> \n> Where the user is in the world is PII that git ought not to record and\n> make available as part of the user's software engineering (make\n> available to colleagues, in the case of proprietary development, and\n> make available to the world, in the case of open source). Git should\n> entirely stop accessing, recording, and sharing the user's time zone,\n> full stop. Failing that, git should by default stop accessing,\n> recording, and sharing the user's time zone, but if individual users\n> want to have their time zones on their commits, they can opt into it.\n> Failing that, users should be able to add a .gitconfig line to ensure\n> that all author timestamps, all committer timestamps, and any other\n> information are in UTC.\n\nMy 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.\n\nRegards,\nRandall\n\n"},{"id":"387570","messageId":"xmqqwobazmzp.fsf@gitster-ct.c.googlers.com","threadId":"52389","inReplyTo":"047b01d5ab92$159a8240$40cf86c0$@nexbridge.com","subject":"Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-05T17:43:54Z","receivedAt":"2019-12-05T17:43:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" writes:\n\n> .... I currently am\n> running a repository located at UTC+2, with developers at\n> UTC-5. It is driving us a bit wonky. I would rather see only UTC.\n\nIf \"seeing\" is the primary reason (i.e. you want to compare times\nyour people worked on their commits), you can always do that on the\ndisplay side (e.g. \"git log --date=local\"). It is not a good excuse\nto advocate for information loss.\n\nI also feel that TZ being PII is not particularly a brilliant\nargument against recording TZ---of course it is PII, so are the\ncommitter e-mail address and the committer name. Those who want to\nhide can hide but in order to keep track of provenance who did what\nwhen, we do record them.\n\nAs you can guess from the above reasoning, I am not fundamentally\nopposed to introducing user.tz to complement existing user.name and\nuser.email configuration variables.\n"},{"id":"387571","messageId":"048901d5ab95$dd6d43e0$9847cba0$@nexbridge.com","threadId":"52389","inReplyTo":"xmqqwobazmzp.fsf@gitster-ct.c.googlers.com","subject":"RE: [ISSUE] Stop accessing, storing, and sharing the user's time zone","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-12-05T18:00:17Z","receivedAt":"2019-12-05T18:00:31Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 5, 2019 12:44 PM, Junio C Hamano wrote:\n> \"Randall S. Becker\" writes:\n> > .... I currently am\n> > running a repository located at UTC+2, with developers at UTC-5. It is\n> > driving us a bit wonky. I would rather see only UTC.\n> \n> If \"seeing\" is the primary reason (i.e. you want to compare times your\npeople\n> worked on their commits), you can always do that on the display side (e.g.\n> \"git log --date=local\"). It is not a good excuse to advocate for\ninformation\n> loss.\n> \n> I also feel that TZ being PII is not particularly a brilliant argument\nagainst\n> recording TZ---of course it is PII, so are the committer e-mail address\nand the\n> committer name. Those who want to hide can hide but in order to keep\n> track of provenance who did what when, we do record them.\n> \n> As you can guess from the above reasoning, I am not fundamentally opposed\n> to introducing user.tz to complement existing user.name and user.email\n> configuration variables.\n\nI rather like this idea. I'm going to look into it when I'm in the\nupside-down toward the end of the month.\n\nCheers,\nRandall\n\n"},{"id":"387575","messageId":"20191205175326.fxgfofr4xvh7kaxb@LykOS.localdomain","threadId":"52389","inReplyTo":"xmqqwobazmzp.fsf@gitster-ct.c.googlers.com","subject":"Re: [ISSUE] Stop accessing, storing, and sharing the user's time zone","fromName":"Santiago Torres Arias","fromEmail":"santiago@nyu.edu","sentAt":"2019-12-05T17:53:27Z","receivedAt":"2019-12-05T19:05:31Z","isPatch":false,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Thu, Dec 05, 2019 at 09:43:54AM -0800, Junio C Hamano wrote:\n> \"Randall S. Becker\" writes:\n> \n> > .... I currently am\n> > running a repository located at UTC+2, with developers at\n> > UTC-5. It is driving us a bit wonky. I would rather see only UTC.\n>\n> I also feel that TZ being PII is not particularly a brilliant\n> argument against recording TZ---of course it is PII, so are the\n> committer e-mail address and the committer name. Those who want to\n> hide can hide but in order to keep track of provenance who did what\n> when, we do record them.\n\nThis is exactly what I had found confusing from the original post.\n> \n> As you can guess from the above reasoning, I am not fundamentally\n> opposed to introducing user.tz to complement existing user.name and\n> user.email configuration variables.\n\nThis sounds like a small delta that would probably please everyone. For\nnow, using the --date argument on git commit allows you to also pass a\ntimezone:\n\n git commit --date=\"$(TZ=PST date)\"\n\nWhich you could easily alias...\n\nThanks,\n-Santiago\n"},{"id":"406732","messageId":"20200930232138.3656304-1-shengfa@google.com","threadId":"52389","inReplyTo":"CAEOYnAQYMrNAe9s1V-0DVLdL-B_KpHMDP5e=yRnbCkMWdrvFHQ@mail.gmail.com","subject":"[RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-09-30T23:21:37Z","receivedAt":"2020-09-30T23:23:42Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Discussed with Jonathan and Junio regarding whether we should support\nletting user specify timezone or restricted to UTC only. There was no\ndefnite conclusion. I think it's a good idea to start with this\nimplementation which might result in the change landing or e.g. a\ndocumentation change.\n\nThe patch add checking for user.hideTimezone and change environment\nvariable TZ to UTC in the beginning of cmd_commit if it's true. As a\nresult, when calculating timezone offset in datestamp(date.c) would result in 0. \n\nShengfa Lin (1):\n hideTimezone: add a user.hideTimezone config\n\n Documentation/config/user.txt | 4 ++++\n builtin/commit.c | 5 +++++\n t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++\n 3 files changed, 46 insertions(+)\n create mode 100755 t/t7527-commit-hide-timezone.sh\n\n-- \n2.28.0.709.gb0816b6eb0-goog\n\n"},{"id":"406733","messageId":"20200930232138.3656304-2-shengfa@google.com","threadId":"52389","inReplyTo":"20200930232138.3656304-1-shengfa@google.com","subject":"[RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-09-30T23:21:38Z","receivedAt":"2020-09-30T23:23:57Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Users requested hiding location in the world from source control\ntrail. This is an implementation to read user.hideTimezone in\ncmd_commit and set timezone to UTC if it's true.\n\nAdded a brief explanation of the new field in Documentation\nand added tests for true/false and reset-author\n\nSigned-off-by: Shengfa Lin \n---\n Documentation/config/user.txt | 4 ++++\n builtin/commit.c | 5 +++++\n t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++\n 3 files changed, 46 insertions(+)\n create mode 100755 t/t7527-commit-hide-timezone.sh\n\ndiff --git a/Documentation/config/user.txt b/Documentation/config/user.txt\nindex 59aec7c3ae..bd6813f527 100644\n--- a/Documentation/config/user.txt\n+++ b/Documentation/config/user.txt\n@@ -36,3 +36,7 @@ user.signingKey::\n \tcommit, you can override the default selection with this variable.\n \tThis option is passed unchanged to gpg's --local-user parameter,\n \tso you may specify a key using any method that gpg supports.\n+\n+user.hideTimezone::\n+ Override TZ to UTC for Git commits to hide user's timezone in commit\n+ date\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 42b964e0ca..fb1cbb8a39 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n \ts.colopts = 0;\n \n+ git_config(git_default_config, NULL);\n+ int hide_timezone = 0;\n+ if (!git_config_get_bool(\"user.hideTimezone\", &hide_timezone) && hide_timezone)\n+ setenv(\"TZ\", \"UTC\", 1);\n+\n \tif (get_oid(\"HEAD\", &oid))\n \t\tcurrent_head = NULL;\n \telse {\ndiff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh\nnew file mode 100755\nindex 0000000000..41ed9c27da\n--- /dev/null\n+++ b/t/t7527-commit-hide-timezone.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='git-commit can override local timezone setting by reading user.hideTimezone from config'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' '\n+ git config user.hideTimezone false &&\n+ echo test1 >> file &&\n+ git add file &&\n+ # unset GIT_AUTHOR_DATE from test_tick\n+ unset GIT_AUTHOR_DATE &&\n+ TZ=Europe/Istanbul git commit -m initial &&\n+ git log -1 > output &&\n+ grep \"Date: .* +0300\" output\n+'\n+\n+test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' '\n+ git config user.hideTimezone true &&\n+ git commit --amend --reset-author &&\n+ git log -1 > output &&\n+ grep \"Date: .* +0000\" output\n+'\n+\n+test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' '\n+ git config user.hideTimezone true &&\n+ echo test2 >> file &&\n+ git add file &&\n+ # TZ setting corresponding to -0600 or -0500 depending on DST\n+ # unset GIT_AUTHOR_DATE from test_tick\n+ unset GIT_AUTHOR_DATE &&\n+ TZ=America/Chicago git commit -m test2 &&\n+ git log -1 > output &&\n+ grep \"Date: .* +0000\" output\n+'\n+\n+test_done\n-- \n2.28.0.709.gb0816b6eb0-goog\n\n"},{"id":"406734","messageId":"xmqqpn62essa.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20200930232138.3656304-2-shengfa@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-30T23:41:57Z","receivedAt":"2020-09-30T23:42:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\n> Users requested hiding location in the world from source control\n> trail. This is an implementation to read user.hideTimezone in\n> cmd_commit and set timezone to UTC if it's true.\n>\n> Added a brief explanation of the new field in Documentation\n> and added tests for true/false and reset-author\n>\n> Signed-off-by: Shengfa Lin \n> ---\n> Documentation/config/user.txt | 4 ++++\n> builtin/commit.c | 5 +++++\n\nThere are many ways other than running \"git commit\" for a commit to\nbe created, including but not limited to \"git merge\", \"git rebase\",\n\"git pull\" (with or without \"--rebase\").\n\n> +user.hideTimezone::\n> + Override TZ to UTC for Git commits to hide user's timezone in commit\n> + date\n\nOne level of indentation in this codebase is a single HT.\n\nUnterminated sentence.\n\nA configuration _without_ command line option that overrides it is\nhighly frowned upon. I do not see a reason why this must be such a\nconfiguration. If anything, a feature like this should first start\nas a command line option, and only after it proves its usefulness,\na new configuration for convenience should be added.\n\nThis only affects \"git commit\" and no other command (which I think\nis a mistake), yet is placed in the \"user.*\" namespace? That does\nnot make any sense. I can sort-of understand if it were called say\n\"commit.useUTC\" though.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 42b964e0ca..fb1cbb8a39 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n> \ts.colopts = 0;\n> \n> + git_config(git_default_config, NULL);\n\nDeclaration after statement is not tolerated in this codebase.\n\nOne level of indentation in this codebase is a single HT.\n\n> + int hide_timezone = 0;\n\nUnnecessary initialization.\n\n> + if (!git_config_get_bool(\"user.hideTimezone\", &hide_timezone) && hide_timezone)\n\nOverlong line. Double-SP before &&\n\n> + setenv(\"TZ\", \"UTC\", 1);\n> +\n> \tif (get_oid(\"HEAD\", &oid))\n> \t\tcurrent_head = NULL;\n> \telse {\n> diff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh\n> new file mode 100755\n> index 0000000000..41ed9c27da\n\nLet's not waste a test number for just a single test or two. Can't\nwe roll this into \n\n> --- /dev/null\n> +++ b/t/t7527-commit-hide-timezone.sh\n> @@ -0,0 +1,37 @@\n> +#!/bin/sh\n> +\n> +test_description='git-commit can override local timezone setting by reading user.hideTimezone from config'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' '\n> + git config user.hideTimezone false &&\n> + echo test1 >> file &&\n\nStyle. Documentation/CodingGuidelines\n\n - Redirection operators should be written with space before, but no\n space after them. In other words, write 'echo test >\"$file\"'\n instead of 'echo test> $file' or 'echo test > $file'. ...\n\n> + git add file &&\n> + # unset GIT_AUTHOR_DATE from test_tick\n> + unset GIT_AUTHOR_DATE &&\n> + TZ=Europe/Istanbul git commit -m initial &&\n> + git log -1 > output &&\n> + grep \"Date: .* +0300\" output\n\nDo they not have DST over there, and is it guaranteed that they will\nnever have one? Would we see this test fail about half of the year,\nwhen timezone rules change over there in some future year? After\nall, they changed in 2016 last time, which is fairly recent.\n\nThis test attempts to establish the baseline, but I do not think it\nis a good idea. I think it is better *not* to unset GIT_AUTHOR_DATE\nlike this. Instead, make sure it is set to some timestamp in some\ntimezone that is not UTC, and the timezone of the resulting commit\nauthor date is in that timezone. But that must have already been\ndone in basic tests on \"git commit\" that we honor the environment\nvariable, no? Which means there is no need to add yet another extra\nbaseline test here.\n\n> +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' '\n> + git config user.hideTimezone true &&\n> + git commit --amend --reset-author &&\n> + git log -1 > output &&\n> + grep \"Date: .* +0000\" output\n\nThis one IS interesting, but keep the GIT_AUTHOR_DATE set and\nexported. As long as that is from a timezone different from UTC, we\nare testing what we want to test here.\n\n> +'\n> +\n> +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' '\n> + git config user.hideTimezone true &&\n> + echo test2 >> file &&\n> + git add file &&\n> + # TZ setting corresponding to -0600 or -0500 depending on DST\n> + # unset GIT_AUTHOR_DATE from test_tick\n> + unset GIT_AUTHOR_DATE &&\n> + TZ=America/Chicago git commit -m test2 &&\n\nThis one is a borderline meh, compared to a test with explicit\nGIT_AUTHOR_DATE getting overridden by the configuration. It is not\nall that wrong, but I do not see a point in adding cycles to the\nalready big testsuite.\n\n> + git log -1 > output &&\n> + grep \"Date: .* +0000\" output\n> +'\n> +\n> +test_done\n\nThanks.\n"},{"id":"406735","messageId":"xmqqlfgqes95.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20200930232138.3656304-1-shengfa@google.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-30T23:53:26Z","receivedAt":"2020-09-30T23:53:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\n> Discussed with Jonathan and Junio regarding whether we should support\n> letting user specify timezone or restricted to UTC only. There was no\n> defnite conclusion.\n\nI do not think was involved in that part of the discussion to\nconsider UTC vs custom zone, though.\n\nWhat I said is that git () { TZ=UTC command git \"$@\" } is simple\nenough that I doubt it is worth the engineering effort to either\nread configuration file early (which is tricky) so that setenv() can\nbe done early in cmd_main() and/or audit the codebase widely enough\n(which is time consuming) to make sure that we pay attention to the\nconfiguration variable in all codepaths that matter to do the\nsetenv().\n\n"},{"id":"406736","messageId":"xmqqh7rees6h.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20200930232138.3656304-2-shengfa@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-30T23:55:02Z","receivedAt":"2020-09-30T23:55:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\n> Users requested hiding location in the world from source control\n> trail. This is an implementation to read user.hideTimezone in\n> cmd_commit and set timezone to UTC if it's true.\n\nNot a very good proposed log message, that sounds as if \"it is\nwhat 'Users' requested, so it must be a worthwhile thing to do\",\nwhich is not the line of thinking to go by.\n\n\n\n\n>\n> Added a brief explanation of the new field in Documentation\n> and added tests for true/false and reset-author\n>\n> Signed-off-by: Shengfa Lin \n> ---\n> Documentation/config/user.txt | 4 ++++\n> builtin/commit.c | 5 +++++\n> t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++\n> 3 files changed, 46 insertions(+)\n> create mode 100755 t/t7527-commit-hide-timezone.sh\n>\n> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt\n> index 59aec7c3ae..bd6813f527 100644\n> --- a/Documentation/config/user.txt\n> +++ b/Documentation/config/user.txt\n> @@ -36,3 +36,7 @@ user.signingKey::\n> \tcommit, you can override the default selection with this variable.\n> \tThis option is passed unchanged to gpg's --local-user parameter,\n> \tso you may specify a key using any method that gpg supports.\n> +\n> +user.hideTimezone::\n> + Override TZ to UTC for Git commits to hide user's timezone in commit\n> + date\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 42b964e0ca..fb1cbb8a39 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n> \ts.colopts = 0;\n> \n> + git_config(git_default_config, NULL);\n> + int hide_timezone = 0;\n> + if (!git_config_get_bool(\"user.hideTimezone\", &hide_timezone) && hide_timezone)\n> + setenv(\"TZ\", \"UTC\", 1);\n> +\n> \tif (get_oid(\"HEAD\", &oid))\n> \t\tcurrent_head = NULL;\n> \telse {\n> diff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh\n> new file mode 100755\n> index 0000000000..41ed9c27da\n> --- /dev/null\n> +++ b/t/t7527-commit-hide-timezone.sh\n> @@ -0,0 +1,37 @@\n> +#!/bin/sh\n> +\n> +test_description='git-commit can override local timezone setting by reading user.hideTimezone from config'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' '\n> + git config user.hideTimezone false &&\n> + echo test1 >> file &&\n> + git add file &&\n> + # unset GIT_AUTHOR_DATE from test_tick\n> + unset GIT_AUTHOR_DATE &&\n> + TZ=Europe/Istanbul git commit -m initial &&\n> + git log -1 > output &&\n> + grep \"Date: .* +0300\" output\n> +'\n> +\n> +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' '\n> + git config user.hideTimezone true &&\n> + git commit --amend --reset-author &&\n> + git log -1 > output &&\n> + grep \"Date: .* +0000\" output\n> +'\n> +\n> +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' '\n> + git config user.hideTimezone true &&\n> + echo test2 >> file &&\n> + git add file &&\n> + # TZ setting corresponding to -0600 or -0500 depending on DST\n> + # unset GIT_AUTHOR_DATE from test_tick\n> + unset GIT_AUTHOR_DATE &&\n> + TZ=America/Chicago git commit -m test2 &&\n> + git log -1 > output &&\n> + grep \"Date: .* +0000\" output\n> +'\n> +\n> +test_done\n"},{"id":"406737","messageId":"xmqqft6yerog.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20200930232138.3656304-2-shengfa@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T00:05:51Z","receivedAt":"2020-10-01T00:05:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\nI won't repeat what I said in the other review message, but since I\nforgot to comment on the log message...\n\n> Users requested hiding location in the world from source control\n> trail. This is an implementation to read user.hideTimezone in\n> cmd_commit and set timezone to UTC if it's true.\n>\n> Added a brief explanation of the new field in Documentation\n> and added tests for true/false and reset-author\n\nNot a very good proposed log message, that sounds as if \"it is\nwhat 'Users' requested, so it must be a worthwhile thing to do\",\nwhich is not the line of thinking to go by.\n\nThe convention we follow in the commit log messages is to:\n\n - first explain that the current system does not do X (in present\n tense, so we do NOT say \"previously we did not do X\"), then\n\n - explain why doing X would be a good thing, and finally\n\n - give an order to the codebase to start doing X.\n\n\nPerhaps\n\n Many places in Git record the timezone of the actor when a\n timestamp is recorded, including the committer and author\n timestamps in a commit object and the tagger timestamp in a tag\n object. Some people however prefer to \"lie\" about where they\n actually are.\n\n They _could_ just say \"export TZ=UTC\" and be done with it, but\n the method would not easily allow them to pretend to be in the\n UTC timezone only with Git, while revealing their true timezone\n to other activities (e.g. sending e-mail?).\n\n Introduce user.hideTimeZone configuration variable, which can be\n optionally set to 'true' to pretend to Git as if the user has\n exported environment variable TZ with the value UTC.\n\nThanks.\n\n"},{"id":"406738","messageId":"xmqqblhmer5b.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"xmqqpn62essa.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T00:17:20Z","receivedAt":"2020-10-01T00:17:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> new file mode 100755\n>> index 0000000000..41ed9c27da\n>\n> Let's not waste a test number for just a single test or two. Can't\n> we roll this into \n\n... an existing test for \"git commit\"?\n"},{"id":"406739","messageId":"xmqq7dsaeqij.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"xmqqpn62essa.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T00:31:00Z","receivedAt":"2020-10-01T00:31:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' '\n>> + git config user.hideTimezone true &&\n>> + git commit --amend --reset-author &&\n>> + git log -1 > output &&\n>> + grep \"Date: .* +0000\" output\n>\n> This one IS interesting, but keep the GIT_AUTHOR_DATE set and\n> exported. As long as that is from a timezone different from UTC, we\n> are testing what we want to test here.\n\nNote.\n\nOnce GIT_AUTHOR_DATE and friends are set fully including timezone,\nwe won't even read TZ because there is no need to. But you can do\nsomething along these lines:\n\n\ttest_config user.hideTimeZone true &&\n\t(\n\t\texport GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&\n\t\tgit commit ... &&\n\t\tgit show -s --format='%aI' >output &&\n\t\techo 2020-09-13T15:26:40+03:00 >expect &&\n\t\t...\n\nI think (haven't actually tested) \"git commit --date=\" option\nis handled the same way, i.e. comparing these two would be a way not\nto touch the environment variable.\n\n TZ=UTC-09 git commit --date=@1600000000 ... &&\n TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 ... &&\n git show -s --format='%aI' HEAD~1 >output0 &&\n git show -s --format='%aI' HEAD~0 >output1\n\n\n\n \n\n\n\n\n\n\n\n"},{"id":"406740","messageId":"xmqq362yeqbo.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"xmqq7dsaeqij.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T00:35:07Z","receivedAt":"2020-10-01T00:35:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \ttest_config user.hideTimeZone true &&\n> \t(\n> \t\texport GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&\n> \t\tgit commit ... &&\n> \t\tgit show -s --format='%aI' >output &&\n> \t\techo 2020-09-13T15:26:40+03:00 >expect &&\n\nOops. The sample date and zone must be adjusted for the values\nexported above. I expect that we'd need to do\n\n\t\t...\n\t\techo 2020-09-13T12:26:40+00:00 >expect &&\n\t\ttest_cmp expect output\n\n"},{"id":"406741","messageId":"xmqqtuved70y.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"xmqqlfgqes95.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T02:17:17Z","receivedAt":"2020-10-01T02:19:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> What I said is that git () { TZ=UTC command git \"$@\" } is simple\n> enough that I doubt it is worth the engineering effort to either\n> read configuration file early (which is tricky) so that setenv() can\n> be done early in cmd_main() and/or audit the codebase widely enough\n> (which is time consuming) to make sure that we pay attention to the\n> configuration variable in all codepaths that matter to do the\n> setenv().\n\nRegarding the implementation, the posted patch to \"git commit\" uses\nthe \"futz with TZ early inside the git process\" approach, but I\nthink we should not do so for another reason. Our setenv() may not\nbe early enough---before the code that decides to call a setenv()\nis run, there are many things that are outside your control (like\nthe tracing subsystem, repository discovery, etc.) will run, and if\nany of them does something that triggers tzset() to be called, it\nwill be done with the value of TZ the process started with, and our\nsetenv() that happens much later won't have any effect to it.\n\nAnother thing is that setting TZ ourselves would affect hooks and\nother programs we spawn as subprocess, which may or may not be\ndesired, depending on the reason why the user is forcing UTC.\n\nAll code that create object headers like committer, author and\ntagger lines use the same git_author_info() and git_committer_info()\nAPI, I think, which eventually goes to fmt_ident(), and they produce\na pair . This gives us an\nentirely different opening to tackle this issue from, because the\n\"number of seconds since epoch\" part won't change the value no\nmatter what timezone you are in.\n\nYou can let the existing code produce its natural result and then\nwhen the \"force UTC\" flag is set, override the offset part to +0000\nif and only if the timezone was obtained from the current\nenvironment (this if-and-only-if is necessary, because you do not\nwant to rewrite and force UTC when you run \"git commit --amend\"\nwithout the \"--reset-author\" option to update a commit that was\ncreated somewhere else to UTC). That way, we do not have to futz\nwith TZ environment or tzset.\n\nJust something to think about.\n"},{"id":"406742","messageId":"20201001024452.GA2930867@google.com","threadId":"52389","inReplyTo":"20200930232138.3656304-2-shengfa@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-01T02:44:52Z","receivedAt":"2020-10-01T02:44:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nShengfa Lin wrote:\n\n> Documentation/config/user.txt | 4 ++++\n> builtin/commit.c | 5 +++++\n> t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++\n> 3 files changed, 46 insertions(+)\n> create mode 100755 t/t7527-commit-hide-timezone.sh\n\nThanks for this discussion starter. I'm interested to see what we end\nup with.\n\nTo summarize the thread so far:\n\nNathaniel Manista, who we can credit with a `Reported-by` line,\nreports that (mildly paraphrased):\n\n\tAuthoring and sharing a commit by default exposes the user's\n\ttime zone.\n\n\t\"gt commit --date=YYYY-MM-DDThh:mm:ss+0000\" suffices to put\n\tthe author time in UTC (though not the commit time in UTC).\n\tBut the user shouldn't have to pass a flag at all.\n\n\tWhere the user is in the world is private information that git\n\tought not to record and make available as part of the user's\n\tsoftware engineering (make available to colleagues, in the\n\tcase of proprietary development, and make available to the\n\tworld, in the case of open source). Git should entirely stop\n\taccessing, recording, and sharing the user's time zone.\n\nOn the other hand, various others have mentioned some beneficial\naspects of recording the timezone --- for example, it makes it easier\nto make sense of the chronology in a program's development. What\nseems uncontroversial is that users should have control over the time\nzone used (as they already do, via the TZ environment variable).\n\nIn response to the suggestion of a \"[user] timeZone\" setting,\nNathaniel suggests\n\n\tThat sounds like a great first step and like something that\n\twouldn't ruffle anyone's feathers while proving the value of\n\tignoring time zone information. 🙂\n\nThere is more to discuss in this design space --- let's see where the\npatch leads us.\n\n> --- a/Documentation/config/user.txt\n> +++ b/Documentation/config/user.txt\n> @@ -36,3 +36,7 @@ user.signingKey::\n> \tcommit, you can override the default selection with this variable.\n> \tThis option is passed unchanged to gpg's --local-user parameter,\n> \tso you may specify a key using any method that gpg supports.\n> +\n> +user.hideTimezone::\n> + Override TZ to UTC for Git commits to hide user's timezone in commit\n> + date\n\nTo me, this seems less appealing than being able to set the time zone.\nBy setting the time zone to match others in the same project, I would\nbe able to blend in without sharing information about my travels. If\nI use the hideTimezone setting instead, then I may stick out as the\nonly contributor using UTC.\n\nUTC is the default timezone in various development environments so\nthis is not a big deal, but it seems enough reason to prefer the\nfuller-control approach.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 42b964e0ca..fb1cbb8a39 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n> \ts.colopts = 0;\n> \n> + git_config(git_default_config, NULL);\n> + int hide_timezone = 0;\n> + if (!git_config_get_bool(\"user.hideTimezone\", &hide_timezone) && hide_timezone)\n> + setenv(\"TZ\", \"UTC\", 1);\n\nLike Junio mentioned, this affects \"git commit\" but not other commands\nthat record the current date with the local timezone.\n\nThe fundamental tool to exercise that machinery is\n\n\t$ git var GIT_AUTHOR_IDENT\n\tJonathan Nieder 1601517809 -0700\n\nso I suppose I'd be interested in seeing that exercised in tests.\n\nLooking at the implementation, I find fmt_ident in ident.c, which\ncalls ident_default_date(), which calls date.c's datestamp, which uses\nlocaltime_r to get the timezone. localtime_r on all platforms I know\nof calls tzset, though apparently[*] it is not required to.\n\nThe unfortunate thing about these APIs is that there's no way to pass\nin a timezone from a string instead of from the environment. This\nmeans that passing through the environment as above is the only\nreasonable way to do it, but that would have the unfortunate result\nof changing the output of commands like \"git log --date=local\" that\nare about writing dates to the terminal instead of storing them.\n\nSo I'd be tempted to do something targetted like this:\n\n-- 8< --\ndiff --git i/date.c w/date.c\nindex f9ea807b3a9..658ba1a9a45 100644\n--- i/date.c\n+++ w/date.c\n@@ -5,6 +5,7 @@\n */\n \n #include \"cache.h\"\n+#include \"config.h\"\n \n /*\n * This is like mktime, but without normalization of tm_wday and tm_yday.\n@@ -998,6 +999,20 @@ void datestamp(struct strbuf *out)\n \ttime_t now;\n \tint offset;\n \tstruct tm tm = { 0 };\n+\tconst char *env_tz;\n+\tchar *config_tz = NULL;\n+\tint restore_tz = 0;\n+\n+\tif (!git_config_get_string(\"user.timezone\", &config_tz)) {\n+\t\tenv_tz = getenv(\"TZ\");\n+\t\tif (env_tz)\n+\t\t\tenv_tz = xstrdup(env_tz);\n+\t\tif (!env_tz || strcmp(env_tz, config_tz)) {\n+\t\t\trestore_tz = 1;\n+\t\t\tsetenv(\"TZ\", config_tz, 1);\n+\t\t\ttzset();\n+\t\t}\n+\t}\n \n \ttime(&now);\n \n@@ -1005,6 +1020,14 @@ void datestamp(struct strbuf *out)\n \toffset /= 60;\n \n \tdate_string(now, offset, out);\n+\n+\tif (restore_tz) {\n+\t\tif (env_tz)\n+\t\t\tsetenv(\"TZ\", env_tz, 1);\n+\t\telse\n+\t\t\tunsetenv(\"TZ\");\n+\t}\n+\tfree(config_tz);\n }\n \n /*\n-- >8 --\n\nLooking over callers, who would this affect? There are three callers:\n\n fast-import.c::parse_ident:\n\tUsed to handle ident string \"now\". That seems in keeping with\n\tthe intent here, and fast-import does respect some other\n\tconfiguration though only affecting storage. Seems fine.sensible.\n\n ident.c::ident_default_date:\n\tUsed to produce author and committer timestamps and timestamps for\n\treflog entries. That's the goal; good.\n\n send-pack.c::generate_push_cert:\n\tUsed for the timestamp sent to the server in a signed push\n\tcertificate. Also good.\n\nSo I think this does the right thing, plus it retains the\nuser-friendly feature of being able to *display* timestamps in their\nlocal timezone.\n\nNow let's talk through the downsides:\n\nIt's complex. The performance isn't likely to be bad when\nuser.timezone is not set, which is nice, but it still is messier than\nI'd like to see.\n\nIt's specific to Git, but a user might want to disable recording their\ntimezone in *all* collaboration tools (mail clients, newsreaders,\netc), not just Git. So I would find it more compelling if there were\na common convention shared across tools for setting your exposed\ntimezone, just like the EMAIL envvar for setting your exposed email\naddress.\n\nThoughts?\n\nThanks,\nJonathan\n\n[*] https://pubs.opengroup.org/onlinepubs/9699919799/functions/localtime.html\n"},{"id":"406743","messageId":"20201001034350.GB2930867@google.com","threadId":"52389","inReplyTo":"xmqqtuved70y.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-01T03:43:50Z","receivedAt":"2020-10-01T03:43:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Our setenv() may not\n> be early enough---before the code that decides to call a setenv()\n> is run, there are many things that are outside your control (like\n> the tracing subsystem, repository discovery, etc.) will run, and if\n> any of them does something that triggers tzset() to be called, it\n> will be done with the value of TZ the process started with, and our\n> setenv() that happens much later won't have any effect to it.\n\nI thought about this before, but in fact it's okay: when calling a\nfunction like localtime() (though not localetime_r() --- see my other\nreply), tzset() is called each time so it is able to reflect any\nupdates to the TZ envvar from the interim.\n\n[....]\n> You can let the existing code produce its natural result and then\n> when the \"force UTC\" flag is set, override the offset part to +0000\n> if and only if the timezone was obtained from the current\n> environment (this if-and-only-if is necessary, because you do not\n> want to rewrite and force UTC when you run \"git commit --amend\"\n> without the \"--reset-author\" option to update a commit that was\n> created somewhere else to UTC). That way, we do not have to futz\n> with TZ environment or tzset.\n\nYes, I think this is simpler and nicer than the proposal in my other\nreply.\n\nIn addition to not having to futz with TZ, I think I like the\nsemantics better. The motivation that started this thread was not so\nmuch \"I want to set a custom timezone to blend in\" but rather \"why are\nwe recording the timezone at all here?\" In that context, it makes\nsense to me to have a setting such as\n\n\tcore.recordTimeZone\n\nthat I can turn *off* to say that I don't think datestamp() callers\nshould consider the timezone to be information worth recording (and\ninstead they should write +0000). To me that seems a little simpler\nto understand than user.hideTimezone since this focuses on turning\nsome functionality off (recording of the time zone) instead of turning\non a new stealth mode.\n\nThanks,\nJonathan\n"},{"id":"406764","messageId":"xmqqh7rec5h3.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20201001034350.GB2930867@google.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T15:48:24Z","receivedAt":"2020-10-01T15:48:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> In addition to not having to futz with TZ, I think I like the\n> semantics better. The motivation that started this thread was not so\n> much \"I want to set a custom timezone to blend in\" but rather \"why are\n> we recording the timezone at all here?\" In that context, it makes\n> sense to me to have a setting such as\n>\n> \tcore.recordTimeZone\n>\n> that I can turn *off* to say that I don't think datestamp() callers\n> should consider the timezone to be information worth recording (and\n> instead they should write +0000). To me that seems a little simpler\n> to understand than user.hideTimezone since this focuses on turning\n> some functionality off (recording of the time zone) instead of turning\n> on a new stealth mode.\n\nHmph. It is a valid way to look at the issue, I guess.\n\nThanks for an input.\n"},{"id":"406802","messageId":"20201002060200.4073817-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqqpn62essa.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T06:02:00Z","receivedAt":"2020-10-02T06:02:06Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Thanks for the comments.\n\nJunio C Hamano writes:\n\n>> Users requested hiding location in the world from source control\n>> trail. This is an implementation to read user.hideTimezone in\n>> cmd_commit and set timezone to UTC if it's true.\n>>\n>> Added a brief explanation of the new field in Documentation\n>> and added tests for true/false and reset-author\n>>\n>> Signed-off-by: Shengfa Lin \n>> ---\n>> Documentation/config/user.txt | 4 ++++\n>> builtin/commit.c | 5 +++++\n>\n> There are many ways other than running \"git commit\" for a commit to\n> be created, including but not limited to \"git merge\", \"git rebase\",\n> \"git pull\" (with or without \"--rebase\").\n\nI overlooked on this.\n\n>> +user.hideTimezone::\n>> + Override TZ to UTC for Git commits to hide user's timezone in commit\n>> + date\n>\n> One level of indentation in this codebase is a single HT.\n>\n> Unterminated sentence.\n\nWhat does HT stands for? I will change the indentation to 8 spaces.\n\n> This only affects \"git commit\" and no other command (which I think\n> is a mistake), yet is placed in the \"user.*\" namespace? That does\n> not make any sense. I can sort-of understand if it were called say\n> \"commit.useUTC\" though.\n\nI don't think this is a recommendation to implement commit.useUTC instead\nsince it makes more sense to support more than just the commit command.\n\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 42b964e0ca..fb1cbb8a39 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>> \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n>> \ts.colopts = 0;\n>> \n>> + git_config(git_default_config, NULL);\n>\n>Declaration after statement is not tolerated in this codebase.\n\nIf I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler\ncatches this as an error?\n\n>> + int hide_timezone = 0;\n>\n> Unnecessary initialization.\n>> + if (!git_config_get_bool(\"user.hideTimezone\", &hide_timezone) && hide_timezone)\n>\n\nIs it unnecessary because I am checking the return value from git_config_get_bool so\nthat the uninitialized value won't be used?\n\n>> + git add file &&\n>> + # unset GIT_AUTHOR_DATE from test_tick\n>> + unset GIT_AUTHOR_DATE &&\n>> + TZ=Europe/Istanbul git commit -m initial &&\n>> + git log -1 > output &&\n>> + grep \"Date: .* +0300\" output\n>\n> Do they not have DST over there, and is it guaranteed that they will\n> never have one? Would we see this test fail about half of the year,\n> when timezone rules change over there in some future year? After\n> all, they changed in 2016 last time, which is fairly recent.\n\nI looked up the timezones and find one without DST without considering\nthe possibility of timezone rules change which would cause the test\nto fail.\n\n> This test attempts to establish the baseline, but I do not think it\n> is a good idea. I think it is better *not* to unset GIT_AUTHOR_DATE\n> like this. Instead, make sure it is set to some timestamp in some\n> timezone that is not UTC, and the timezone of the resulting commit\n> author date is in that timezone. But that must have already been\n> done in basic tests on \"git commit\" that we honor the environment\n> variable, no? Which means there is no need to add yet another extra\n> baseline test here.\n>\n\nI am not sure if this test has already been done in commit basic tests.\nWill remove this test.\n\n>> +'\n>> +\n>> +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' '\n>> + git config user.hideTimezone true &&\n>> + echo test2 >> file &&\n>> + git add file &&\n>> + # TZ setting corresponding to -0600 or -0500 depending on DST\n>> + # unset GIT_AUTHOR_DATE from test_tick\n>> + unset GIT_AUTHOR_DATE &&\n>> + TZ=America/Chicago git commit -m test2 &&\n>\n> This one is a borderline meh, compared to a test with explicit\n> GIT_AUTHOR_DATE getting overridden by the configuration. It is not\n> all that wrong, but I do not see a point in adding cycles to the\n> already big testsuite.\n>\n\nWill remove this one as well.\n"},{"id":"406803","messageId":"20201002060709.4076630-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqqblhmer5b.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T06:07:09Z","receivedAt":"2020-10-02T06:07:12Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Junio C Hamano writes:\n\n>>> new file mode 100755\n>>> index 0000000000..41ed9c27da\n>>\n>> Let's not waste a test number for just a single test or two. Can't\n>> we roll this into \n>\n> ... an existing test for \"git commit\"?\n\nWill move into an existing test for \"git commit\".\n\n"},{"id":"406804","messageId":"20201002061550.GF3252492@google.com","threadId":"52389","inReplyTo":"20201002060200.4073817-1-shengfa@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-02T06:15:50Z","receivedAt":"2020-10-02T06:15:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nShengfa Lin wrote:\n> Thanks for the comments.\n\n>>> +user.hideTimezone::\n>>> + Override TZ to UTC for Git commits to hide user's timezone in commit\n>>> + date\n>>\n>> One level of indentation in this codebase is a single HT.\n>>\n>> Unterminated sentence.\n>\n> What does HT stands for? I will change the indentation to 8 spaces.\n\nHT means \"horizontal tab\", like might be shown with \"man ascii\".\n\nGit uses tabs for indentation. This file is documentation instead of\nsource so clang-format doesn't know about it, but I might as well\nmention anyway: if you run \"make style\", then clang-format will give\nsome suggestions around formatting. The configuration for that is not\nyet perfect so you can take its suggestions with a grain of salt, but\nthey should get you in the right direction.\n\n[...]\n>>> --- a/builtin/commit.c\n>>> +++ b/builtin/commit.c\n>>> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>> \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n>>> \ts.colopts = 0;\n>>> \n>>> + git_config(git_default_config, NULL);\n>>\n>> Declaration after statement is not tolerated in this codebase.\n>\n> If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler\n> catches this as an error?\n\nYes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement.\n\n>>> + int hide_timezone = 0;\n>>\n>> Unnecessary initialization.\n>>\n>>> + if (!git_config_get_bool(\"user.hideTimezone\", &hide_timezone) && hide_timezone)\n>\n> Is it unnecessary because I am checking the return value from git_config_get_bool so\n> that the uninitialized value won't be used?\n\nBy leaving it uninitialized, you can help avoid the reader wondering\nwhether there is some code path where the default value is used.\n\n[...]\n>> Instead, make sure it is set to some timestamp in some\n>> timezone that is not UTC, and the timezone of the resulting commit\n>> author date is in that timezone. But that must have already been\n>> done in basic tests on \"git commit\" that we honor the environment\n>> variable, no? Which means there is no need to add yet another extra\n>> baseline test here.\n>\n> I am not sure if this test has already been done in commit basic tests.\n> Will remove this test.\n\nLet's see: *checks with \"git grep -e TZ -- t\"*.\n\nLooks like t0006 tests various aspects of TZ handling pretty well and\nt1100 includes of test using TZ with commit-tree (good).\n\nThanks and hope that helps,\nJonathan\n"},{"id":"406805","messageId":"20201002063716.4087336-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqq7dsaeqij.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T06:37:16Z","receivedAt":"2020-10-02T06:37:19Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Junio C Hamano writes:\n\n> \n> I think (haven't actually tested) \"git commit --date=\" option\n> is handled the same way, i.e. comparing these two would be a way not\n> to touch the environment variable.\n> \n> TZ=UTC-09 git commit --date=@1600000000 ... &&\n> TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 ... &&\n> git show -s --format='%aI' HEAD~1 >output0 &&\n> git show -s --format='%aI' HEAD~0 >output1\n\nLike this?\n\ntest_expect_fail '...' '\n echo t1 >file &&\n git add file &&\n TZ=UTC-09 git commit --date=@1600000000 -m \"t1\" &&\n echo t2 >>file &&\n git add file &&\n TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 -m \"t2\" &&\n git show -s --format='%aI' HEAD~1 >output0 &&\n git show -s --format='%aI' HEAD~0 >output1 &&\n test_cmp output0 output1\n'\n\nI tested it.\n"},{"id":"406806","messageId":"20201002064105.4089915-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqq362yeqbo.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T06:41:05Z","receivedAt":"2020-10-02T06:41:09Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Junio C Hamano writes:\n\n>> \ttest_config user.hideTimeZone true &&\n>> \t(\n>> \t\texport GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&\n>> \t\tgit commit ... &&\n>> \t\tgit show -s --format='%aI' >output &&\n>> \t\techo 2020-09-13T15:26:40+03:00 >expect &&\n>\n> Oops. The sample date and zone must be adjusted for the values\n> exported above. I expect that we'd need to do\n> \n> \t\t...\n> \t\techo 2020-09-13T12:26:40+00:00 >expect &&\n> \t\ttest_cmp expect output\n\nTested the following:\n\ntest_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true' '\n test_config user.hideTimezone true &&\n (\n echo test2 >file &&\n git add file &&\n export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&\n git commit -m \"test2\" &&\n git show -s --format='%aI' >output &&\n echo 2020-09-13T12:26:40+00:00 >expect &&\n test_cmp output expect\n )\n'\n\n"},{"id":"406807","messageId":"20201002064603.4092711-1-shengfa@google.com","threadId":"52389","inReplyTo":"20201002064105.4089915-1-shengfa@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T06:46:03Z","receivedAt":"2020-10-02T06:46:07Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Shengfa Lin writes:\n\n>>> \ttest_config user.hideTimeZone true &&\n>>> \t(\n>>> \t\texport GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&\n>>> \t\tgit commit ... &&\n>>> \t\tgit show -s --format='%aI' >output &&\n>>> \t\techo 2020-09-13T15:26:40+03:00 >expect &&\n>>\n>> Oops. The sample date and zone must be adjusted for the values\n>> exported above. I expect that we'd need to do\n>> \n>> \t\t...\n>> \t\techo 2020-09-13T12:26:40+00:00 >expect &&\n>> \t\ttest_cmp expect output\n>\n> Tested the following:\n> \n> test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true' '\n> test_config user.hideTimezone true &&\n> (\n> echo test2 >file &&\n> git add file &&\n> export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&\n> git commit -m \"test2\" &&\n> git show -s --format='%aI' >output &&\n> echo 2020-09-13T12:26:40+00:00 >expect &&\n> test_cmp output expect\n> )\n> '\n\nI assume the opening and closing parentheses means the environment variables are only effective\nwithin like an environment scope. Please correct if wrong.\n"},{"id":"406808","messageId":"20201002065135.4096175-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqqh7rees6h.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T06:51:35Z","receivedAt":"2020-10-02T06:51:39Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Junio C Hamano writes:\n\n>> Users requested hiding location in the world from source control\n>> trail. This is an implementation to read user.hideTimezone in\n>> cmd_commit and set timezone to UTC if it's true.\n>\n> Not a very good proposed log message, that sounds as if \"it is\n> what 'Users' requested, so it must be a worthwhile thing to do\",\n> which is not the line of thinking to go by.\n\nThat's a good point. Users requested does not necessarily equivalent\nto worthwhile thing to do. Thanks for pointing it out.\n\n\n\n\n"},{"id":"406843","messageId":"20201002211719.68606-1-shengfa@google.com","threadId":"52389","inReplyTo":"20201001024452.GA2930867@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T21:17:19Z","receivedAt":"2020-10-02T21:17:22Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Jonathan Nieder writes:\n\n> Like Junio mentioned, this affects \"git commit\" but not other commands\n> that record the current date with the local timezone.\n> \n> The fundamental tool to exercise that machinery is\n> \n> \t$ git var GIT_AUTHOR_IDENT\n> \tJonathan Nieder 1601517809 -0700\n> \n> so I suppose I'd be interested in seeing that exercised in tests.\n\nNext patch should include a test for git var.\n\n> [...]\n> \n> The unfortunate thing about these APIs is that there's no way to pass\n> in a timezone from a string instead of from the environment. This\n> means that passing through the environment as above is the only\n> reasonable way to do it, but that would have the unfortunate result\n> of changing the output of commands like \"git log --date=local\" that\n> are about writing dates to the terminal instead of storing them.\n> \n\nThe TZ should apply for write but not read.\n\n> [...]\n> Looking over callers, who would this affect? There are three callers:\n> \n> fast-import.c::parse_ident:\n> \tUsed to handle ident string \"now\". That seems in keeping with\n> \tthe intent here, and fast-import does respect some other\n> \tconfiguration though only affecting storage. Seems fine.sensible.\n> \n> ident.c::ident_default_date:\n> \tUsed to produce author and committer timestamps and timestamps for\n> \treflog entries. That's the goal; good.\n> \n> send-pack.c::generate_push_cert:\n> \tUsed for the timestamp sent to the server in a signed push\n> \tcertificate. Also good.\n> \n\nThanks for the analysis here. I was wondering whether the TZ should\naffect these callers.\n\n> So I think this does the right thing, plus it retains the\n> user-friendly feature of being able to *display* timestamps in their\n> local timezone.\n> \n> Now let's talk through the downsides:\n> \n> It's complex. The performance isn't likely to be bad when\n> user.timezone is not set, which is nice, but it still is messier than\n> I'd like to see.\n> \n\n+1.\n"},{"id":"406844","messageId":"20201002212319.71516-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqqlfgqes95.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T21:23:19Z","receivedAt":"2020-10-02T21:23:22Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Junio C Hamano writes:\n\nShengfa Lin writes:\n\n>> Discussed with Jonathan and Junio regarding whether we should support\n>> letting user specify timezone or restricted to UTC only. There was no\n>> defnite conclusion.\n>\n> I do not think was involved in that part of the discussion to\n> consider UTC vs custom zone, though.\n> \n> What I said is that git () { TZ=UTC command git \"$@\" } is simple\n> enough that I doubt it is worth the engineering effort to either\n> read configuration file early (which is tricky) so that setenv() can\n> be done early in cmd_main() and/or audit the codebase widely enough\n> (which is time consuming) to make sure that we pay attention to the\n> configuration variable in all codepaths that matter to do the\n> setenv().\n\nThanks for clarifying.\n"},{"id":"406847","messageId":"20201002214251.76114-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqqtuved70y.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T21:42:51Z","receivedAt":"2020-10-02T21:42:55Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Junio C Hamano writes:\n\n>> [...]\n\n> Regarding the implementation, the posted patch to \"git commit\" uses\n> the \"futz with TZ early inside the git process\" approach, but I\n> think we should not do so for another reason. Our setenv() may not\n> be early enough---before the code that decides to call a setenv()\n> is run, there are many things that are outside your control (like\n> the tracing subsystem, repository discovery, etc.) will run, and if\n> any of them does something that triggers tzset() to be called, it\n> will be done with the value of TZ the process started with, and our\n> setenv() that happens much later won't have any effect to it.\n> \n\nSetenv for TZ early in the git process even at the beginning of\ncmd_main does not set TZ for things run earlier. In this sense,\nuser setting TZ themselves would be consistent. On the other\nhand, we may not require \"things\" run earlier to have consistent\nTZ depending on whether user time would be exposed.\n\n> Another thing is that setting TZ ourselves would affect hooks and\n> other programs we spawn as subprocess, which may or may not be\n> desired, depending on the reason why the user is forcing UTC.\n> \n\nShould not setenv for TZ bluntly because of risk of undesired side\neffect\n\n> All code that create object headers like committer, author and\n> tagger lines use the same git_author_info() and git_committer_info()\n> API, I think, which eventually goes to fmt_ident(), and they produce\n> a pair . This gives us an\n> entirely different opening to tackle this issue from, because the\n> \"number of seconds since epoch\" part won't change the value no\n> matter what timezone you are in.\n> \n\nA good candidate for handling in a centralized and controllable\nmanner.\n\n> You can let the existing code produce its natural result and then\n> when the \"force UTC\" flag is set, override the offset part to +0000\n> if and only if the timezone was obtained from the current\n> environment (this if-and-only-if is necessary, because you do not\n\nI don't understand \"if the timezone was obtained from the current\nenvironment. How do we tell? getenv(\"TZ\")?\n\n> want to rewrite and force UTC when you run \"git commit --amend\"\n> without the \"--reset-author\" option to update a commit that was\n> created somewhere else to UTC). That way, we do not have to futz\n> with TZ environment or tzset.\n> \n> Just something to think about.\n\nDoes this mean we need to make sure \"git commit --amend\"\nwithout the \"--reset-author\" will not trigger regeneration of\ntimestamp through datestamp in \"date.c\" but with the\n\"--reset-author\", it will?\n\nPreferable not to futz with TZ environment.\n"},{"id":"406848","messageId":"20201002215656.80643-1-shengfa@google.com","threadId":"52389","inReplyTo":"20201001034350.GB2930867@google.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T21:56:56Z","receivedAt":"2020-10-02T21:57:00Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Jonathan Nieder writes:\n\n>> Our setenv() may not\n>> be early enough---before the code that decides to call a setenv()\n>> is run, there are many things that are outside your control (like\n>> the tracing subsystem, repository discovery, etc.) will run, and if\n>> any of them does something that triggers tzset() to be called, it\n>> will be done with the value of TZ the process started with, and our\n>> setenv() that happens much later won't have any effect to it.\n>\n> I thought about this before, but in fact it's okay: when calling a\n> function like localtime() (though not localetime_r() --- see my other\n> reply), tzset() is called each time so it is able to reflect any\n> updates to the TZ envvar from the interim.\n> \n\nMy understanding is that it's concerning that setenv may not be early\nenough that not all things have the same TZ; thus, it's still possible\nto expose the user timezone.\n\n> [....]\n> \n> In addition to not having to futz with TZ, I think I like the\n> semantics better. The motivation that started this thread was not so\n> much \"I want to set a custom timezone to blend in\" but rather \"why are\n> we recording the timezone at all here?\" In that context, it makes\n> sense to me to have a setting such as\n> \n> \tcore.recordTimeZone\n> \n> that I can turn *off* to say that I don't think datestamp() callers\n> should consider the timezone to be information worth recording (and\n> instead they should write +0000). To me that seems a little simpler\n> to understand than user.hideTimezone since this focuses on turning\n> some functionality off (recording of the time zone) instead of turning\n> on a new stealth mode.\n> \n> Thanks,\n> Jonathan\n\n+1, simpler to understand.\nUsing user.hideTimezone because user is\ntrying to hide timezone sounds a little off; directly instructing the\nsystem is more straight forward.\nIf we have a setting of \"core.recordTimeZone\", do we need to make it\nas a command option as Junio suggested earlier?\n"},{"id":"406849","messageId":"xmqq8sco8era.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20201002215656.80643-1-shengfa@google.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-02T22:06:01Z","receivedAt":"2020-10-02T22:06:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\n>> In addition to not having to futz with TZ, I think I like the\n>> semantics better. The motivation that started this thread was not so\n>> much \"I want to set a custom timezone to blend in\" but rather \"why are\n>> we recording the timezone at all here?\" In that context, it makes\n>> sense to me to have a setting such as\n>> \n>> \tcore.recordTimeZone\n>> \n>> that I can turn *off* to say that I don't think datestamp() callers\n>> should consider the timezone to be information worth recording (and\n>> instead they should write +0000). To me that seems a little simpler\n>> to understand than user.hideTimezone since this focuses on turning\n>> some functionality off (recording of the time zone) instead of turning\n>> on a new stealth mode.\n>> \n>> Thanks,\n>> Jonathan\n>\n> +1, simpler to understand.\n\nYup.\n\n> If we have a setting of \"core.recordTimeZone\", do we need to make it\n> as a command option as Junio suggested earlier?\n\nUsually we add command line option --[no-]record-time-zone first\nwithout configuration option when introducing a new features like\nthis one. Once the feature proves useful, we'd add a matching\nconfiguration variable for convenience, but leave the command line\noption (negative form in this case) so that a configured per-user\ndefault can be overridden as needed.\n"},{"id":"406850","messageId":"20201002223212.93266-1-shengfa@google.com","threadId":"52389","inReplyTo":"20201002061550.GF3252492@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-02T22:32:12Z","receivedAt":"2020-10-02T22:32:15Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Jonathan Nieder writes:\n\n>>> [...]\n>>\n>> What does HT stands for? I will change the indentation to 8 spaces.\n>\n> HT means \"horizontal tab\", like might be shown with \"man ascii\".\n> \n> Git uses tabs for indentation. This file is documentation instead of\n> source so clang-format doesn't know about it, but I might as well\n> mention anyway: if you run \"make style\", then clang-format will give\n> some suggestions around formatting. The configuration for that is not\n> yet perfect so you can take its suggestions with a grain of salt, but\n> they should get you in the right direction.\n>\n\nGot it, thanks!\n\n>[...]\n>>>> --- a/builtin/commit.c\n>>>> +++ b/builtin/commit.c\n>>>> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>>> \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n>>>> \ts.colopts = 0;\n>>>> \n>>>> + git_config(git_default_config, NULL);\n>>>\n>>> Declaration after statement is not tolerated in this codebase.\n>>\n>> If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler\n>> catches this as an error?\n>\n> Yes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement.\n>\n\nGot the error from int hide_timezone = 0; but not\ngit_config(git_default_config, NULL);.\n\n>>>> + int hide_timezone = 0;\n>>>\n>>> Unnecessary initialization.\n>>>\n>>>> + if (!git_config_get_bool(\"user.hideTimezone\", &hide_timezone) && hide_timezone)\n>>\n>> Is it unnecessary because I am checking the return value from git_config_get_bool so\n>> that the uninitialized value won't be used?\n>\n> By leaving it uninitialized, you can help avoid the reader wondering\n> whether there is some code path where the default value is used.\n>\n\nI see.\n\n>[...]\n>>> Instead, make sure it is set to some timestamp in some\n>>> timezone that is not UTC, and the timezone of the resulting commit\n>>> author date is in that timezone. But that must have already been\n>>> done in basic tests on \"git commit\" that we honor the environment\n>>> variable, no? Which means there is no need to add yet another extra\n>>> baseline test here.\n>>\n>> I am not sure if this test has already been done in commit basic tests.\n>> Will remove this test.\n>\n> Let's see: *checks with \"git grep -e TZ -- t\"*.\n> \n> Looks like t0006 tests various aspects of TZ handling pretty well and\n> t1100 includes of test using TZ with commit-tree (good).\n> \n> Thanks and hope that helps,\n> Jonathan\n\nThanks! That helps a lot.\n\n\n"},{"id":"406854","messageId":"20201003035050.151766-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqq8sco8era.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-03T03:50:50Z","receivedAt":"2020-10-03T03:50:55Z","isPatch":true,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Junio C Hamano writes:\n\n>> If we have a setting of \"core.recordTimeZone\", do we need to make it\n>> as a command option as Junio suggested earlier?\n>\n> Usually we add command line option --[no-]record-time-zone first\n> without configuration option when introducing a new features like\n> this one. Once the feature proves useful, we'd add a matching\n> configuration variable for convenience, but leave the command line\n> option (negative form in this case) so that a configured per-user\n> default can be overridden as needed.\n\nAny recommendation as to where to add this command line option?\nShould the option be added separately in \"git merge\", \"git rebase\",\n\"git commit\", \"git pull\" and other places as OPT_BOOL?\nOr could it be added directly to handle_options in git.c?\nIf added to git.c, it will only be added it once.\nThen an environment variable(such as GIT_RECORD_TIME_ZONE) can be added\nto save the option and datestamp in date.c can set offset accordingly.\n\n"},{"id":"406855","messageId":"xmqq4knb9az0.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20201003035050.151766-1-shengfa@google.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-03T04:42:27Z","receivedAt":"2020-10-03T04:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\n> Junio C Hamano writes:\n>\n>>> If we have a setting of \"core.recordTimeZone\", do we need to make it\n>>> as a command option as Junio suggested earlier?\n>>\n>> Usually we add command line option --[no-]record-time-zone first\n>> without configuration option when introducing a new features like\n>> this one. Once the feature proves useful, we'd add a matching\n>> configuration variable for convenience, but leave the command line\n>> option (negative form in this case) so that a configured per-user\n>> default can be overridden as needed.\n>\n> Any recommendation as to where to add this command line option?\n> Should the option be added separately in \"git merge\", \"git rebase\",\n> \"git commit\", \"git pull\" and other places as OPT_BOOL?\n\nI think that would be most end-user friendly.\n\n> Or could it be added directly to handle_options in git.c?\n> If added to git.c, it will only be added it once.\n\nThat would be easier for lazy developers, but not as end-user\nfriendly as giving it to individual commands, I would think.\n\n> Then an environment variable(such as GIT_RECORD_TIME_ZONE) can be added\n> to save the option and datestamp in date.c can set offset accordingly.\n\nWe prefer not to use new environment variable unless there is a\nclear advantage. Consider GIT_DIR etc. that were added early in\nGit's history as historical mistakes.\n"},{"id":"406856","messageId":"xmqqzh537vp5.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20201002223212.93266-1-shengfa@google.com","subject":"Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-03T04:57:42Z","receivedAt":"2020-10-03T04:57:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\n>>>>> + git_config(git_default_config, NULL);\n>>>>\n>>>> Declaration after statement is not tolerated in this codebase.\n>>>\n>>> If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler\n>>> catches this as an error?\n>>\n>> Yes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement.\n>\n> Got the error from int hide_timezone = 0; but not\n> git_config(git_default_config, NULL);.\n> ...\n>>>>> + int hide_timezone = 0;\n\nThat is exactly expected.\n\nThe problematic sequence was\n\n\t...\n\ts.colopts = 0;\n\n\tgit_config(git_default_config, NULL);\n\tint hide_timezone = 0;\n\nThe assignment of 0 to s.colopts, which existed before your patch,\nwas already a statement. So is a new call to git_config() function\nyou added. As the existing \"s.colopts = 0\" were in a place where a\nstatement can legally appear, the place you added git_config() call\nis also where a statement can legally appear. There is nothing for\nthe compiler to complain about.\n\nThe declaration of hide_timezone added _after_ that statement is a\ndifferent story. Because the -Wdeclaration-after-statement is about\nstarting a function (or \"a block\" in general) with all the necessary\ndeclarations before we write a single statement, and also forbidding\nus from adding other declarations after we write a statement. Since\nthere are bunch of statements in the same block already, including\nthe assignment to s.colopts and a call to git_config(), we cannot\nadd a declaration after that.\n\nThanks.\n\n"},{"id":"406873","messageId":"20201003195325.GL1392312@camp.crustytoothpaste.net","threadId":"52389","inReplyTo":"20201001034350.GB2930867@google.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-10-03T19:53:25Z","receivedAt":"2020-10-03T19:54:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-10-01 at 03:43:50, Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n> > You can let the existing code produce its natural result and then\n> > when the \"force UTC\" flag is set, override the offset part to +0000\n> > if and only if the timezone was obtained from the current\n> > environment (this if-and-only-if is necessary, because you do not\n> > want to rewrite and force UTC when you run \"git commit --amend\"\n> > without the \"--reset-author\" option to update a commit that was\n> > created somewhere else to UTC). That way, we do not have to futz\n> > with TZ environment or tzset.\n> \n> Yes, I think this is simpler and nicer than the proposal in my other\n> reply.\n\nYeah, I agree this is more desirable.\n\n> In addition to not having to futz with TZ, I think I like the\n> semantics better. The motivation that started this thread was not so\n> much \"I want to set a custom timezone to blend in\" but rather \"why are\n> we recording the timezone at all here?\" In that context, it makes\n> sense to me to have a setting such as\n> \n> \tcore.recordTimeZone\n> \n> that I can turn *off* to say that I don't think datestamp() callers\n> should consider the timezone to be information worth recording (and\n> instead they should write +0000). To me that seems a little simpler\n> to understand than user.hideTimezone since this focuses on turning\n> some functionality off (recording of the time zone) instead of turning\n> on a new stealth mode.\n\nI'd like to make one suggestion here, and that's that instead of writing\n\"+0000\" in this case, we write \"-0000\". As far as I'm aware, it should\nbe parsed equivalently but it mirrors RFC 5322:\n\n Though \"-0000\" also indicates Universal Time, it is used to indicate\n that the time was generated on a system that may be in a local time\n zone other than Universal Time and that the date-time contains no\n information about the local time zone.\n\nThis is exactly my case. As you can tell from my emails, I'm not\nphysically located in a UTC timezone, but my system is in UTC and uses\nthat for timestamps. I use UTC because I know and work with people from\naround the world and it's more convenient to use an objective standard;\nmy real time zone is unimportant. That's materially different than\nsomeone who's located in Reykjavík, where we'd want to write +0000,\nsince they are physically located in a UTC-equivalent timezone.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"406874","messageId":"xmqq5z7r7ya7.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20201003195325.GL1392312@camp.crustytoothpaste.net","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-03T22:14:08Z","receivedAt":"2020-10-03T22:14:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> I'd like to make one suggestion here, and that's that instead of writing\n> \"+0000\" in this case, we write \"-0000\". As far as I'm aware, it should\n> be parsed equivalently but it mirrors RFC 5322:\n>\n> Though \"-0000\" also indicates Universal Time, it is used to indicate\n> that the time was generated on a system that may be in a local time\n> zone other than Universal Time and that the date-time contains no\n> information about the local time zone.\n>\n> This is exactly my case. As you can tell from my emails, I'm not\n> physically located in a UTC timezone, but my system is in UTC and uses\n> that for timestamps.\n\nWhat you had in the header of the message I am responding to was:\n\n Date: Sat, 3 Oct 2020 19:53:25 +0000\n\nI cannot tell if somebody in the middle rewrote -0000 to +0000, or\nyour original already said +0000 in your message.\n\n> I use UTC because I know and work with people from\n> around the world and it's more convenient to use an objective standard;\n> my real time zone is unimportant.\n\nIt cuts both ways, and there is a small downside, though. As I\nassume most people are not actively hacking on Git in the early\nmorning say 01am-04am in their local timezone, I may delay my\nresponse to a message from a contributor if I notice that the day\nfor that contributor is already over and my message will not be read\nfor some time even if I rushed it. \"real time zone is unimportant\"\nmay or may not be true. A responder in such a case is actively\ninconvenienced by the zone information being hidden [*1*].\n\nHaving said that.\n\n> That's materially different than\n> someone who's located in Reykjavík, where we'd want to write +0000,\n> since they are physically located in a UTC-equivalent timezone.\n\nI agree that it would be a good thing to have a way to say \"I am not\ntelling which zone the timestamp is from\", that can be used to\ndifferenciate \"This timestamp is in UTC because I am in UTC\".\n\n\n[Footnote]\n\n*1* How much the person feel inconvenienced is very subjective, so\n let's not go into \"it's just a small inconvenience---my privacy\n is more valuable\" kind of discussion. The value of privacy is\n subjective in the same way and we shouldn't compare subjective\n things to decide which side must bend their way to accomodate\n the other side.\n\n"},{"id":"407179","messageId":"xmqqk0w0wl9t.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"xmqqh7rec5h3.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-08T19:49:34Z","receivedAt":"2020-10-08T19:49:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> In addition to not having to futz with TZ, I think I like the\n>> semantics better. The motivation that started this thread was not so\n>> much \"I want to set a custom timezone to blend in\" but rather \"why are\n>> we recording the timezone at all here?\" In that context, it makes\n>> sense to me to have a setting such as\n>>\n>> \tcore.recordTimeZone\n>>\n>> that I can turn *off* to say that I don't think datestamp() callers\n>> should consider the timezone to be information worth recording (and\n>> instead they should write +0000). To me that seems a little simpler\n>> to understand than user.hideTimezone since this focuses on turning\n>> some functionality off (recording of the time zone) instead of turning\n>> on a new stealth mode.\n>\n> Hmph. It is a valid way to look at the issue, I guess.\n>\n> Thanks for an input.\n\nI do not have a strong opinion on recordTimeZone vs hideTimeZone any\nmore (the latter, as proposed by Shengfa, is shorter to type ;-),\nbut I think it is a good idea to keep it in user.* hierarchy. It\nsits next to user.name and user.email and controls how the user ident\nis formulated.\n\n"},{"id":"407221","messageId":"xmqqimbjtkfz.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"CAEOYnASgxCE5NjhoSgDwyQyAmdLhw5UyFq_Fu==8q7y6uXGz6w@mail.gmail.com","subject":"Re: [RFC PATCH 0/1] adding user.hideTimezone for setting UTC timezone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T16:48:00Z","receivedAt":"2020-10-09T16:48:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nathaniel Manista writes:\n\n> If we say that we would like to eventually live in a world in which a\n> user's time zones are not recorded until after that user deliberately opts\n> into git recording zir time zones, does that point us toward an eventual\n> destination of \"users who don't wish to record their time zones don't have\n> to do anything to their .gitconfigs, and users who do wish to record their\n> time zones only have to write one line in their .gitconfigs\"? If that's the\n> case, ought that one line that some users choose to write be targeted to be\n> \"recordTimeZone = true\" rather than anything else (particularly rather than\n> \"hideTimeZone = false\")? If all that holds, does\n>\n> today: introduce recordTimeZone with a default of true and advertise its\n> existence so that those users who have feelings one way or the other on the\n> matter may explicitly set it to one value or the other\n> six months from today: flip recordTimeZone's default from true to false\n>\n> look like a plausible sketch of how to get to the desired future state?\n>\n> What am I missing (and 🤞 that my \"if\"s hold too...)?\n\nSince it is perfectly OK to have a configuration variable whose\ndefault value is 'true', the choice between the world in which times\nare by default hidden and the opposite world does not affect which\nway the configuration variable should be named and defined at all.\n\nThe system could hide zone when there is no user.hideTimeZone\nconfigured, and those who think that the value of giving others a\nuseful bit of info outweighs the value of hiding their own zone can\nset it explicitly to 'false' to expose their zone.\n\nOr the system could record zone when there is no user.recordTimeZone\nconfigured, and you can set it explicitly to 'false' if you think if\nit is more valuable to omit your zone from the commit than recording\nthe zone in which the commit was written.\n\nEither way, we can make the system to \"do what the majority of users\nwant, with an escape hatch to protect monority\"; the naming does not\naffect it in any way.\n\nThe only thing we should consider when naming, therefore, is how\nclearly the name conveys the semantics.\n\nAnd from that point of view, user.hideTimeZone=yes is a mistake. It\ncan be mistaken as if we do record but simply choose not to show at\ntime of the output. Contrast that with user.recordTimeZone=no which\nhas no such room for confusion. If we are not recording, there is\nnothing clever to be done when showing.\n\nFWIW, I am not in favor of dropping info by default myself, but that\nis outside the scope of this message.\n"},{"id":"407423","messageId":"20201013052851.373029-1-shengfa@google.com","threadId":"52389","inReplyTo":"CAEOYnAQYMrNAe9s1V-0DVLdL-B_KpHMDP5e=yRnbCkMWdrvFHQ@mail.gmail.com","subject":"[WIP v2 0/2] experiment with commit option record-time-zone","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-13T05:28:49Z","receivedAt":"2020-10-13T05:29:14Z","isPatch":false,"sender":{"key":"shengfa@google.com","avatar":null},"body":"My current understanding is that we should start with providing command\noptions first and if they prove to be useful, we can progress with\nconfig such as user.recordTimeZone. Also, we wanted to save the\ntimestamp as -0000 when user specify not to record time zone so to\ndistinguish with +0000(user from UTC time zone).\n\nExperimenting to support command option --[no-]record-time-zone, currently only\nadded for commit command; the plan is to add it to other time zone\nrecording command such as rebase, pull and merge. Documentation change\nis also missing.\n\nI added three tests where the first two are failing. I am not sure why\nthey are failing. I am guessing the datestamp method is not called when\nspecifying the date or GIT_AUTHOR_DATE. Also, I am not sure the correct\nhandling of all the show_date calls yet. Anyway, I wanted to share WIP\nbefore delaying longer.\n\n\nShengfa Lin (2):\n Adding a record-time-zone command option for commit\n Demonstrate failing and passing tests\n\n builtin/am.c | 2 +-\n builtin/blame.c | 4 +-\n builtin/commit.c | 3 +-\n builtin/fast-import.c | 2 +-\n builtin/show-branch.c | 2 +-\n builtin/tag.c | 2 +-\n cache.h | 10 +++--\n date.c | 96 ++++++++++++++++++++++++++++-------------\n http-backend.c | 2 +-\n pretty.c | 6 ++-\n ref-filter.c | 2 +-\n reflog-walk.c | 2 +-\n refs.c | 6 ++-\n sha1-name.c | 2 +-\n t/helper/test-date.c | 8 ++--\n t/t7514-commit-patch.sh | 28 ++++++++++++\n 16 files changed, 126 insertions(+), 51 deletions(-)\n\n-- \n2.28.0.1011.ga647a8990f-goog\n\n"},{"id":"407424","messageId":"20201013052851.373029-3-shengfa@google.com","threadId":"52389","inReplyTo":"20201013052851.373029-1-shengfa@google.com","subject":"[WIP v2 2/2] Demonstrate failing and passing tests","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-13T05:28:51Z","receivedAt":"2020-10-13T05:31:21Z","isPatch":false,"sender":{"key":"shengfa@google.com","avatar":null},"body":"date=@... and GIT_AUTHOR_DATE=@... tests are failing\nwhile unsetting GIT_AUTHOR_DATE passes\n\nSigned-off-by: Shengfa Lin \n---\n t/t7514-commit-patch.sh | 25 ++++++++++++++++++++++---\n 1 file changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nindex 3ba1ff4f81..b87d29f020 100755\n--- a/t/t7514-commit-patch.sh\n+++ b/t/t7514-commit-patch.sh\n@@ -31,13 +31,32 @@ test_expect_success 'edit hunk \"commit --dry-run -p -m message\"' '\n \ttest -r editor_was_started\n '\n \n-test_expect_success 'commit date shows timezone offset -0000 when no-record-time-zone is specified' '\n- echo test >>file &&\n+test_expect_success 'commit with --date shows timezone offset -0000 when no-record-time-zone is specified' '\n+ echo test1 >>file &&\n git add file &&\n- TZ=UTC-09 git commit --date=@1600000000 -m \"test\" --no-record-time-zone &&\n+ TZ=UTC-09 git commit --date=@1600000000 -m \"test1\" --no-record-time-zone &&\n git show -s --format='%aI' >output &&\n echo 2020-09-13T12:26:40-00:00 >expect &&\n test_cmp output expect\n '\n \n+test_expect_success 'commit with GIT_AUTHOR_DATE shows timezone offset -0000 when no-record-time-zone is specified' '\n+ echo test2 >>file &&\n+ git add file &&\n+ export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 &&\n+ git commit -m \"test2\" --no-record-time-zone &&\n+ git show -s --format='%aI' >output &&\n+ echo 2020-09-13T12:26:40-00:00 >expect &&\n+ test_cmp output expect\n+'\n+\n+test_expect_success 'commit with unset GIT_AUTHOR_DATE shows timezone offset -0000 when no-record-time-zone is specified' '\n+ echo test2 >>file &&\n+ git add file &&\n+ unset GIT_AUTHOR_DATE &&\n+ TZ=UTC-09 git commit -m \"test2\" --no-record-time-zone &&\n+ git show -s --format='%aI' >output &&\n+ grep \"\\-00:00\" output\n+'\n+\n test_done\n-- \n2.28.0.1011.ga647a8990f-goog\n\n"},{"id":"407425","messageId":"20201013052851.373029-2-shengfa@google.com","threadId":"52389","inReplyTo":"20201013052851.373029-1-shengfa@google.com","subject":"[WIP v2 1/2] Adding a record-time-zone command option for commit","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-13T05:28:50Z","receivedAt":"2020-10-13T05:31:21Z","isPatch":false,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Many places in Git record the timezone of actor when a\ntimestamp is recorded, including the commiter and author\ntimestamps in a commit object and the tagger timestamp in a tag\nobject. Some people however prefer not to share where they\nactually are.\n\nThey _could_ just say \"export TZ=UTC\" and be done with it,\nbut the method would not easily allow them to pretend to be\nin the UTC timezone only with Git, while revealing their true\ntimezone to other activities (e.g. sending e-emails?).\n\nIntroduce --[no-]record-time-zone for commit command, which can be\nspecified to disallow Git to record time zone. Timezone will be\nrecorded by default.\n\nNote that this is a WIP and the test is failing.\n\nSigned-off-by: Shengfa Lin \n---\n builtin/am.c | 2 +-\n builtin/blame.c | 4 +-\n builtin/commit.c | 3 +-\n builtin/fast-import.c | 2 +-\n builtin/show-branch.c | 2 +-\n builtin/tag.c | 2 +-\n cache.h | 10 +++--\n date.c | 96 ++++++++++++++++++++++++++++-------------\n http-backend.c | 2 +-\n pretty.c | 6 ++-\n ref-filter.c | 2 +-\n reflog-walk.c | 2 +-\n refs.c | 6 ++-\n sha1-name.c | 2 +-\n t/helper/test-date.c | 8 ++--\n t/t7514-commit-patch.sh | 9 ++++\n 16 files changed, 107 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 2c7673f74e..059cc5fce7 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -884,7 +884,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)\n \t\t\tif (tz > 0)\n \t\t\t\ttz2 = -tz2;\n \n-\t\t\tfprintf(out, \"Date: %s\\n\", show_date(timestamp, tz2, DATE_MODE(RFC2822)));\n+\t\t\tfprintf(out, \"Date: %s\\n\", show_date(timestamp, tz2, NULL, DATE_MODE(RFC2822)));\n \t\t} else if (starts_with(sb.buf, \"# \")) {\n \t\t\tcontinue;\n \t\t} else {\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bb0f29300e..8205f01868 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -306,7 +306,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,\n \t\tsize_t time_width;\n \t\tint tz;\n \t\ttz = atoi(tz_str);\n-\t\ttime_str = show_date(time, tz, &blame_date_mode);\n+\t\ttime_str = show_date(time, tz, NULL, &blame_date_mode);\n \t\tstrbuf_addstr(&time_buf, time_str);\n \t\t/*\n \t\t * Add space paddings to time_buf to display a fixed width\n@@ -1002,7 +1002,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tblame_date_width = sizeof(\"Thu Oct 19 16:00:04 2006 -0700\");\n \t\tbreak;\n \tcase DATE_STRFTIME:\n-\t\tblame_date_width = strlen(show_date(0, 0, &blame_date_mode)) + 1; /* add the null */\n+\t\tblame_date_width = strlen(show_date(0, 0, NULL, &blame_date_mode)) + 1; /* add the null */\n \t\tbreak;\n \t}\n \tblame_date_width -= 1; /* strip the null */\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1dfd799ec5..ee3ca2c7ac 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1547,7 +1547,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"ok to record an empty change\")),\n \t\tOPT_HIDDEN_BOOL(0, \"allow-empty-message\", &allow_empty_message,\n \t\t\t\tN_(\"ok to record a change with an empty message\")),\n-\n+ OPT_BOOL(0, \"record-time-zone\", &record_time_zone, N_(\"record user timezone\")),\n \t\tOPT_END()\n \t};\n \n@@ -1580,6 +1580,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \targc = parse_and_validate_options(argc, argv, builtin_commit_options,\n \t\t\t\t\t builtin_commit_usage,\n \t\t\t\t\t prefix, current_head, &s);\n+\n \tif (verbose == -1)\n \t\tverbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;\n \ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 1bf50a73dc..5aebe70670 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -327,7 +327,7 @@ static void write_crash_report(const char *err)\n \tfprintf(rpt, \"fast-import crash report:\\n\");\n \tfprintf(rpt, \" fast-import process: %\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n \tfprintf(rpt, \" parent process : %\"PRIuMAX\"\\n\", (uintmax_t) getppid());\n-\tfprintf(rpt, \" at %s\\n\", show_date(time(NULL), 0, DATE_MODE(ISO8601)));\n+\tfprintf(rpt, \" at %s\\n\", show_date(time(NULL), 0, NULL, DATE_MODE(ISO8601)));\n \tfputc('\\n', rpt);\n \n \tfputs(\"fatal: \", rpt);\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d6d2dabeca..e4210c06ab 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -777,7 +777,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\telse\n \t\t\t\tmsg++;\n \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n-\t\t\t\t\t\tshow_date(timestamp, tz,\n+\t\t\t\t\t\tshow_date(timestamp, tz, NULL,\n \t\t\t\t\t\t\t DATE_MODE(RELATIVE)),\n \t\t\t\t\t\tmsg);\n \t\t\tfree(logmsg);\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ecf011776d..6ee3c7109e 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -332,7 +332,7 @@ static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)\n \t\tfree(buf);\n \n \t\tif ((c = lookup_commit_reference(the_repository, oid)) != NULL)\n-\t\t\tstrbuf_addf(sb, \", %s\", show_date(c->date, 0, DATE_MODE(SHORT)));\n+\t\t\tstrbuf_addf(sb, \", %s\", show_date(c->date, 0, NULL, DATE_MODE(SHORT)));\n \t\tbreak;\n \tcase OBJ_TREE:\n \t\tstrbuf_addstr(sb, \"tree object\");\ndiff --git a/cache.h b/cache.h\nindex c0072d43b1..84f9e88649 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1625,17 +1625,18 @@ struct date_mode {\n /*\n * Convenience helper for passing a constant type, like:\n *\n- * show_date(t, tz, DATE_MODE(NORMAL));\n+ * show_date(t, tz, NULL, DATE_MODE(NORMAL));\n */\n #define DATE_MODE(t) date_mode_from_type(DATE_##t)\n struct date_mode *date_mode_from_type(enum date_mode_type type);\n \n-const char *show_date(timestamp_t time, int timezone, const struct date_mode *mode);\n+const char *show_date(timestamp_t time, int timezone, int *sign, const struct date_mode *mode);\n void show_date_relative(timestamp_t time, struct strbuf *timebuf);\n void show_date_human(timestamp_t time, int tz, const struct timeval *now,\n \t\t\tstruct strbuf *timebuf);\n int parse_date(const char *date, struct strbuf *out);\n-int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset);\n+int parse_date_basic(const char *date, timestamp_t *timestamp,\n+ int *offset, int *zero_offset_negative_sign);\n int parse_expiry_date(const char *date, timestamp_t *timestamp);\n void datestamp(struct strbuf *out);\n #define approxidate(s) approxidate_careful((s), NULL)\n@@ -1979,4 +1980,7 @@ int print_sha1_ellipsis(void);\n /* Return 1 if the file is empty or does not exists, 0 otherwise. */\n int is_empty_or_missing_file(const char *filename);\n \n+/* date.c */\n+extern int record_time_zone;\n+\n #endif /* CACHE_H */\ndiff --git a/date.c b/date.c\nindex f9ea807b3a..f32ab51f2b 100644\n--- a/date.c\n+++ b/date.c\n@@ -6,6 +6,8 @@\n \n #include \"cache.h\"\n \n+int record_time_zone = 1;\n+\n /*\n * This is like mktime, but without normalization of tm_wday and tm_yday.\n */\n@@ -213,7 +215,21 @@ struct date_mode *date_mode_from_type(enum date_mode_type type)\n \treturn &mode;\n }\n \n-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)\n+static int negative_zero(int tz, int *sign)\n+{\n+ return !tz && sign && (*sign) == '-';\n+}\n+\n+static const char *tz_fmt(int tz, int *sign)\n+{\n+ if (!negative_zero(tz, sign))\n+ return \" %+05d\";\n+ else\n+ return \" -%04d\";\n+}\n+\n+\n+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)\n {\n \tstruct {\n \t\tunsigned int\tyear:1,\n@@ -277,10 +293,10 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm\n \t\tstrbuf_addf(buf, \" %d\", tm->tm_year + 1900);\n \n \tif (!hide.tz)\n-\t\tstrbuf_addf(buf, \" %+05d\", tz);\n+ strbuf_addf(buf, tz_fmt(tz, sign), tz);\n }\n \n-const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n+const char *show_date(timestamp_t time, int tz, int *signp, const struct date_mode *mode)\n {\n \tstruct tm *tm;\n \tstruct tm tmbuf = { 0 };\n@@ -331,15 +347,18 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \tif (mode->type == DATE_SHORT)\n \t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02d\", tm->tm_year + 1900,\n \t\t\t\ttm->tm_mon + 1, tm->tm_mday);\n-\telse if (mode->type == DATE_ISO8601)\n-\t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02d %02d:%02d:%02d %+05d\",\n+\telse if (mode->type == DATE_ISO8601) {\n+\t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02d %02d:%02d:%02d\",\n \t\t\t\ttm->tm_year + 1900,\n \t\t\t\ttm->tm_mon + 1,\n \t\t\t\ttm->tm_mday,\n-\t\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec,\n-\t\t\t\ttz);\n+\t\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec);\n+\t\tstrbuf_addf(&timebuf, tz_fmt(tz, signp), tz);\n+ }\n \telse if (mode->type == DATE_ISO8601_STRICT) {\n \t\tchar sign = (tz >= 0) ? '+' : '-';\n+ if (negative_zero(tz, signp))\n+ sign = '-';\n \t\ttz = abs(tz);\n \t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02dT%02d:%02d:%02d%c%02d:%02d\",\n \t\t\t\ttm->tm_year + 1900,\n@@ -347,16 +366,18 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \t\t\t\ttm->tm_mday,\n \t\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec,\n \t\t\t\tsign, tz / 100, tz % 100);\n-\t} else if (mode->type == DATE_RFC2822)\n-\t\tstrbuf_addf(&timebuf, \"%.3s, %d %.3s %d %02d:%02d:%02d %+05d\",\n+\t} else if (mode->type == DATE_RFC2822) {\n+\t\tstrbuf_addf(&timebuf, \"%.3s, %d %.3s %d %02d:%02d:%02d\",\n \t\t\tweekday_names[tm->tm_wday], tm->tm_mday,\n \t\t\tmonth_names[tm->tm_mon], tm->tm_year + 1900,\n-\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec, tz);\n+\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec);\n+\t\tstrbuf_addf(&timebuf, tz_fmt(tz, signp), tz);\n+ }\n \telse if (mode->type == DATE_STRFTIME)\n \t\tstrbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,\n \t\t\t\t!mode->local);\n \telse\n-\t\tshow_date_normal(&timebuf, time, tm, tz, &human_tm, human_tz, mode->local);\n+\t\tshow_date_normal(&timebuf, time, tm, tz, signp, &human_tm, human_tz, mode->local);\n \treturn timebuf.buf;\n }\n \n@@ -786,14 +807,16 @@ static int match_tz(const char *date, int *offp)\n \treturn end - date;\n }\n \n-static void date_string(timestamp_t date, int offset, struct strbuf *buf)\n+static void date_string(timestamp_t date, int offset,\n+ int zero_offset_negative_sign, struct strbuf *buf)\n {\n \tint sign = '+';\n+ if (offset < 0) {\n+ offset = -offset;\n+ sign = '-';\n+ } else if (!offset && zero_offset_negative_sign)\n+ sign = '-';\n \n-\tif (offset < 0) {\n-\t\toffset = -offset;\n-\t\tsign = '-';\n-\t}\n \tstrbuf_addf(buf, \"%\"PRItime\" %c%02d%02d\", date, sign, offset / 60, offset % 60);\n }\n \n@@ -826,17 +849,21 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in\n \n /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822\n (i.e. English) day/month names, and it doesn't work correctly with %z. */\n-int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n+int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset, int *zero_offset_negative_sign)\n {\n \tstruct tm tm;\n \tint tm_gmt;\n \ttimestamp_t dummy_timestamp;\n \tint dummy_offset;\n+ int dummy_zero_offset_negative_sign;\n+ int negative_sign;\n \n \tif (!timestamp)\n \t\ttimestamp = &dummy_timestamp;\n \tif (!offset)\n \t\toffset = &dummy_offset;\n+ if (!zero_offset_negative_sign)\n+ zero_offset_negative_sign = &dummy_zero_offset_negative_sign;\n \n \tmemset(&tm, 0, sizeof(tm));\n \ttm.tm_year = -1;\n@@ -848,6 +875,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n \ttm.tm_sec = -1;\n \t*offset = -1;\n \ttm_gmt = 0;\n+ negative_sign = 0;\n \n \tif (*date == '@' &&\n \t !match_object_header_date(date + 1, timestamp, offset))\n@@ -864,9 +892,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n \t\t\tmatch = match_alpha(date, &tm, offset);\n \t\telse if (isdigit(c))\n \t\t\tmatch = match_digit(date, &tm, offset, &tm_gmt);\n-\t\telse if ((c == '-' || c == '+') && isdigit(date[1]))\n+\t\telse if ((c == '-' || c == '+') && isdigit(date[1])) {\n \t\t\tmatch = match_tz(date, offset);\n-\n+ if (c == '-')\n+ negative_sign = 1;\n+ }\n \t\tif (!match) {\n \t\t\t/* BAD CRAP */\n \t\t\tmatch = 1;\n@@ -895,6 +925,9 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n \n \tif (!tm_gmt)\n \t\t*timestamp -= *offset * 60;\n+\n+ *zero_offset_negative_sign = (!(*offset) && negative_sign);\n+\n \treturn 0; /* success */\n }\n \n@@ -924,9 +957,10 @@ int parse_date(const char *date, struct strbuf *result)\n {\n \ttimestamp_t timestamp;\n \tint offset;\n-\tif (parse_date_basic(date, ×tamp, &offset))\n+ int zero_offset_negative_sign;\n+\tif (parse_date_basic(date, ×tamp, &offset, &zero_offset_negative_sign))\n \t\treturn -1;\n-\tdate_string(timestamp, offset, result);\n+\tdate_string(timestamp, offset, zero_offset_negative_sign, result);\n \treturn 0;\n }\n \n@@ -996,15 +1030,16 @@ void parse_date_format(const char *format, struct date_mode *mode)\n void datestamp(struct strbuf *out)\n {\n \ttime_t now;\n-\tint offset;\n-\tstruct tm tm = { 0 };\n+\tint offset = 0;\n \n-\ttime(&now);\n+ time(&now);\n+ if (record_time_zone) {\n+ struct tm tm = { 0 };\n+ offset = tm_to_time_t(localtime_r(&now, &tm)) - now;\n+ offset /= 60;\n+ }\n \n-\toffset = tm_to_time_t(localtime_r(&now, &tm)) - now;\n-\toffset /= 60;\n-\n-\tdate_string(now, offset, out);\n+\tdate_string(now, offset, !record_time_zone, out);\n }\n \n /*\n@@ -1330,7 +1365,7 @@ timestamp_t approxidate_relative(const char *date)\n \tint offset;\n \tint errors = 0;\n \n-\tif (!parse_date_basic(date, ×tamp, &offset))\n+\tif (!parse_date_basic(date, ×tamp, &offset, NULL))\n \t\treturn timestamp;\n \n \tget_time(&tv);\n@@ -1346,7 +1381,7 @@ timestamp_t approxidate_careful(const char *date, int *error_ret)\n \tif (!error_ret)\n \t\terror_ret = &dummy;\n \n-\tif (!parse_date_basic(date, ×tamp, &offset)) {\n+\tif (!parse_date_basic(date, ×tamp, &offset, NULL)) {\n \t\t*error_ret = 0;\n \t\treturn timestamp;\n \t}\n@@ -1371,3 +1406,4 @@ int date_overflows(timestamp_t t)\n \tsys = t;\n \treturn t != sys || (t < 1) != (sys < 1);\n }\n+\ndiff --git a/http-backend.c b/http-backend.c\nindex a03b4bae22..644c4c7af6 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -97,7 +97,7 @@ static void hdr_int(struct strbuf *hdr, const char *name, uintmax_t value)\n \n static void hdr_date(struct strbuf *hdr, const char *name, timestamp_t when)\n {\n-\tconst char *value = show_date(when, 0, DATE_MODE(RFC2822));\n+\tconst char *value = show_date(when, 0, NULL, DATE_MODE(RFC2822));\n \thdr_str(hdr, name, value);\n }\n \ndiff --git a/pretty.c b/pretty.c\nindex 7a7708a0ea..9b86e4ec12 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -416,6 +416,7 @@ const char *show_ident_date(const struct ident_split *ident,\n {\n \ttimestamp_t date = 0;\n \tlong tz = 0;\n+ int sign = '+';\n \n \tif (ident->date_begin && ident->date_end)\n \t\tdate = parse_timestamp(ident->date_begin, NULL, 10);\n@@ -426,8 +427,11 @@ const char *show_ident_date(const struct ident_split *ident,\n \t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n \t\tif (tz >= INT_MAX || tz <= INT_MIN)\n \t\t\ttz = 0;\n+ else if (!tz && ident->tz_begin &&\n+ (*ident->tz_begin) == '-')\n+ sign = '-';\n \t}\n-\treturn show_date(date, tz, mode);\n+\treturn show_date(date, tz, &sign, mode);\n }\n \n void pp_user_info(struct pretty_print_context *pp,\ndiff --git a/ref-filter.c b/ref-filter.c\nindex c62f6b4822..85bf338c99 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1136,7 +1136,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \ttz = strtol(zone, NULL, 10);\n \tif ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)\n \t\tgoto bad;\n-\tv->s = xstrdup(show_date(timestamp, tz, &date_mode));\n+\tv->s = xstrdup(show_date(timestamp, tz, NULL, &date_mode));\n \tv->value = timestamp;\n \treturn;\n bad:\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 3a25b27d8f..2a9c7cadad 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -224,7 +224,7 @@ void get_reflog_selector(struct strbuf *sb,\n \tif (commit_reflog->selector == SELECTOR_DATE ||\n \t (commit_reflog->selector == SELECTOR_NONE && force_date)) {\n \t\tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n-\t\tstrbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));\n+\t\tstrbuf_addstr(sb, show_date(info->timestamp, info->tz, NULL, dmode));\n \t} else {\n \t\tstrbuf_addf(sb, \"%d\", commit_reflog->reflogs->nr\n \t\t\t - 2 - commit_reflog->recno);\ndiff --git a/refs.c b/refs.c\nindex fa01153151..6cb056a2d1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -890,13 +890,15 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n \t\t\toidcpy(cb->oid, noid);\n \t\t\tif (!oideq(&cb->ooid, noid))\n \t\t\t\twarning(_(\"log for ref %s has gap after %s\"),\n-\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n+\t\t\t\t\tcb->refname, \n+ show_date(cb->date, cb->tz,\n+ NULL, DATE_MODE(RFC2822)));\n \t\t}\n \t\telse if (cb->date == cb->at_time)\n \t\t\toidcpy(cb->oid, noid);\n \t\telse if (!oideq(noid, cb->oid))\n \t\t\twarning(_(\"log for ref %s unexpectedly ended on %s\"),\n-\t\t\t\tcb->refname, show_date(cb->date, cb->tz,\n+\t\t\t\tcb->refname, show_date(cb->date, cb->tz, NULL,\n \t\t\t\t\t\t DATE_MODE(RFC2822)));\n \t\toidcpy(&cb->ooid, ooid);\n \t\toidcpy(&cb->noid, noid);\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 0b23b86ceb..9362c20803 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -915,7 +915,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\t\t\tif (!(flags & GET_OID_QUIETLY)) {\n \t\t\t\t\twarning(_(\"log for '%.*s' only goes back to %s\"),\n \t\t\t\t\t\tlen, str,\n-\t\t\t\t\t\tshow_date(co_time, co_tz, DATE_MODE(RFC2822)));\n+\t\t\t\t\t\tshow_date(co_time, co_tz, NULL, DATE_MODE(RFC2822)));\n \t\t\t\t}\n \t\t\t} else {\n \t\t\t\tif (flags & GET_OID_QUIETLY) {\ndiff --git a/t/helper/test-date.c b/t/helper/test-date.c\nindex 099eff4f0f..cf8d4574b7 100644\n--- a/t/helper/test-date.c\n+++ b/t/helper/test-date.c\n@@ -28,7 +28,7 @@ static void show_human_dates(const char **argv)\n {\n \tfor (; *argv; argv++) {\n \t\ttime_t t = atoi(*argv);\n-\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, 0, DATE_MODE(HUMAN)));\n+\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, 0, NULL, DATE_MODE(HUMAN)));\n \t}\n }\n \n@@ -51,7 +51,7 @@ static void show_dates(const char **argv, const char *format)\n \t\t\targ++;\n \t\ttz = atoi(arg);\n \n-\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, tz, &mode));\n+\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, tz, NULL, &mode));\n \t}\n }\n \n@@ -67,7 +67,7 @@ static void parse_dates(const char **argv)\n \t\tparse_date(*argv, &result);\n \t\tif (sscanf(result.buf, \"%\"PRItime\" %d\", &t, &tz) == 2)\n \t\t\tprintf(\"%s -> %s\\n\",\n-\t\t\t *argv, show_date(t, tz, DATE_MODE(ISO8601)));\n+\t\t\t *argv, show_date(t, tz, NULL, DATE_MODE(ISO8601)));\n \t\telse\n \t\t\tprintf(\"%s -> bad\\n\", *argv);\n \t}\n@@ -79,7 +79,7 @@ static void parse_approxidate(const char **argv)\n \tfor (; *argv; argv++) {\n \t\ttimestamp_t t;\n \t\tt = approxidate_relative(*argv);\n-\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, 0, DATE_MODE(ISO8601)));\n+\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, 0, NULL, DATE_MODE(ISO8601)));\n \t}\n }\n \ndiff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nindex 998a2103c7..3ba1ff4f81 100755\n--- a/t/t7514-commit-patch.sh\n+++ b/t/t7514-commit-patch.sh\n@@ -31,4 +31,13 @@ test_expect_success 'edit hunk \"commit --dry-run -p -m message\"' '\n \ttest -r editor_was_started\n '\n \n+test_expect_success 'commit date shows timezone offset -0000 when no-record-time-zone is specified' '\n+ echo test >>file &&\n+ git add file &&\n+ TZ=UTC-09 git commit --date=@1600000000 -m \"test\" --no-record-time-zone &&\n+ git show -s --format='%aI' >output &&\n+ echo 2020-09-13T12:26:40-00:00 >expect &&\n+ test_cmp output expect\n+'\n+\n test_done\n-- \n2.28.0.1011.ga647a8990f-goog\n\n"},{"id":"407483","messageId":"xmqqk0vtki66.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20201013052851.373029-2-shengfa@google.com","subject":"Re: [WIP v2 1/2] Adding a record-time-zone command option for commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T20:03:13Z","receivedAt":"2020-10-13T20:03:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\n> Many places in Git record the timezone of actor when a\n> timestamp is recorded, including the commiter and author\n> timestamps in a commit object and the tagger timestamp in a tag\n> object. Some people however prefer not to share where they\n> actually are.\n>\n> They _could_ just say \"export TZ=UTC\" and be done with it,\n> but the method would not easily allow them to pretend to be\n> in the UTC timezone only with Git, while revealing their true\n> timezone to other activities (e.g. sending e-emails?).\n>\n> Introduce --[no-]record-time-zone for commit command, which can be\n> specified to disallow Git to record time zone. Timezone will be\n> recorded by default.\n>\n> Note that this is a WIP and the test is failing.\n\nAnd there is no outline of the design decision made so far, so it is\nhard to give useful review comments.\n\nIt does not help that the patch is distracting by turning tabs to\nspaces and breaking alingment X-<.\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 2c7673f74e..059cc5fce7 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -884,7 +884,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)\n> \t\t\tif (tz > 0)\n> \t\t\t\ttz2 = -tz2;\n> \n> -\t\t\tfprintf(out, \"Date: %s\\n\", show_date(timestamp, tz2, DATE_MODE(RFC2822)));\n> +\t\t\tfprintf(out, \"Date: %s\\n\", show_date(timestamp, tz2, NULL, DATE_MODE(RFC2822)));\n\nFor example, it appears that tweaking \"show_date()\" API function\nseems to be a central part of the design, as there are so many\ninstances like this change in the patch. If the proposed log\nmessage mentioned, even briefly, what the extra parameter added to\nthe API function meant (especially what NULL means there) upfront,\nthen readers can coast the part that added NULL there, like these\nones, and concentrate on the parts of this patch that matter, which\npresumably uses something more interesting than NULL instead.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 1dfd799ec5..ee3ca2c7ac 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1547,7 +1547,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \t\t\t\tN_(\"ok to record an empty change\")),\n> \t\tOPT_HIDDEN_BOOL(0, \"allow-empty-message\", &allow_empty_message,\n> \t\t\t\tN_(\"ok to record a change with an empty message\")),\n> -\n> + OPT_BOOL(0, \"record-time-zone\", &record_time_zone, N_(\"record user timezone\")),\n\nOur code indents with HT; get used to the style early and your\npatches won't distract reviewers as much, leading to more quality\nreviews and suggestions.\n\nLikewise. The record_time_zone global variable seems to play a\ncrucial role in this change, but without preparing readers by\nmentioning where it is defined, what normal/default value it takes,\nand who potentially touches it, in the proposed log message, it\nmakes reading the change harder than necessary.\n\nA system-wide global like this is usually defined in environment.c,\nby the way. Look for say trust_executable_bit and mimic where it \nis defined and declared.\n\n> \t\tOPT_END()\n> \t};\n> \n> @@ -1580,6 +1580,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \targc = parse_and_validate_options(argc, argv, builtin_commit_options,\n> \t\t\t\t\t builtin_commit_usage,\n> \t\t\t\t\t prefix, current_head, &s);\n> +\n> \tif (verbose == -1)\n> \t\tverbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;\n> \n\nDistraction.\n\n> +static int negative_zero(int tz, int *sign)\n> +{\n> + return !tz && sign && (*sign) == '-';\n> +}\n> +\n> +static const char *tz_fmt(int tz, int *sign)\n> +{\n> + if (!negative_zero(tz, sign))\n> + return \" %+05d\";\n> + else\n> + return \" -%04d\";\n> +}\n> +\n> +\n> +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)\n> {\n> \tstruct {\n> \t\tunsigned int\tyear:1,\n> @@ -277,10 +293,10 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm\n> \t\tstrbuf_addf(buf, \" %d\", tm->tm_year + 1900);\n> \n> \tif (!hide.tz)\n> -\t\tstrbuf_addf(buf, \" %+05d\", tz);\n> + strbuf_addf(buf, tz_fmt(tz, sign), tz);\n> }\n> \n> -const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n> +const char *show_date(timestamp_t time, int tz, int *signp, const struct date_mode *mode)\n\nWith its type, we can tell easily that sign is a pointer, so no need\nfor 'p' (we do not have modep, either, next door). More important\nis if 'sign' is a good name that conveys what the parameter (which\nis almost always NULL) means. Without any introductory text, it is\nhard to tell and offer recommendations.\n\n> @@ -826,17 +849,21 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in\n> \n> /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822\n> (i.e. English) day/month names, and it doesn't work correctly with %z. */\n> -int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n> +int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset, int *zero_offset_negative_sign)\n> {\n> \tstruct tm tm;\n> \tint tm_gmt;\n> \ttimestamp_t dummy_timestamp;\n> \tint dummy_offset;\n> + int dummy_zero_offset_negative_sign;\n> + int negative_sign;\n> \tif (!timestamp)\n> \t\ttimestamp = &dummy_timestamp;\n> \tif (!offset)\n> \t\toffset = &dummy_offset;\n\nI see no need for the extra dummy_zero_offset_negative_sign variable.\nI can guess this mimics \"if (!offset) offset = &dummy_offset\" without\nmuch thought---a big difference is that after calling match_tz() to\nfill *offset, the code needs to obtain the value match_tz() parsed\nto decide if it needs to do the mktime() dance to guess the current\nzone offset, and also needs to read *offset to adjust *timestamp\nthe function returns.\n\nThe zero_offset_negative_sign pointer that specifies the location to\noptionally return a bit of info is *ONLY* used once at the very end\nof the function, so \n\n> + if (!zero_offset_negative_sign)\n> + zero_offset_negative_sign = &dummy_zero_offset_negative_sign;\n\nthere is absolutely no need for the dummy variable or this\nassignment, especially since the patch adds negative_sign variable\nthat always exists, and ...\n\n> \tmemset(&tm, 0, sizeof(tm));\n> \ttm.tm_year = -1;\n> @@ -848,6 +875,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n> \ttm.tm_sec = -1;\n> \t*offset = -1;\n> \ttm_gmt = 0;\n> + negative_sign = 0;\n> \n> \tif (*date == '@' &&\n> \t !match_object_header_date(date + 1, timestamp, offset))\n> @@ -864,9 +892,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n> \t\t\tmatch = match_alpha(date, &tm, offset);\n> \t\telse if (isdigit(c))\n> \t\t\tmatch = match_digit(date, &tm, offset, &tm_gmt);\n> -\t\telse if ((c == '-' || c == '+') && isdigit(date[1]))\n> +\t\telse if ((c == '-' || c == '+') && isdigit(date[1])) {\n> \t\t\tmatch = match_tz(date, offset);\n> -\n> + if (c == '-')\n> + negative_sign = 1;\n> + }\n\n... is usable.\n\n> \t\tif (!match) {\n> \t\t\t/* BAD CRAP */\n> \t\t\tmatch = 1;\n> @@ -895,6 +925,9 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n> \n> \tif (!tm_gmt)\n> \t\t*timestamp -= *offset * 60;\n> +\n> + *zero_offset_negative_sign = (!(*offset) && negative_sign);\n> +\n\nThe only change needed for this optional extra bit return is to\nmake sure that the assignment happens only when it was requested by\nthe caller, i.e.\n\n\tif (zero_offset_negative_sign)\n\t\t*zero_offset_negative_sign = ...\n\nHaving said all that, quite honestly, I do not know if this is going\nin the right direction. Specifically, I do not offhand see why we\nneed such a huge surgery to date.c just to touch datestamp() and\ndate_string().\n\nI may be totally off, partly due to lack of explanation of how the\npatch attempts to achieve its goal, but wouldn't it be just the\nmatter of touching the single callsite of datestamp() in ident.c, so\nthat after it gets git_default_date string filled, null out the last\n5 bytes in it with \"-0000\" if record_tz is off?\n\n> @@ -996,15 +1030,16 @@ void parse_date_format(const char *format, struct date_mode *mode)\n> void datestamp(struct strbuf *out)\n> {\n> \ttime_t now;\n> -\tint offset;\n> -\tstruct tm tm = { 0 };\n> +\tint offset = 0;\n> \n> -\ttime(&now);\n> + time(&now);\n> + if (record_time_zone) {\n> + struct tm tm = { 0 };\n> + offset = tm_to_time_t(localtime_r(&now, &tm)) - now;\n> + offset /= 60;\n> + }\n> \n> -\toffset = tm_to_time_t(localtime_r(&now, &tm)) - now;\n> -\toffset /= 60;\n> -\n> -\tdate_string(now, offset, out);\n> +\tdate_string(now, offset, !record_time_zone, out);\n> }\n> \n> /*\n> @@ -1330,7 +1365,7 @@ timestamp_t approxidate_relative(const char *date)\n> \tint offset;\n> \tint errors = 0;\n> \n> -\tif (!parse_date_basic(date, ×tamp, &offset))\n> +\tif (!parse_date_basic(date, ×tamp, &offset, NULL))\n> \t\treturn timestamp;\n> \n> \tget_time(&tv);\n> @@ -1346,7 +1381,7 @@ timestamp_t approxidate_careful(const char *date, int *error_ret)\n> \tif (!error_ret)\n> \t\terror_ret = &dummy;\n> \n> -\tif (!parse_date_basic(date, ×tamp, &offset)) {\n> +\tif (!parse_date_basic(date, ×tamp, &offset, NULL)) {\n> \t\t*error_ret = 0;\n> \t\treturn timestamp;\n> \t}\n> @@ -1371,3 +1406,4 @@ int date_overflows(timestamp_t t)\n> \tsys = t;\n> \treturn t != sys || (t < 1) != (sys < 1);\n> }\n> +\n"},{"id":"408036","messageId":"20201021050146.3001222-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqqk0vtki66.fsf@gitster.c.googlers.com","subject":"Re: [WIP v2 1/2] Adding a record-time-zone command option for commit","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-21T05:01:46Z","receivedAt":"2020-10-21T05:01:52Z","isPatch":false,"sender":{"key":"shengfa@google.com","avatar":null},"body":"Junio C Hamano writes:\n\n>> Many places in Git record the timezone of actor when a\n>> timestamp is recorded, including the commiter and author\n>> timestamps in a commit object and the tagger timestamp in a tag\n>> object. Some people however prefer not to share where they\n>> actually are.\n>>\n>> They _could_ just say \"export TZ=UTC\" and be done with it,\n>> but the method would not easily allow them to pretend to be\n>> in the UTC timezone only with Git, while revealing their true\n>> timezone to other activities (e.g. sending e-emails?).\n>>\n>> Introduce --[no-]record-time-zone for commit command, which can be\n>> specified to disallow Git to record time zone. Timezone will be\n>> recorded by default.\n>>\n>> Note that this is a WIP and the test is failing.\n>\n> And there is no outline of the design decision made so far, so it is\n> hard to give useful review comments.\n\nThanks for the comments and sorry for not describing the design.\nI will add it here.\n\nFirst, I would like to use a \"global\" variable to keep track of whether\nrecord-time-zone is set and default to true. Then in various places such\nas commit, pull, merge and rebase; we can add command option that can\nmodify this value.\n\nThen in datestamp in date.c, we can check this value; offset would be\ninitialized to 0 and only be set if record_time_zone is true. Additionally,\ndate_string from the same file would take an extra argument to indicate if\nwe want to use nagative sign for zero offset. Then the timestamp along with\nsign and 4 digits offset would be stored in \"git_default_date\" as buf\n\"1603255519 -0000\". I think of this as the \"encoding\" step.\n\nInitially, I thought this would be sufficient to show \"-0000\" in commit log\nmessage. However, I found that the show_date function is used for \"decoding\";\nconverting timestamp and tz to more readable format. Then I realize the\nfunction won't distinguish between +0 and -0 as it only takes in a tz as\nargument. As a result, I added the sign pointer as an argument; the reason for\npointer being there are many call sites for the show_date function but I am not sure\nif they all need to display in the new format(-0000). I used NULL to denote\nnot sure and just do whatever they were doing before. For show_ident_date in\npretty.c, I have extracted the sign in ident tz and pass it into show_date.\nThen added helper functions in date.c to print either %+05d or -%04d depending\non tz and sign pointer.\n\nIn fmt_ident(ident.c), we are calling parse_date which calls parse_date_basic\nand used the parameters it parsed to call date_string again, so I modified\nparse_date_basic to parse the sign as well.\n\n> It does not help that the patch is distracting by turning tabs to\n> spaces and breaking alingment X-<.\n\nI was assuming 1 HT is 8 spaces. Then after doing :set list in vim, I think\n1 HT is actually ctrl + I. Is this correct?\n\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index 2c7673f74e..059cc5fce7 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -884,7 +884,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)\n>> \t\t\tif (tz > 0)\n>> \t\t\t\ttz2 = -tz2;\n>> \n>> -\t\t\tfprintf(out, \"Date: %s\\n\", show_date(timestamp, tz2, DATE_MODE(RFC2822)));\n>> +\t\t\tfprintf(out, \"Date: %s\\n\", show_date(timestamp, tz2, NULL, DATE_MODE(RFC2822)));\n>\n> For example, it appears that tweaking \"show_date()\" API function\n> seems to be a central part of the design, as there are so many\n> instances like this change in the patch. If the proposed log\n> message mentioned, even briefly, what the extra parameter added to\n> the API function meant (especially what NULL means there) upfront,\n> then readers can coast the part that added NULL there, like these\n> ones, and concentrate on the parts of this patch that matter, which\n> presumably uses something more interesting than NULL instead.\n\nAdded above, the extra parameter is the decoded sign if not NULL.\n\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 1dfd799ec5..ee3ca2c7ac 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1547,7 +1547,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>> \t\t\t\tN_(\"ok to record an empty change\")),\n>> \t\tOPT_HIDDEN_BOOL(0, \"allow-empty-message\", &allow_empty_message,\n>> \t\t\t\tN_(\"ok to record a change with an empty message\")),\n>> -\n>> + OPT_BOOL(0, \"record-time-zone\", &record_time_zone, N_(\"record user timezone\")),\n>\n> Our code indents with HT; get used to the style early and your\n> patches won't distract reviewers as much, leading to more quality\n> reviews and suggestions.\n\nMy bad, I didn't realize I was doing it wrong all along, I was thinking that I was\nsometimes missing spaces.\nI use vim, is there any easy way to ensure that indent is with HT?\nI was using 4 tabs since each tab is 2 spaces. Should I type in ctrl + I\ninstead?\n\n> Likewise. The record_time_zone global variable seems to play a\n> crucial role in this change, but without preparing readers by\n> mentioning where it is defined, what normal/default value it takes,\n> and who potentially touches it, in the proposed log message, it\n> makes reading the change harder than necessary.\n> \n> A system-wide global like this is usually defined in environment.c,\n> by the way. Look for say trust_executable_bit and mimic where it \n> is defined and declared.\n\nWill move to environment.c.\n\n>> \t\tOPT_END()\n>> \t};\n>> \n>> @@ -1580,6 +1580,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>> \targc = parse_and_validate_options(argc, argv, builtin_commit_options,\n>> \t\t\t\t\t builtin_commit_usage,\n>> \t\t\t\t\t prefix, current_head, &s);\n>> +\n>> \tif (verbose == -1)\n>> \t\tverbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;\n>> \n>\n> Distraction.\n\nWill remove.\n\n>> +static int negative_zero(int tz, int *sign)\n>> +{\n>> + return !tz && sign && (*sign) == '-';\n>> +}\n>> +\n>> +static const char *tz_fmt(int tz, int *sign)\n>> +{\n>> + if (!negative_zero(tz, sign))\n>> + return \" %+05d\";\n>> + else\n>> + return \" -%04d\";\n>> +}\n>> +\n>> +\n>> +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)\n>> {\n>> \tstruct {\n>> \t\tunsigned int\tyear:1,\n>> @@ -277,10 +293,10 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm\n>> \t\tstrbuf_addf(buf, \" %d\", tm->tm_year + 1900);\n>> \n>> \tif (!hide.tz)\n>> -\t\tstrbuf_addf(buf, \" %+05d\", tz);\n>> + strbuf_addf(buf, tz_fmt(tz, sign), tz);\n>> }\n>> \n>> -const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n>> +const char *show_date(timestamp_t time, int tz, int *signp, const struct date_mode *mode)\n>\n> With its type, we can tell easily that sign is a pointer, so no need\n> for 'p' (we do not have modep, either, next door). More important\n> is if 'sign' is a good name that conveys what the parameter (which\n> is almost always NULL) means. Without any introductory text, it is\n> hard to tell and offer recommendations.\n\nThe reason I used signp instead of sign here is because there is a variable with\nname sign used in the function. Regarding \"good name\", maybe sign_hint is more appropriate.\n\n>> @@ -826,17 +849,21 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in\n>> \n>> /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822\n>> (i.e. English) day/month names, and it doesn't work correctly with %z. */\n>> -int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>> +int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset, int *zero_offset_negative_sign)\n>> {\n>> \tstruct tm tm;\n>> \tint tm_gmt;\n>> \ttimestamp_t dummy_timestamp;\n>> \tint dummy_offset;\n>> + int dummy_zero_offset_negative_sign;\n>> + int negative_sign;\n>> \tif (!timestamp)\n>> \t\ttimestamp = &dummy_timestamp;\n>> \tif (!offset)\n>> \t\toffset = &dummy_offset;\n>\n> I see no need for the extra dummy_zero_offset_negative_sign variable.\n> I can guess this mimics \"if (!offset) offset = &dummy_offset\" without\n> much thought---a big difference is that after calling match_tz() to\n> fill *offset, the code needs to obtain the value match_tz() parsed\n> to decide if it needs to do the mktime() dance to guess the current\n> zone offset, and also needs to read *offset to adjust *timestamp\n> the function returns.\n> \n> The zero_offset_negative_sign pointer that specifies the location to\n> optionally return a bit of info is *ONLY* used once at the very end\n> of the function, so \n>> + if (!zero_offset_negative_sign)\n>> + zero_offset_negative_sign = &dummy_zero_offset_negative_sign;\n>\n> there is absolutely no need for the dummy variable or this\n> assignment, especially since the patch adds negative_sign variable\n> that always exists, and ...\n>\n>> \tmemset(&tm, 0, sizeof(tm));\n>> \ttm.tm_year = -1;\n>> @@ -848,6 +875,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>> \ttm.tm_sec = -1;\n>> \t*offset = -1;\n>> \ttm_gmt = 0;\n>> + negative_sign = 0;\n>> \n>> \tif (*date == '@' &&\n>> \t !match_object_header_date(date + 1, timestamp, offset))\n>> @@ -864,9 +892,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>> \t\t\tmatch = match_alpha(date, &tm, offset);\n>> \t\telse if (isdigit(c))\n>> \t\t\tmatch = match_digit(date, &tm, offset, &tm_gmt);\n>> -\t\telse if ((c == '-' || c == '+') && isdigit(date[1]))\n>> +\t\telse if ((c == '-' || c == '+') && isdigit(date[1])) {\n>> \t\t\tmatch = match_tz(date, offset);\n>> -\n>> + if (c == '-')\n>> + negative_sign = 1;\n>> + }\n>\n>... is usable.\n\nGot it, thanks for pointing out.\n\n>> \t\tif (!match) {\n>> \t\t\t/* BAD CRAP */\n>> \t\t\tmatch = 1;\n>> @@ -895,6 +925,9 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>> \n>> \tif (!tm_gmt)\n>> \t\t*timestamp -= *offset * 60;\n>> +\n>> + *zero_offset_negative_sign = (!(*offset) && negative_sign);\n>> +\n>\n> The only change needed for this optional extra bit return is to\n> make sure that the assignment happens only when it was requested by\n> the caller, i.e.\n>\n> \tif (zero_offset_negative_sign)\n> \t\t*zero_offset_negative_sign = ...\n>\n> Having said all that, quite honestly, I do not know if this is going\n> in the right direction. Specifically, I do not offhand see why we\n> need such a huge surgery to date.c just to touch datestamp() and\n> date_string().\n> \n> I may be totally off, partly due to lack of explanation of how the\n> patch attempts to achieve its goal, but wouldn't it be just the\n> matter of touching the single callsite of datestamp() in ident.c, so\n> that after it gets git_default_date string filled, null out the last\n> 5 bytes in it with \"-0000\" if record_tz is off?\n>\n\nChanging parse_date_basic because it's called by parse_date in fmt_ident(ident.c).\n\n"},{"id":"408085","messageId":"xmqqzh4f76jr.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"20201021050146.3001222-1-shengfa@google.com","subject":"Re: [WIP v2 1/2] Adding a record-time-zone command option for commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-21T18:55:20Z","receivedAt":"2020-10-21T18:55:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shengfa Lin writes:\n\n> Thanks for the comments and sorry for not describing the design.\n> I will add it here.\n\nThanks. Please do not forget to add it to the updated patch, too.\nThat's where it matters most---you do not necessarily have to\nexplain things to _me_, but you should, to everybody who will read\n\"git log\" in the future in order to understand what we did and why.\n\n> First, I would like to use a \"global\" variable to keep track of whether\n> record-time-zone is set and default to true. Then in various places such\n> as commit, pull, merge and rebase; we can add command option that can\n> modify this value.\n>\n> Then in datestamp in date.c, we can check this value; offset would be\n> initialized to 0 and only be set if record_time_zone is true. Additionally,\n> date_string from the same file would take an extra argument to indicate if\n> we want to use nagative sign for zero offset. Then the timestamp along with\n> sign and 4 digits offset would be stored in \"git_default_date\" as buf\n> \"1603255519 -0000\". I think of this as the \"encoding\" step.\n\nYes, we could check it in datestamp(), but ... \n\n> Initially, I thought this would be sufficient to show \"-0000\" in commit log\n> message. However, I found that the show_date function is used for \"decoding\";\n> converting timestamp and tz to more readable format. Then I realize the\n> function won't distinguish between +0 and -0 as it only takes in a tz as\n> argument. As a result,...\n\n... I would have imagined that you do not have to deal with all\nthose complications if you don't hook this to such a low level of\nthe call graph. That is why I wondered:\n\n>> I may be totally off, ... but wouldn't it be just the\n>> matter of touching the single callsite of datestamp() in ident.c, so\n>> that after it gets git_default_date string filled, null out the last\n>> 5 bytes in it with \"-0000\" if record_tz is off?\n\nWithout any change to datestamp() you made in the patch, the call to\nthe function from ident.c may give us back a string that ends with\nthe integer that is the number of seconds since epoch, and sign plus\n4 digits, e.g. +0900 or -0800, that would reveal the true timezone.\nI would have thought that these five bytes can be replaced with\n-0000 under some condition (including \"the global is set\" which is a\nsign that the feature is being used, but not limited to that one---\nwe may need to make sure the call to ident_default_date() to fill\ngit_default_date.buf is done on behalf of the user to get a new\ntimestamp to record the user's activity, not doing something like\n\"git commit -C \"). I do not immediately see a\nreason why such a change near the surface level, which does not\ndisrupt the workings of the code at lower levels, would not work.\n\nThanks.\n\n\n\n"},{"id":"408172","messageId":"xmqqd01ans4h.fsf@gitster.c.googlers.com","threadId":"52389","inReplyTo":"xmqqzh4f76jr.fsf@gitster.c.googlers.com","subject":"Re: [WIP v2 1/2] Adding a record-time-zone command option for commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-22T16:27:10Z","receivedAt":"2020-10-22T16:27:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Yes, we could check it in datestamp(), but ... \n>\n>> Initially, I thought this would be sufficient to show \"-0000\" in commit log\n>> message. However, I found that the show_date function is used for \"decoding\";\n>> converting timestamp and tz to more readable format. Then I realize the\n>> function won't distinguish between +0 and -0 as it only takes in a tz as\n>> argument. As a result,...\n>\n> ... I would have imagined that you do not have to deal with all\n> those complications if you don't hook this to such a low level of\n> the call graph. That is why I wondered:\n> ...\n\nLet me answer some of my puzzlement myself; that is, I would have\nunderstood the change well if it were explained to me this way, and\nif that explanation matched what the patches did ;-)\n\nThe topic has two major parts.\n\nThe code that prepares the timestamp to be recorded for the current\nuser, who wants to record an anonymous timezone \"-0000\", is one (and\nthe easier) part. And this part could be done all inside\nident_default_date() without touching anything in date.c; when we\nneed to call datestamp(), we are getting the current time for the\ncurrent user, so we can mask the timezone.\n\nThe other part is that we need to read the timestamp from existing\nrecords, and if we choose to distinguish between timestamp in UTC\nand timestamp with anonymous timezone, we'd need to devise a way to\nencode the anonymous timezone differently. It is where the extra\nbit that says \"this bit does not usually mean anything but only when\nthe offset (which is a signed integer whose valid range is set to\nbetween -2400 to +2400 by date.c::match_tz()) is zero, and this bit\nis set, the zone is anonymous\" comes in.\n\n\tSide note. I suspect the damage to the callchain can be\n\tlimited much narrower if we didn't add this bit throughout\n\tthe API. What if we instead pick a number outside the valid\n\trange of offsets, say -10000, as a sentinel value and passed\n\tthat throughout the code when we want an anonymous zone?\n\n\tThe functions in the callchain that care about the timezone\n\tmust understand how anonymous zone is encoded anyway, so to\n\tthem it's a matter of using an int plus one bit or using an\n\tint that can have a special value. But other functions in\n\tthe callchain whose sole purpose (with respect to the\n\ttimezone information) is to pass it between their caller and\n\ttheir callee as an opaque piece of data, using just a single\n\tinteger is much less error prone---the patch does not have\n\tto touch them at all.\n\nThanks.\n"},{"id":"408381","messageId":"20201026041443.1362933-1-shengfa@google.com","threadId":"52389","inReplyTo":"xmqqd01ans4h.fsf@gitster.c.googlers.com","subject":"Re: [WIP v2 1/2] Adding a record-time-zone command option for commit","fromName":"Shengfa Lin","fromEmail":"shengfa@google.com","sentAt":"2020-10-26T04:14:43Z","receivedAt":"2020-10-26T04:14:49Z","isPatch":false,"sender":{"key":"shengfa@google.com","avatar":null},"body":">> Yes, we could check it in datestamp(), but ... \n>>\n>>> Initially, I thought this would be sufficient to show \"-0000\" in commit log\n>>> message. However, I found that the show_date function is used for \"decoding\";\n>>> converting timestamp and tz to more readable format. Then I realize the\n>>> function won't distinguish between +0 and -0 as it only takes in a tz as\n>>> argument. As a result,...\n>>\n>> ... I would have imagined that you do not have to deal with all\n>> those complications if you don't hook this to such a low level of\n>> the call graph. That is why I wondered:\n>> ...\n>\n> Let me answer some of my puzzlement myself; that is, I would have\n> understood the change well if it were explained to me this way, and\n> if that explanation matched what the patches did ;-)\n\nYes, I agree.\n\n> The topic has two major parts.\n> \n> The code that prepares the timestamp to be recorded for the current\n> user, who wants to record an anonymous timezone \"-0000\", is one (and\n> the easier) part. And this part could be done all inside\n> ident_default_date() without touching anything in date.c; when we\n> need to call datestamp(), we are getting the current time for the\n> current user, so we can mask the timezone.\n\nSo for this part, there is no need to modify datestamp in dates.c.\nWe could modify ident_default_date buf after datestamp to set the last\n5 bytes to \"-0000\" using strcpy.\n\n> The other part is that we need to read the timestamp from existing\n> records, and if we choose to distinguish between timestamp in UTC\n> and timestamp with anonymous timezone, we'd need to devise a way to\n> encode the anonymous timezone differently. It is where the extra\n> bit that says \"this bit does not usually mean anything but only when\n> the offset (which is a signed integer whose valid range is set to\n> between -2400 to +2400 by date.c::match_tz()) is zero, and this bit\n> is set, the zone is anonymous\" comes in.\n\nYes, that's correct.\n\n> \tSide note. I suspect the damage to the callchain can be\n> \tlimited much narrower if we didn't add this bit throughout\n> \tthe API. What if we instead pick a number outside the valid\n> \trange of offsets, say -10000, as a sentinel value and passed\n> \tthat throughout the code when we want an anonymous zone?\n\nGood idea.\n\n> \tThe functions in the callchain that care about the timezone\n> \tmust understand how anonymous zone is encoded anyway, so to\n> \tthem it's a matter of using an int plus one bit or using an\n> \tint that can have a special value. But other functions in\n> \tthe callchain whose sole purpose (with respect to the\n> \ttimezone information) is to pass it between their caller and\n> \ttheir callee as an opaque piece of data, using just a single\n> \tinteger is much less error prone---the patch does not have\n> \tto touch them at all.\n\nThat would be easier to follow and requires less changes as well.\n\n> Thanks.\n\nThanks for the clarification. Now I think we have a much better\nunderstanding. I will try to do a better job describing patches\nnext time.\n"}]}