Where available, use the commit time rather than commit id, for sort #3562
Conversation
pljones
force-pushed
the
feature/sort-commits-by-timestamp
branch
2 times, most recently
from
65d46c6 to
87ab540
Compare
There was a problem hiding this comment.
APP_VERSION passed around in the protocol messages, to retain the full string.
|
|
||
| // Regex for SemVer: major.minor.patch-suffix | ||
| QRegularExpression semVerRegex ( R"(^(\d+)\.(\d+)\.(\d+)-?(.*)$)" ); | ||
| QRegularExpression semVerRegex ( R"(^(\d+)\.(\d+)\.(\d+)-?(.*):?(.*)$)" ); |
There was a problem hiding this comment.
Capture seconds since epoch if present.
| .arg ( patch, 3, 10, QLatin1Char ( '0' ) ) | ||
| .arg ( x ) | ||
| .arg ( suffix ); | ||
| .arg ( tstamp.isEmpty() ? suffix : tstamp ); |
There was a problem hiding this comment.
Use seconds since epoch, rather than commit id, if present.
| if ( pCurListViewItem ) | ||
| { | ||
| pCurListViewItem->setText ( LVC_VERSION, strVersion ); | ||
| pCurListViewItem->setText ( LVC_VERSION, GetDisplayVersion ( strVersion ) ); |
There was a problem hiding this comment.
As this method gets passed the protocol version (APP_VERSION), it needs to call GetDisplayVersion to retain existing display behaviour.
(Also note this only gets called once each time the server is seen, not on every refresh.)
pljones
force-pushed
the
feature/sort-commits-by-timestamp
branch
from
87ab540 to
e6f645c
Compare
pljones
force-pushed
the
feature/sort-commits-by-timestamp
branch
from
e6f645c to
438a867
Compare
| double rRangeStop, | ||
| double& rValue ); | ||
|
|
||
| inline QString GetDisplayVersion ( QString str ) { return str.contains ( ':' ) ? str.mid ( 0, str.lastIndexOf ( ':' ) ) : str; } |
There was a problem hiding this comment.
GetDisplayVersion strips the seconds since epoch, where present.
str.mid ( 0, str.lastIndexOf ( ':' ) ) for Qt 5 compatibility.
| // version and application name (use version from qt prject file) | ||
| #undef VERSION | ||
| #define VERSION APP_VERSION | ||
| #define VERSION GetDisplayVersion ( APP_VERSION ) |
There was a problem hiding this comment.
GetDisplayVersion ( APP_VERSION ) used where APP_VERSION is currently referenced by this define, in existing code. APP_VERSION wasn't referenced anywhere directly except here before.
|
Example from build that displays version but hasn't had this update, where a server is running that has got this update: Example from a build that displays version and has this update, for the same server (compared with the above): |
|
Ack the review request, but I'm away this week, so won't be able to examine until next week. |
|
https://semver.org/#spec-item-10
...I think.... |
|
I might have more time for reviewing before Christmas. |
3 participants
Short description of changes
As discussed elsewhere, I'd like the
--showallserverssort to use the commit time rather than commit id, where it's available. This means changing theAPP_VERSIONto add the git timestamp to the end and then splitting that off for display purposes. The defineDISPLAYthus becomes a call to a method that does the trim.CHANGELOG: Use the commit time rather than commit id for sort
Context: Fixes an issue?
No.
Does this change need documentation? What needs to be documented and how?
When the docs for the sort on Version are written, they'd need to take this into account.
Status of this Pull Request
Tested locally.
What is missing until this pull request can be merged?
Nothing broke -- existing servers sort exactly the same way as before as they have no timestamp.
Checklist