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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2020, 23:41 UTC
Message-ID: <xmqqpn62essa.fsf@gitster.c.googlers.com>
In-Reply-To: <20200930232138.3656304-2-shengfa@google.com>

Shengfa Lin <shengfa@google.com> writes:

Show 11 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").

> +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.

Show 9 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.

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 &&

Show 8 quoted lines

> +    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

Show 12 quoted lines

> --- /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'. ...

Show 6 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.

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.

Show 5 quoted lines

> +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.

Show 10 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.

Show 5 quoted lines

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

Thanks.