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
Conversation
ba9822c
to
d27af95
Compare
I have tested that I can start a local network game, and find it via LAN server query, and join the game and play :) |
d27af95
to
87a6962
Compare
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. |
@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. |
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. |
Then there is also the million pound question, are "modified" revisions considered equal? :) |
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. |
87a6962
to
d5bbabe
Compare
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. |
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. |
d5bbabe
to
eb551cc
Compare
eb551cc
to
588f7e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly tentative approve. Someone else can merge :)
Bug #7021 really covers two separate but related issues:
This patchset does several things:
rev.cpp
.findversion.sh
anddetermineversion.vbs
._ottd_revision
, and all other use a prefix of the git revision hash._ottd_revision
to be included, which should cover the master branch, and most other short branch names.