| Message ID | 20200930232138.3656304-2-shengfa@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | adding user.hideTimezone for setting UTC timezone | expand |
Shengfa Lin <shengfa@google.com> writes: > Users requested hiding location in the world from source control > trail. This is an implementation to read user.hideTimezone in > cmd_commit and set timezone to UTC if it's true. > > Added a brief explanation of the new field in Documentation > and added tests for true/false and reset-author > > Signed-off-by: Shengfa Lin <shengfa@google.com> > --- > Documentation/config/user.txt | 4 ++++ > builtin/commit.c | 5 +++++ There are many ways other than running "git commit" for a commit to be created, including but not limited to "git merge", "git rebase", "git pull" (with or without "--rebase"). > +user.hideTimezone:: > + Override TZ to UTC for Git commits to hide user's timezone in commit > + date One level of indentation in this codebase is a single HT. Unterminated sentence. A configuration _without_ command line option that overrides it is highly frowned upon. I do not see a reason why this must be such a configuration. If anything, a feature like this should first start as a command line option, and only after it proves its usefulness, a new configuration for convenience should be added. This only affects "git commit" and no other command (which I think is a mistake), yet is placed in the "user.*" namespace? That does not make any sense. I can sort-of understand if it were called say "commit.useUTC" though. > diff --git a/builtin/commit.c b/builtin/commit.c > index 42b964e0ca..fb1cbb8a39 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > status_format = STATUS_FORMAT_NONE; /* Ignore status.short */ > s.colopts = 0; > > + git_config(git_default_config, NULL); Declaration after statement is not tolerated in this codebase. One level of indentation in this codebase is a single HT. > + int hide_timezone = 0; Unnecessary initialization. > + if (!git_config_get_bool("user.hideTimezone", &hide_timezone) && hide_timezone) Overlong line. Double-SP before && > + setenv("TZ", "UTC", 1); > + > if (get_oid("HEAD", &oid)) > current_head = NULL; > else { > diff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh > new file mode 100755 > index 0000000000..41ed9c27da Let's not waste a test number for just a single test or two. Can't we roll this into > --- /dev/null > +++ b/t/t7527-commit-hide-timezone.sh > @@ -0,0 +1,37 @@ > +#!/bin/sh > + > +test_description='git-commit can override local timezone setting by reading user.hideTimezone from config' > + > +. ./test-lib.sh > + > +test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' ' > + git config user.hideTimezone false && > + echo test1 >> file && Style. Documentation/CodingGuidelines - Redirection operators should be written with space before, but no space after them. In other words, write 'echo test >"$file"' instead of 'echo test> $file' or 'echo test > $file'. ... > + git add file && > + # unset GIT_AUTHOR_DATE from test_tick > + unset GIT_AUTHOR_DATE && > + TZ=Europe/Istanbul git commit -m initial && > + git log -1 > output && > + grep "Date: .* +0300" output Do they not have DST over there, and is it guaranteed that they will never have one? Would we see this test fail about half of the year, when timezone rules change over there in some future year? After all, they changed in 2016 last time, which is fairly recent. This test attempts to establish the baseline, but I do not think it is a good idea. I think it is better *not* to unset GIT_AUTHOR_DATE like this. Instead, make sure it is set to some timestamp in some timezone that is not UTC, and the timezone of the resulting commit author date is in that timezone. But that must have already been done in basic tests on "git commit" that we honor the environment variable, no? Which means there is no need to add yet another extra baseline test here. > +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' ' > + git config user.hideTimezone true && > + git commit --amend --reset-author && > + git log -1 > output && > + grep "Date: .* +0000" output This one IS interesting, but keep the GIT_AUTHOR_DATE set and exported. As long as that is from a timezone different from UTC, we are testing what we want to test here. > +' > + > +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' ' > + git config user.hideTimezone true && > + echo test2 >> file && > + git add file && > + # TZ setting corresponding to -0600 or -0500 depending on DST > + # unset GIT_AUTHOR_DATE from test_tick > + unset GIT_AUTHOR_DATE && > + TZ=America/Chicago git commit -m test2 && This one is a borderline meh, compared to a test with explicit GIT_AUTHOR_DATE getting overridden by the configuration. It is not all that wrong, but I do not see a point in adding cycles to the already big testsuite. > + git log -1 > output && > + grep "Date: .* +0000" output > +' > + > +test_done Thanks.
Shengfa Lin <shengfa@google.com> writes: > Users requested hiding location in the world from source control > trail. This is an implementation to read user.hideTimezone in > cmd_commit and set timezone to UTC if it's true. Not a very good proposed log message, that sounds as if "it is what 'Users' requested, so it must be a worthwhile thing to do", which is not the line of thinking to go by. > > Added a brief explanation of the new field in Documentation > and added tests for true/false and reset-author > > Signed-off-by: Shengfa Lin <shengfa@google.com> > --- > Documentation/config/user.txt | 4 ++++ > builtin/commit.c | 5 +++++ > t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > create mode 100755 t/t7527-commit-hide-timezone.sh > > diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt > index 59aec7c3ae..bd6813f527 100644 > --- a/Documentation/config/user.txt > +++ b/Documentation/config/user.txt > @@ -36,3 +36,7 @@ user.signingKey:: > commit, you can override the default selection with this variable. > This option is passed unchanged to gpg's --local-user parameter, > so you may specify a key using any method that gpg supports. > + > +user.hideTimezone:: > + Override TZ to UTC for Git commits to hide user's timezone in commit > + date > diff --git a/builtin/commit.c b/builtin/commit.c > index 42b964e0ca..fb1cbb8a39 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > status_format = STATUS_FORMAT_NONE; /* Ignore status.short */ > s.colopts = 0; > > + git_config(git_default_config, NULL); > + int hide_timezone = 0; > + if (!git_config_get_bool("user.hideTimezone", &hide_timezone) && hide_timezone) > + setenv("TZ", "UTC", 1); > + > if (get_oid("HEAD", &oid)) > current_head = NULL; > else { > diff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh > new file mode 100755 > index 0000000000..41ed9c27da > --- /dev/null > +++ b/t/t7527-commit-hide-timezone.sh > @@ -0,0 +1,37 @@ > +#!/bin/sh > + > +test_description='git-commit can override local timezone setting by reading user.hideTimezone from config' > + > +. ./test-lib.sh > + > +test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' ' > + git config user.hideTimezone false && > + echo test1 >> file && > + git add file && > + # unset GIT_AUTHOR_DATE from test_tick > + unset GIT_AUTHOR_DATE && > + TZ=Europe/Istanbul git commit -m initial && > + git log -1 > output && > + grep "Date: .* +0300" output > +' > + > +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' ' > + git config user.hideTimezone true && > + git commit --amend --reset-author && > + git log -1 > output && > + grep "Date: .* +0000" output > +' > + > +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' ' > + git config user.hideTimezone true && > + echo test2 >> file && > + git add file && > + # TZ setting corresponding to -0600 or -0500 depending on DST > + # unset GIT_AUTHOR_DATE from test_tick > + unset GIT_AUTHOR_DATE && > + TZ=America/Chicago git commit -m test2 && > + git log -1 > output && > + grep "Date: .* +0000" output > +' > + > +test_done
Shengfa Lin <shengfa@google.com> writes: I won't repeat what I said in the other review message, but since I forgot to comment on the log message... > Users requested hiding location in the world from source control > trail. This is an implementation to read user.hideTimezone in > cmd_commit and set timezone to UTC if it's true. > > Added a brief explanation of the new field in Documentation > and added tests for true/false and reset-author Not a very good proposed log message, that sounds as if "it is what 'Users' requested, so it must be a worthwhile thing to do", which is not the line of thinking to go by. The convention we follow in the commit log messages is to: - first explain that the current system does not do X (in present tense, so we do NOT say "previously we did not do X"), then - explain why doing X would be a good thing, and finally - give an order to the codebase to start doing X. Perhaps Many places in Git record the timezone of the actor when a timestamp is recorded, including the committer and author timestamps in a commit object and the tagger timestamp in a tag object. Some people however prefer to "lie" about where they actually are. They _could_ just say "export TZ=UTC" and be done with it, but the method would not easily allow them to pretend to be in the UTC timezone only with Git, while revealing their true timezone to other activities (e.g. sending e-mail?). Introduce user.hideTimeZone configuration variable, which can be optionally set to 'true' to pretend to Git as if the user has exported environment variable TZ with the value UTC. Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> new file mode 100755 >> index 0000000000..41ed9c27da > > Let's not waste a test number for just a single test or two. Can't > we roll this into ... an existing test for "git commit"?
Junio C Hamano <gitster@pobox.com> writes: >> +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' ' >> + git config user.hideTimezone true && >> + git commit --amend --reset-author && >> + git log -1 > output && >> + grep "Date: .* +0000" output > > This one IS interesting, but keep the GIT_AUTHOR_DATE set and > exported. As long as that is from a timezone different from UTC, we > are testing what we want to test here. Note. Once GIT_AUTHOR_DATE and friends are set fully including timezone, we won't even read TZ because there is no need to. But you can do something along these lines: test_config user.hideTimeZone true && ( export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 && git commit ... && git show -s --format='%aI' >output && echo 2020-09-13T15:26:40+03:00 >expect && ... I think (haven't actually tested) "git commit --date=<datestring>" option is handled the same way, i.e. comparing these two would be a way not to touch the environment variable. TZ=UTC-09 git commit --date=@1600000000 ... && TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 ... && git show -s --format='%aI' HEAD~1 >output0 && git show -s --format='%aI' HEAD~0 >output1
Junio C Hamano <gitster@pobox.com> writes: > test_config user.hideTimeZone true && > ( > export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 && > git commit ... && > git show -s --format='%aI' >output && > echo 2020-09-13T15:26:40+03:00 >expect && Oops. The sample date and zone must be adjusted for the values exported above. I expect that we'd need to do ... echo 2020-09-13T12:26:40+00:00 >expect && test_cmp expect output
Hi, Shengfa Lin wrote: > Documentation/config/user.txt | 4 ++++ > builtin/commit.c | 5 +++++ > t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > create mode 100755 t/t7527-commit-hide-timezone.sh Thanks for this discussion starter. I'm interested to see what we end up with. To summarize the thread so far: Nathaniel Manista, who we can credit with a `Reported-by` line, reports that (mildly paraphrased): Authoring and sharing a commit by default exposes the user's time zone. "gt commit --date=YYYY-MM-DDThh:mm:ss+0000" suffices to put the author time in UTC (though not the commit time in UTC). But the user shouldn't have to pass a flag at all. Where the user is in the world is private information that git ought not to record and make available as part of the user's software engineering (make available to colleagues, in the case of proprietary development, and make available to the world, in the case of open source). Git should entirely stop accessing, recording, and sharing the user's time zone. On the other hand, various others have mentioned some beneficial aspects of recording the timezone --- for example, it makes it easier to make sense of the chronology in a program's development. What seems uncontroversial is that users should have control over the time zone used (as they already do, via the TZ environment variable). In response to the suggestion of a "[user] timeZone" setting, Nathaniel suggests That sounds like a great first step and like something that wouldn't ruffle anyone's feathers while proving the value of ignoring time zone information.
Thanks for the comments. Junio C Hamano <gitster@pobox.com> writes: >> Users requested hiding location in the world from source control >> trail. This is an implementation to read user.hideTimezone in >> cmd_commit and set timezone to UTC if it's true. >> >> Added a brief explanation of the new field in Documentation >> and added tests for true/false and reset-author >> >> Signed-off-by: Shengfa Lin <shengfa@google.com> >> --- >> Documentation/config/user.txt | 4 ++++ >> builtin/commit.c | 5 +++++ > > There are many ways other than running "git commit" for a commit to > be created, including but not limited to "git merge", "git rebase", > "git pull" (with or without "--rebase"). I overlooked on this. >> +user.hideTimezone:: >> + Override TZ to UTC for Git commits to hide user's timezone in commit >> + date > > One level of indentation in this codebase is a single HT. > > Unterminated sentence. What does HT stands for? I will change the indentation to 8 spaces. > This only affects "git commit" and no other command (which I think > is a mistake), yet is placed in the "user.*" namespace? That does > not make any sense. I can sort-of understand if it were called say > "commit.useUTC" though. I don't think this is a recommendation to implement commit.useUTC instead since it makes more sense to support more than just the commit command. >> diff --git a/builtin/commit.c b/builtin/commit.c >> index 42b964e0ca..fb1cbb8a39 100644 >> --- a/builtin/commit.c >> +++ b/builtin/commit.c >> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >> status_format = STATUS_FORMAT_NONE; /* Ignore status.short */ >> s.colopts = 0; >> >> + git_config(git_default_config, NULL); > >Declaration after statement is not tolerated in this codebase. If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler catches this as an error? >> + int hide_timezone = 0; > > Unnecessary initialization. >> + if (!git_config_get_bool("user.hideTimezone", &hide_timezone) && hide_timezone) > Is it unnecessary because I am checking the return value from git_config_get_bool so that the uninitialized value won't be used? >> + git add file && >> + # unset GIT_AUTHOR_DATE from test_tick >> + unset GIT_AUTHOR_DATE && >> + TZ=Europe/Istanbul git commit -m initial && >> + git log -1 > output && >> + grep "Date: .* +0300" output > > Do they not have DST over there, and is it guaranteed that they will > never have one? Would we see this test fail about half of the year, > when timezone rules change over there in some future year? After > all, they changed in 2016 last time, which is fairly recent. I looked up the timezones and find one without DST without considering the possibility of timezone rules change which would cause the test to fail. > This test attempts to establish the baseline, but I do not think it > is a good idea. I think it is better *not* to unset GIT_AUTHOR_DATE > like this. Instead, make sure it is set to some timestamp in some > timezone that is not UTC, and the timezone of the resulting commit > author date is in that timezone. But that must have already been > done in basic tests on "git commit" that we honor the environment > variable, no? Which means there is no need to add yet another extra > baseline test here. > I am not sure if this test has already been done in commit basic tests. Will remove this test. >> +' >> + >> +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' ' >> + git config user.hideTimezone true && >> + echo test2 >> file && >> + git add file && >> + # TZ setting corresponding to -0600 or -0500 depending on DST >> + # unset GIT_AUTHOR_DATE from test_tick >> + unset GIT_AUTHOR_DATE && >> + TZ=America/Chicago git commit -m test2 && > > This one is a borderline meh, compared to a test with explicit > GIT_AUTHOR_DATE getting overridden by the configuration. It is not > all that wrong, but I do not see a point in adding cycles to the > already big testsuite. > Will remove this one as well.
Junio C Hamano <gitster@pobox.com> writes: >>> new file mode 100755 >>> index 0000000000..41ed9c27da >> >> Let's not waste a test number for just a single test or two. Can't >> we roll this into > > ... an existing test for "git commit"? Will move into an existing test for "git commit".
Hi, Shengfa Lin wrote: > Thanks for the comments. >>> +user.hideTimezone:: >>> + Override TZ to UTC for Git commits to hide user's timezone in commit >>> + date >> >> One level of indentation in this codebase is a single HT. >> >> Unterminated sentence. > > What does HT stands for? I will change the indentation to 8 spaces. HT means "horizontal tab", like might be shown with "man ascii". Git uses tabs for indentation. This file is documentation instead of source so clang-format doesn't know about it, but I might as well mention anyway: if you run "make style", then clang-format will give some suggestions around formatting. The configuration for that is not yet perfect so you can take its suggestions with a grain of salt, but they should get you in the right direction. [...] >>> --- a/builtin/commit.c >>> +++ b/builtin/commit.c >>> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >>> status_format = STATUS_FORMAT_NONE; /* Ignore status.short */ >>> s.colopts = 0; >>> >>> + git_config(git_default_config, NULL); >> >> Declaration after statement is not tolerated in this codebase. > > If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler > catches this as an error? Yes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement. >>> + int hide_timezone = 0; >> >> Unnecessary initialization. >> >>> + if (!git_config_get_bool("user.hideTimezone", &hide_timezone) && hide_timezone) > > Is it unnecessary because I am checking the return value from git_config_get_bool so > that the uninitialized value won't be used? By leaving it uninitialized, you can help avoid the reader wondering whether there is some code path where the default value is used. [...] >> Instead, make sure it is set to some timestamp in some >> timezone that is not UTC, and the timezone of the resulting commit >> author date is in that timezone. But that must have already been >> done in basic tests on "git commit" that we honor the environment >> variable, no? Which means there is no need to add yet another extra >> baseline test here. > > I am not sure if this test has already been done in commit basic tests. > Will remove this test. Let's see: *checks with "git grep -e TZ -- t"*. Looks like t0006 tests various aspects of TZ handling pretty well and t1100 includes of test using TZ with commit-tree (good). Thanks and hope that helps, Jonathan
Junio C Hamano <gitster@pobox.com> writes: > > I think (haven't actually tested) "git commit --date=<datestring>" option > is handled the same way, i.e. comparing these two would be a way not > to touch the environment variable. > > TZ=UTC-09 git commit --date=@1600000000 ... && > TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 ... && > git show -s --format='%aI' HEAD~1 >output0 && > git show -s --format='%aI' HEAD~0 >output1 Like this? test_expect_fail '...' ' echo t1 >file && git add file && TZ=UTC-09 git commit --date=@1600000000 -m "t1" && echo t2 >>file && git add file && TZ=UTC-09 git -c user.hideTimeZone=true commit --date=@1600000000 -m "t2" && git show -s --format='%aI' HEAD~1 >output0 && git show -s --format='%aI' HEAD~0 >output1 && test_cmp output0 output1 ' I tested it.
Junio C Hamano <gitster@pobox.com> writes: >> test_config user.hideTimeZone true && >> ( >> export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 && >> git commit ... && >> git show -s --format='%aI' >output && >> echo 2020-09-13T15:26:40+03:00 >expect && > > Oops. The sample date and zone must be adjusted for the values > exported above. I expect that we'd need to do > > ... > echo 2020-09-13T12:26:40+00:00 >expect && > test_cmp expect output Tested the following: test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true' ' test_config user.hideTimezone true && ( echo test2 >file && git add file && export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 && git commit -m "test2" && git show -s --format='%aI' >output && echo 2020-09-13T12:26:40+00:00 >expect && test_cmp output expect ) '
Shengfa Lin <shengfa@google.com> writes: >>> test_config user.hideTimeZone true && >>> ( >>> export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 && >>> git commit ... && >>> git show -s --format='%aI' >output && >>> echo 2020-09-13T15:26:40+03:00 >expect && >> >> Oops. The sample date and zone must be adjusted for the values >> exported above. I expect that we'd need to do >> >> ... >> echo 2020-09-13T12:26:40+00:00 >expect && >> test_cmp expect output > > Tested the following: > > test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true' ' > test_config user.hideTimezone true && > ( > echo test2 >file && > git add file && > export GIT_AUTHOR_DATE=@1600000000 TZ=UTC-09 && > git commit -m "test2" && > git show -s --format='%aI' >output && > echo 2020-09-13T12:26:40+00:00 >expect && > test_cmp output expect > ) > ' I assume the opening and closing parentheses means the environment variables are only effective within like an environment scope. Please correct if wrong.
Junio C Hamano <gitster@pobox.com> writes: >> Users requested hiding location in the world from source control >> trail. This is an implementation to read user.hideTimezone in >> cmd_commit and set timezone to UTC if it's true. > > Not a very good proposed log message, that sounds as if "it is > what 'Users' requested, so it must be a worthwhile thing to do", > which is not the line of thinking to go by. That's a good point. Users requested does not necessarily equivalent to worthwhile thing to do. Thanks for pointing it out.
Jonathan Nieder <jrnieder@gmail.com> writes: > Like Junio mentioned, this affects "git commit" but not other commands > that record the current date with the local timezone. > > The fundamental tool to exercise that machinery is > > $ git var GIT_AUTHOR_IDENT > Jonathan Nieder <jrnieder@gmail.com> 1601517809 -0700 > > so I suppose I'd be interested in seeing that exercised in tests. Next patch should include a test for git var. > [...] > > The unfortunate thing about these APIs is that there's no way to pass > in a timezone from a string instead of from the environment. This > means that passing through the environment as above is the only > reasonable way to do it, but that would have the unfortunate result > of changing the output of commands like "git log --date=local" that > are about writing dates to the terminal instead of storing them. > The TZ should apply for write but not read. > [...] > Looking over callers, who would this affect? There are three callers: > > fast-import.c::parse_ident: > Used to handle ident string "now". That seems in keeping with > the intent here, and fast-import does respect some other > configuration though only affecting storage. Seems fine.sensible. > > ident.c::ident_default_date: > Used to produce author and committer timestamps and timestamps for > reflog entries. That's the goal; good. > > send-pack.c::generate_push_cert: > Used for the timestamp sent to the server in a signed push > certificate. Also good. > Thanks for the analysis here. I was wondering whether the TZ should affect these callers. > So I think this does the right thing, plus it retains the > user-friendly feature of being able to *display* timestamps in their > local timezone. > > Now let's talk through the downsides: > > It's complex. The performance isn't likely to be bad when > user.timezone is not set, which is nice, but it still is messier than > I'd like to see. > +1.
Jonathan Nieder <jrnieder@gmail.com> writes: >>> [...] >> >> What does HT stands for? I will change the indentation to 8 spaces. > > HT means "horizontal tab", like might be shown with "man ascii". > > Git uses tabs for indentation. This file is documentation instead of > source so clang-format doesn't know about it, but I might as well > mention anyway: if you run "make style", then clang-format will give > some suggestions around formatting. The configuration for that is not > yet perfect so you can take its suggestions with a grain of salt, but > they should get you in the right direction. > Got it, thanks! >[...] >>>> --- a/builtin/commit.c >>>> +++ b/builtin/commit.c >>>> @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >>>> status_format = STATUS_FORMAT_NONE; /* Ignore status.short */ >>>> s.colopts = 0; >>>> >>>> + git_config(git_default_config, NULL); >>> >>> Declaration after statement is not tolerated in this codebase. >> >> If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler >> catches this as an error? > > Yes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement. > Got the error from int hide_timezone = 0; but not git_config(git_default_config, NULL);. >>>> + int hide_timezone = 0; >>> >>> Unnecessary initialization. >>> >>>> + if (!git_config_get_bool("user.hideTimezone", &hide_timezone) && hide_timezone) >> >> Is it unnecessary because I am checking the return value from git_config_get_bool so >> that the uninitialized value won't be used? > > By leaving it uninitialized, you can help avoid the reader wondering > whether there is some code path where the default value is used. > I see. >[...] >>> Instead, make sure it is set to some timestamp in some >>> timezone that is not UTC, and the timezone of the resulting commit >>> author date is in that timezone. But that must have already been >>> done in basic tests on "git commit" that we honor the environment >>> variable, no? Which means there is no need to add yet another extra >>> baseline test here. >> >> I am not sure if this test has already been done in commit basic tests. >> Will remove this test. > > Let's see: *checks with "git grep -e TZ -- t"*. > > Looks like t0006 tests various aspects of TZ handling pretty well and > t1100 includes of test using TZ with commit-tree (good). > > Thanks and hope that helps, > Jonathan Thanks! That helps a lot.
Shengfa Lin <shengfa@google.com> writes: >>>>> + git_config(git_default_config, NULL); >>>> >>>> Declaration after statement is not tolerated in this codebase. >>> >>> If I use the DEVELOPER=1 flag in config.mak and call make again, would the compiler >>> catches this as an error? >> >> Yes, DEVELOPER_CFLAGS includes -Wdeclaration-after-statement. > > Got the error from int hide_timezone = 0; but not > git_config(git_default_config, NULL);. > ... >>>>> + int hide_timezone = 0; That is exactly expected. The problematic sequence was ... s.colopts = 0; git_config(git_default_config, NULL); int hide_timezone = 0; The assignment of 0 to s.colopts, which existed before your patch, was already a statement. So is a new call to git_config() function you added. As the existing "s.colopts = 0" were in a place where a statement can legally appear, the place you added git_config() call is also where a statement can legally appear. There is nothing for the compiler to complain about. The declaration of hide_timezone added _after_ that statement is a different story. Because the -Wdeclaration-after-statement is about starting a function (or "a block" in general) with all the necessary declarations before we write a single statement, and also forbidding us from adding other declarations after we write a statement. Since there are bunch of statements in the same block already, including the assignment to s.colopts and a call to git_config(), we cannot add a declaration after that. Thanks.
diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt index 59aec7c3ae..bd6813f527 100644 --- a/Documentation/config/user.txt +++ b/Documentation/config/user.txt @@ -36,3 +36,7 @@ user.signingKey:: commit, you can override the default selection with this variable. This option is passed unchanged to gpg's --local-user parameter, so you may specify a key using any method that gpg supports. + +user.hideTimezone:: + Override TZ to UTC for Git commits to hide user's timezone in commit + date diff --git a/builtin/commit.c b/builtin/commit.c index 42b964e0ca..fb1cbb8a39 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1569,6 +1569,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) status_format = STATUS_FORMAT_NONE; /* Ignore status.short */ s.colopts = 0; + git_config(git_default_config, NULL); + int hide_timezone = 0; + if (!git_config_get_bool("user.hideTimezone", &hide_timezone) && hide_timezone) + setenv("TZ", "UTC", 1); + if (get_oid("HEAD", &oid)) current_head = NULL; else { diff --git a/t/t7527-commit-hide-timezone.sh b/t/t7527-commit-hide-timezone.sh new file mode 100755 index 0000000000..41ed9c27da --- /dev/null +++ b/t/t7527-commit-hide-timezone.sh @@ -0,0 +1,37 @@ +#!/bin/sh + +test_description='git-commit can override local timezone setting by reading user.hideTimezone from config' + +. ./test-lib.sh + +test_expect_success 'commit date shows timezone offset +0300 when user.hideTimezone is false' ' + git config user.hideTimezone false && + echo test1 >> file && + git add file && + # unset GIT_AUTHOR_DATE from test_tick + unset GIT_AUTHOR_DATE && + TZ=Europe/Istanbul git commit -m initial && + git log -1 > output && + grep "Date: .* +0300" output +' + +test_expect_success 'commit date shows timezone offset +0000 when user.hideTimezone is true and reset' ' + git config user.hideTimezone true && + git commit --amend --reset-author && + git log -1 > output && + grep "Date: .* +0000" output +' + +test_expect_success 'commit date shows timezone offset +0000 even TZ setting says otherwise' ' + git config user.hideTimezone true && + echo test2 >> file && + git add file && + # TZ setting corresponding to -0600 or -0500 depending on DST + # unset GIT_AUTHOR_DATE from test_tick + unset GIT_AUTHOR_DATE && + TZ=America/Chicago git commit -m test2 && + git log -1 > output && + grep "Date: .* +0000" output +' + +test_done
Users requested hiding location in the world from source control trail. This is an implementation to read user.hideTimezone in cmd_commit and set timezone to UTC if it's true. Added a brief explanation of the new field in Documentation and added tests for true/false and reset-author Signed-off-by: Shengfa Lin <shengfa@google.com> --- Documentation/config/user.txt | 4 ++++ builtin/commit.c | 5 +++++ t/t7527-commit-hide-timezone.sh | 37 +++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100755 t/t7527-commit-hide-timezone.sh