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 #7021: Better revision strings for network and gamelog #7121

Merged
merged 5 commits into from Feb 3, 2019

Conversation

@nielsmh
Copy link
Contributor

nielsmh commented Jan 27, 2019

Bug #7021 really covers two separate but related issues:

  1. The network revision field sent for network server status queries is too short to hold many git revision strings as implemented currently, this can lead to mismatched client/server versions looking identical.
  2. The gamelog revision string is limited to the same too short length, leading to gamelog not recognizing two versions as different and not writing entries about version changes.

This patchset does several things:

  • Include the full git revision hash in rev.cpp.
  • Change the revision string format to not use an M suffix for "modified", but a "g", "u" or "m" prefix to the hash for clean, unknown, modified status.
  • Use 10 hex digits of git hash for revision string.
  • Fix some additional inconsistencies between findversion.sh and determineversion.vbs.
  • Make gamelog revision string use a different length constant than network revision string, such that network revision string length can be increased.
  • Change the format used for gamelog revision strings: For release versions use the version name from _ottd_revision, and all other use a prefix of the git revision hash.
  • Increase the size of the network revision string to fill the rest of the potential size of the server query packet, i.e. from 15 to 33 bytes. This allows up to 32 bytes of _ottd_revision to be included, which should cover the master branch, and most other short branch names.
  • Ensure the 10 digit prefix of the git hash is always included in the network revision string.
  • Compare network revision strings by git hash prefix, if a full string comparison does not match.
  • Fix #7021 :)
src/gamelog.cpp Show resolved Hide resolved
@nielsmh nielsmh force-pushed the nielsmh:fix-networkrevision branch 2 times, most recently from ba9822c to d27af95 Jan 27, 2019
src/network/network.cpp Outdated Show resolved Hide resolved
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Jan 28, 2019

I have tested that I can start a local network game, and find it via LAN server query, and join the game and play :)

@nielsmh nielsmh force-pushed the nielsmh:fix-networkrevision branch from d27af95 to 87a6962 Jan 28, 2019
src/network/network_admin.cpp Outdated Show resolved Hide resolved
src/gamelog.cpp Show resolved Hide resolved
src/network/network.cpp Show resolved Hide resolved
@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Jan 30, 2019

Please don't include the git branch in the network revision. Unless you are at the head of a branch, you are in a detached head state instead and there is no branch. This means a checkout of a previous revision will not be compatible. Additionally, it is very well possible to have a different branch name locally from the remote.

It is possible to use "git reset --hard $revision" to get around this, but this seems to be very much bruteforce and not how git is meant to be used.

It's probably perfectly reasonable for network revision to just use the sha hash and a flag for modified.

src/network/core/tcp_http.cpp Outdated Show resolved Hide resolved
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Jan 30, 2019

@PeterN The idea behind including date and branch name in the network revision is that players may see it and want to use it as a guide for which version they need to play on that server. At least I think some way of getting an answer to that question is needed.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Jan 30, 2019

If you can find a way to include the branch name but not test it for compatibility then that would be okay. Either a separate field or with the revision encoded or formatted in such a way to make comparing just the sha hash possible.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Jan 30, 2019

Then there is also the million pound question, are "modified" revisions considered equal? :)
In theory you're into a situation like SQL NULL, unknown value which compared different to everything. In practice, it's probably some players agreeing to try the same custom modifications.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Jan 30, 2019

No, modified is not equal, so yes that should also be compared, of course. If branch information was always available, then yes it would be fine. But sadly it's not.

@nielsmh nielsmh force-pushed the nielsmh:fix-networkrevision branch from 87a6962 to d5bbabe Jan 30, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Jan 30, 2019

Changed so network revision is either version string (on release versions) or git hash only (with modified flag), and now using GetNetworkRevisionString() for all network protocols, also admin port and HTTP.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Jan 30, 2019

Hmm, "git branch --contains HEAD" will list all the local branches that contain the current checked out revision. If the list contains "master" then that could be used as the branch name, if not then I'm not sure how you'd pick the right branch if multiple are listed.

Having said that, if we ever switched to a full git model with branches being merged instead of rebased then this could end up being incorrect info anyway.

@nielsmh nielsmh force-pushed the nielsmh:fix-networkrevision branch from d5bbabe to eb551cc Feb 1, 2019
src/network/network.cpp Outdated Show resolved Hide resolved
@nielsmh nielsmh force-pushed the nielsmh:fix-networkrevision branch from eb551cc to 588f7e1 Feb 1, 2019
@nielsmh nielsmh added this to the 1.9.0 milestone Feb 1, 2019
@LordAro
LordAro approved these changes Feb 3, 2019
Copy link
Member

LordAro left a comment

Slightly tentative approve. Someone else can merge :)

@nielsmh nielsmh merged commit 5f8354f into OpenTTD:master Feb 3, 2019
1 check passed
1 check passed
OpenTTD CI Build #20190201.23 succeeded
Details
@nielsmh nielsmh deleted the nielsmh:fix-networkrevision branch Feb 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.