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

[Shengfa Lin <shengfa@google.com>]

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

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

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

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

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

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