git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [WIP v2 1/2] Adding a record-time-zone command option for commit

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 13, 2020, 20:03 UTC
Message-ID
<xmqqk0vtki66.fsf@gitster.c.googlers.com>
In-Reply-To
<20201013052851.373029-2-shengfa@google.com>
Shengfa Lin <shengfa@google.com> writes:
Show 16 quoted lines
> Many places in Git record the timezone of actor when a
> timestamp is recorded, including the commiter and author
> timestamps in a commit object and the tagger timestamp in a tag
> object. Some people however prefer not to share 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-emails?).
>
> Introduce --[no-]record-time-zone for commit command, which can be
> specified to disallow Git to record time zone. Timezone will be
> recorded by default.
>
> Note that this is a WIP and the test is failing.

And there is no outline of the design decision made so far, so it is hard to give useful review comments.

It does not help that the patch is distracting by turning tabs to spaces and breaking alingment X-<.

Show 10 quoted lines
> diff --git a/builtin/am.c b/builtin/am.c
> index 2c7673f74e..059cc5fce7 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -884,7 +884,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
>  			if (tz > 0)
>  				tz2 = -tz2;
>  
> -			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, DATE_MODE(RFC2822)));
> +			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, NULL, DATE_MODE(RFC2822)));

For example, it appears that tweaking "show_date()" API function seems to be a central part of the design, as there are so many instances like this change in the patch. If the proposed log message mentioned, even briefly, what the extra parameter added to the API function meant (especially what NULL means there) upfront, then readers can coast the part that added NULL there, like these ones, and concentrate on the parts of this patch that matter, which presumably uses something more interesting than NULL instead.

Show 10 quoted lines
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 1dfd799ec5..ee3ca2c7ac 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1547,7 +1547,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  				N_("ok to record an empty change")),
>  		OPT_HIDDEN_BOOL(0, "allow-empty-message", &allow_empty_message,
>  				N_("ok to record a change with an empty message")),
> -
> +                OPT_BOOL(0, "record-time-zone", &record_time_zone, N_("record user timezone")),

Our code indents with HT; get used to the style early and your patches won't distract reviewers as much, leading to more quality reviews and suggestions.

Likewise. The record_time_zone global variable seems to play a crucial role in this change, but without preparing readers by mentioning where it is defined, what normal/default value it takes, and who potentially touches it, in the proposed log message, it makes reading the change harder than necessary.

A system-wide global like this is usually defined in environment.c, by the way. Look for say trust_executable_bit and mimic where it is defined and declared.

Show 11 quoted lines
>  		OPT_END()
>  	};
>  
> @@ -1580,6 +1580,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>  					  builtin_commit_usage,
>  					  prefix, current_head, &s);
> +
>  	if (verbose == -1)
>  		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
>  
Distraction.
Show 28 quoted lines
> +static int negative_zero(int tz, int *sign)
> +{
> +        return !tz && sign && (*sign) == '-';
> +}
> +
> +static const char *tz_fmt(int tz, int *sign)
> +{
> +        if (!negative_zero(tz, sign))
> +                return " %+05d";
> +        else
> +                return " -%04d";
> +}
> +
> +
> +static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, int *sign, struct tm *human_tm, int human_tz, int local)
>  {
>  	struct {
>  		unsigned int	year:1,
> @@ -277,10 +293,10 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm
>  		strbuf_addf(buf, " %d", tm->tm_year + 1900);
>  
>  	if (!hide.tz)
> -		strbuf_addf(buf, " %+05d", tz);
> +                strbuf_addf(buf, tz_fmt(tz, sign), tz);
>  }
>  
> -const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
> +const char *show_date(timestamp_t time, int tz, int *signp, const struct date_mode *mode)

With its type, we can tell easily that sign is a pointer, so no need for 'p' (we do not have modep, either, next door). More important is if 'sign' is a good name that conveys what the parameter (which is almost always NULL) means. Without any introductory text, it is hard to tell and offer recommendations.

Show 17 quoted lines
> @@ -826,17 +849,21 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in
>  
>  /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822
>     (i.e. English) day/month names, and it doesn't work correctly with %z. */
> -int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
> +int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset, int *zero_offset_negative_sign)
>  {
>  	struct tm tm;
>  	int tm_gmt;
>  	timestamp_t dummy_timestamp;
>  	int dummy_offset;
> +        int dummy_zero_offset_negative_sign;
> +        int negative_sign;
>  	if (!timestamp)
>  		timestamp = &dummy_timestamp;
>  	if (!offset)
>  		offset = &dummy_offset;

I see no need for the extra dummy_zero_offset_negative_sign variable. I can guess this mimics "if (!offset) offset = &dummy_offset" without much thought---a big difference is that after calling match_tz() to fill *offset, the code needs to obtain the value match_tz() parsed to decide if it needs to do the mktime() dance to guess the current zone offset, and also needs to read *offset to adjust *timestamp the function returns.

The zero_offset_negative_sign pointer that specifies the location to optionally return a bit of info is *ONLY* used once at the very end of the function, so 

> +        if (!zero_offset_negative_sign)
> +                zero_offset_negative_sign = &dummy_zero_offset_negative_sign;

there is absolutely no need for the dummy variable or this assignment, especially since the patch adds negative_sign variable that always exists, and ...

Show 21 quoted lines
>  	memset(&tm, 0, sizeof(tm));
>  	tm.tm_year = -1;
> @@ -848,6 +875,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>  	tm.tm_sec = -1;
>  	*offset = -1;
>  	tm_gmt = 0;
> +        negative_sign = 0;
>  
>  	if (*date == '@' &&
>  	    !match_object_header_date(date + 1, timestamp, offset))
> @@ -864,9 +892,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>  			match = match_alpha(date, &tm, offset);
>  		else if (isdigit(c))
>  			match = match_digit(date, &tm, offset, &tm_gmt);
> -		else if ((c == '-' || c == '+') && isdigit(date[1]))
> +		else if ((c == '-' || c == '+') && isdigit(date[1])) {
>  			match = match_tz(date, offset);
> -
> +                        if (c == '-')
> +                                negative_sign = 1;
> +                }
... is usable.
Show 10 quoted lines
>  		if (!match) {
>  			/* BAD CRAP */
>  			match = 1;
> @@ -895,6 +925,9 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>  
>  	if (!tm_gmt)
>  		*timestamp -= *offset * 60;
> +
> +        *zero_offset_negative_sign = (!(*offset) && negative_sign);
> +

The only change needed for this optional extra bit return is to make sure that the assignment happens only when it was requested by the caller, i.e.

	if (zero_offset_negative_sign)
		*zero_offset_negative_sign = ...

Having said all that, quite honestly, I do not know if this is going in the right direction. Specifically, I do not offhand see why we need such a huge surgery to date.c just to touch datestamp() and date_string().

I may be totally off, partly due to lack of explanation of how the patch attempts to achieve its goal, but wouldn't it be just the matter of touching the single callsite of datestamp() in ident.c, so that after it gets git_default_date string filled, null out the last 5 bytes in it with "-0000" if record_tz is off?

Show 47 quoted lines
> @@ -996,15 +1030,16 @@ void parse_date_format(const char *format, struct date_mode *mode)
>  void datestamp(struct strbuf *out)
>  {
>  	time_t now;
> -	int offset;
> -	struct tm tm = { 0 };
> +	int offset = 0;
>  
> -	time(&now);
> +        time(&now);
> +        if (record_time_zone) {
> +          struct tm tm = { 0 };
> +          offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
> +          offset /= 60;
> +        }
>  
> -	offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
> -	offset /= 60;
> -
> -	date_string(now, offset, out);
> +	date_string(now, offset, !record_time_zone, out);
>  }
>  
>  /*
> @@ -1330,7 +1365,7 @@ timestamp_t approxidate_relative(const char *date)
>  	int offset;
>  	int errors = 0;
>  
> -	if (!parse_date_basic(date, &timestamp, &offset))
> +	if (!parse_date_basic(date, &timestamp, &offset, NULL))
>  		return timestamp;
>  
>  	get_time(&tv);
> @@ -1346,7 +1381,7 @@ timestamp_t approxidate_careful(const char *date, int *error_ret)
>  	if (!error_ret)
>  		error_ret = &dummy;
>  
> -	if (!parse_date_basic(date, &timestamp, &offset)) {
> +	if (!parse_date_basic(date, &timestamp, &offset, NULL)) {
>  		*error_ret = 0;
>  		return timestamp;
>  	}
> @@ -1371,3 +1406,4 @@ int date_overflows(timestamp_t t)
>  	sys = t;
>  	return t != sys || (t < 1) != (sys < 1);
>  }
> +
Previous: Shengfa LinNext: Shengfa Lin
Message 42 of 47 in “[ISSUE] Stop accessing, storing, and sharing the user's time zone”
  1. Nathaniel ManistaDec 5, 2019
  2. Junio C HamanoDec 5, 2019
  3. Randall S. BeckerDec 5, 2019
  4. Junio C HamanoDec 5, 2019
  5. Randall S. BeckerDec 5, 2019
  6. Santiago Torres AriasDec 5, 2019
  7. 0/1 adding user.hideTimezone for setting UTC timezoneShengfa Lin, Sep 30, 2020
  8. 1/1 hideTimezone: add a user.hideTimezone configShengfa Lin, Sep 30, 2020
  9. Junio C HamanoSep 30, 2020
  10. Junio C HamanoOct 1, 2020
  11. Shengfa LinOct 2, 2020
  12. Junio C HamanoOct 1, 2020
  13. Junio C HamanoOct 1, 2020
  14. Shengfa LinOct 2, 2020
  15. Shengfa LinOct 2, 2020
  16. Shengfa LinOct 2, 2020
  17. Shengfa LinOct 2, 2020
  18. Jonathan NiederOct 2, 2020
  19. Shengfa LinOct 2, 2020
  20. Junio C HamanoOct 3, 2020
  21. Junio C HamanoSep 30, 2020
  22. Shengfa LinOct 2, 2020
  23. Junio C HamanoOct 1, 2020
  24. Jonathan NiederOct 1, 2020
  25. Shengfa LinOct 2, 2020
  26. Junio C HamanoSep 30, 2020
  27. Junio C HamanoOct 1, 2020
  28. Jonathan NiederOct 1, 2020
  29. Junio C HamanoOct 1, 2020
  30. Junio C HamanoOct 8, 2020
  31. Shengfa LinOct 2, 2020
  32. Junio C HamanoOct 2, 2020
  33. Shengfa LinOct 3, 2020
  34. Junio C HamanoOct 3, 2020
  35. brian m. carlsonOct 3, 2020
  36. Junio C HamanoOct 3, 2020
  37. Shengfa LinOct 2, 2020
  38. Shengfa LinOct 2, 2020
  39. 0/2 experiment with commit option record-time-zoneShengfa Lin, Oct 13, 2020
  40. 2/2 Demonstrate failing and passing testsShengfa Lin, Oct 13, 2020
  41. 1/2 Adding a record-time-zone command option for commitShengfa Lin, Oct 13, 2020
  42. Junio C HamanoOct 13, 2020
  43. Shengfa LinOct 21, 2020
  44. Junio C HamanoOct 21, 2020
  45. Junio C HamanoOct 22, 2020
  46. Shengfa LinOct 26, 2020
  47. Junio C HamanoOct 9, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.