Junio C Hamano <gitster@xxxxxxxxx> 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).
