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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 1, 2020, 02:44 UTC
Message-ID: <20201001024452.GA2930867@google.com>
In-Reply-To: <20200930232138.3656304-2-shengfa@google.com>

Hi,

Shengfa Lin wrote:

Show 5 quoted lines

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

There is more to discuss in this design space --- let's see where the patch leads us.

Show 10 quoted lines

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

To me, this seems less appealing than being able to set the time zone. By setting the time zone to match others in the same project, I would be able to blend in without sharing information about my travels. If I use the hideTimezone setting instead, then I may stick out as the only contributor using UTC.

UTC is the default timezone in various development environments so this is not a big deal, but it seems enough reason to prefer the fuller-control approach.

Show 12 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);
> +  int hide_timezone = 0;
> +  if (!git_config_get_bool("user.hideTimezone", &hide_timezone)  && hide_timezone)
> +    setenv("TZ", "UTC", 1);

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.

Looking at the implementation, I find fmt_ident in ident.c, which calls ident_default_date(), which calls date.c's datestamp, which uses localtime_r to get the timezone. localtime_r on all platforms I know of calls tzset, though apparently[*] it is not required to.

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.

So I'd be tempted to do something targetted like this:

-- 8< --

diff --git i/date.c w/date.c
index f9ea807b3a9..658ba1a9a45 100644
--- i/date.c
+++ w/date.c
@@ -5,6 +5,7 @@
  */
 
 #include "cache.h"
+#include "config.h"
 
 /*
  * This is like mktime, but without normalization of tm_wday and tm_yday.
@@ -998,6 +999,20 @@ void datestamp(struct strbuf *out)
 	time_t now;
 	int offset;
 	struct tm tm = { 0 };
+	const char *env_tz;
+	char *config_tz = NULL;
+	int restore_tz = 0;
+
+	if (!git_config_get_string("user.timezone", &config_tz)) {
+		env_tz = getenv("TZ");
+		if (env_tz)
+			env_tz = xstrdup(env_tz);
+		if (!env_tz || strcmp(env_tz, config_tz)) {
+			restore_tz = 1;
+			setenv("TZ", config_tz, 1);
+			tzset();
+		}
+	}
 
 	time(&now);
 
@@ -1005,6 +1020,14 @@ void datestamp(struct strbuf *out)
 	offset /= 60;
 
 	date_string(now, offset, out);
+
+	if (restore_tz) {
+		if (env_tz)
+			setenv("TZ", env_tz, 1);
+		else
+			unsetenv("TZ");
+	}
+	free(config_tz);
 }
 
 /*
-- >8 --

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.

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.

It's specific to Git, but a user might want to disable recording their
timezone in *all* collaboration tools (mail clients, newsreaders,
etc), not just Git.  So I would find it more compelling if there were
a common convention shared across tools for setting your exposed
timezone, just like the EMAIL envvar for setting your exposed email
address.

Thoughts?

Thanks,
Jonathan

[*] https://pubs.opengroup.org/onlinepubs/9699919799/functions/localtime.html