Shengfa Lin <shengfa@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
