Re: [RFC PATCH 1/1] hideTimezone: add a user.hideTimezone config
- From
- Shengfa Lin <shengfa@google.com>
- Date
- Oct 2, 2020, 06:02 UTC
- Message-ID
- <20201002060200.4073817-1-shengfa@google.com>
- In-Reply-To
- <xmqqpn62essa.fsf@gitster.c.googlers.com>
Thanks for the comments.
Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
>> 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.
Show 7 quoted lines
>> +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.
Show 11 quoted lines
>> 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?
Show 5 quoted lines
>> + 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?
Show 11 quoted lines
>> + 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.
Show 9 quoted lines
> 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.
Show 16 quoted lines
>> +' >> + >> +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.