Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect time displayed in some views due to timezone differences #811

Closed
wants to merge 1 commit into from

Conversation

PaulChanHK
Copy link
Contributor

@PaulChanHK PaulChanHK commented Apr 9, 2018

The issues

  1. if date-local is no, timezone information is missing, and time is displayed incorrectly.
    e.g. "2018-04-09 15:15 +0800" is displayed as "2018-04-09 15:15" while
    "2018-03-19 04:48 -0700" is displayed as "2018-03-19 04:48" while
  2. if date-custom is yes and %z is set in date-format, timezone is displayed incorrectly as "+0000"

With this change,

  1. if date-local is no, display timezone, with the format string "%Y-%m-%d %H:%M %z"
  2. if date-custom is yes and %z is not set in date-format, force to display local time
  3. handle "%z" in the format string, display actual timezone instead of "+0000"

The issues
1. if date-local is no, timezone information is missing, and time is displayed incorrectly.
   e.g. "2018-04-09 15:15 +0800" is displayed as "2018-04-09 15:15" while
        "2018-03-19 04:48 -0700" is displayed as "2018-03-19 04:48" while
2. if date-custom is yes and %z in custom date-format is displayed as "+0000"

With this change,
1. if date-local is no, display timezone as well, with the format string "%Y-%m-%d %H:%M %z"
2. if date-custom is yes and %z is not set in date-format, force to display local time
3. handle "%z" the format string, display actual timezone instead of "+0000"
@jonas
Copy link
Owner

jonas commented May 19, 2018

This breaks several tests which unfortunately didn't got reported because of a bug in the tools/travis.sh script.
Would you mind rebasing on the latest master?

@jonas jonas mentioned this pull request May 19, 2018
Copy link
Owner

@jonas jonas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests needs to be updated to match the new formatted date strings, but this looks really good.

}
else {
format = (date == DATE_CUSTOM && custom_format) ?
custom_format : (local ? "%Y-%m-%d %H:%M" : "%Y-%m-%d %H:%M %z");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My attempt from a while back is here: 9206a35

I agree that it makes sense to show the timezone offset for "non-local mode" since that's what Git does.

custom_format : (local ? "%Y-%m-%d %H:%M" : "%Y-%m-%d %H:%M %z");
fmt_tz = strstr(format, "%z") ? 1 : 0;
/* use local time if %z is not in format string */
if (local || (!fmt_tz)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return buf;
}

/* gmtime_r uses +0000 for %z, replace it with the actual timezone */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this in the spec or all platforms simply does this?
What do you think about replacing %z before passing the buffer to strftime?

tz_rp[3] = '0' + tmpv;
tz_rp[4] = '0' + (tz_min - tmpv * 10);
}
while ( (tz_str = strstr(tz_str, "+0000")) != NULL ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good with a check for buffer overflow.

tz_rp[4] = '0' + (tz_min - tmpv * 10);
}
while ( (tz_str = strstr(tz_str, "+0000")) != NULL ) {
memcpy(tz_str, tz_rp, 5);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like tz_rp is NUL terminated, so potentially this could return junk from a previous use of buf.

@jonas jonas closed this in 7486358 May 20, 2018
@jonas
Copy link
Owner

jonas commented May 20, 2018

Thanks a lot. I ended up fixing the tests and tweaking the formatting locally before merging.

rolandwalker pushed a commit to rolandwalker/tig that referenced this pull request May 21, 2018
The issues:

 1. timezone information is missing from non-local dates,
    e.g. "2018-04-09 15:15 +0800" is displayed as "2018-04-09 15:15"
         "2018-03-19 04:48 -0700" is displayed as "2018-03-19 04:48"
 2. custom dates are formatting %z as "+0000"

With this change:

 1. Show timezone information by default for non-local dates using the
    format string "%Y-%m-%d %H:%M %z"
 2. display the actual timezone when formatting "%z" instead of "+0000"

[ jf: Updated the tests to match the new date formatting and fixed
  mkdate to replace %z and %Z with the timezone rather than passing it
  to strftime and expecting "+0000", which is not always the case. ]

Closes jonas#811
@PaulChanHK PaulChanHK deleted the wrong_time_display branch October 31, 2020 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants