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
SLShengfa Lin <shengfa@google.com>
Date
Oct 21, 2020, 05:01 UTC
Message-ID
<20201021050146.3001222-1-shengfa@google.com>
In-Reply-To
<xmqqk0vtki66.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 19 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.

Thanks for the comments and sorry for not describing the design. I will add it here.

First, I would like to use a "global" variable to keep track of whether record-time-zone is set and default to true. Then in various places such as commit, pull, merge and rebase; we can add command option that can modify this value.

Then in datestamp in date.c, we can check this value; offset would be initialized to 0 and only be set if record_time_zone is true. Additionally, date_string from the same file would take an extra argument to indicate if we want to use nagative sign for zero offset. Then the timestamp along with sign and 4 digits offset would be stored in "git_default_date" as buf "1603255519 -0000". I think of this as the "encoding" step.

Initially, I thought this would be sufficient to show "-0000" in commit log message. However, I found that the show_date function is used for "decoding"; converting timestamp and tz to more readable format. Then I realize the function won't distinguish between +0 and -0 as it only takes in a tz as argument. As a result, I added the sign pointer as an argument; the reason for pointer being there are many call sites for the show_date function but I am not sure if they all need to display in the new format(-0000). I used NULL to denote not sure and just do whatever they were doing before. For show_ident_date in pretty.c, I have extracted the sign in ident tz and pass it into show_date. Then added helper functions in date.c to print either %+05d or -%04d depending on tz and sign pointer.

In fmt_ident(ident.c), we are calling parse_date which calls parse_date_basic and used the parameters it parsed to call date_string again, so I modified parse_date_basic to parse the sign as well.

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

I was assuming 1 HT is 8 spaces. Then after doing :set list in vim, I think 1 HT is actually ctrl + I. Is this correct?

Show 19 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.
Added above, the extra parameter is the decoded sign if not NULL.
Show 14 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.

My bad, I didn't realize I was doing it wrong all along, I was thinking that I was sometimes missing spaces. I use vim, is there any easy way to ensure that indent is with HT? I was using 4 tabs since each tab is 2 spaces. Should I type in ctrl + I instead?

Show 9 quoted lines
> 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.
Will move to environment.c.
Show 13 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.
Will remove.
Show 34 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.

The reason I used signp instead of sign here is because there is a variable with name sign used in the function. Regarding "good name", maybe sign_hint is more appropriate.

Show 59 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 ...
>
>>  	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.
Got it, thanks for pointing out.
Show 29 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?
>
Changing parse_date_basic because it's called by parse_date in fmt_ident(ident.c).
Previous: Junio C HamanoNext: Junio C Hamano
Message 43 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.