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 #6742: Only possible to build station next to competitor by using CTRL+click #6906

Merged
merged 2 commits into from Oct 31, 2018

Conversation

Hemaolle
Copy link
Contributor

@Hemaolle Hemaolle commented Sep 16, 2018

This builds on @Hopman's original pull request (#6831) for by adding a fix suggested in that pull request for the case where there are two competitor stations nearby.

Closes #6831

@LordAro
Copy link
Member

LordAro commented Sep 16, 2018

I feel like the code would be cleaner if GetOwnedStationAt took the company ID as a parameter, rather than _current_company being hard coded in

Also, "there are no more than" (is->are)

@LordAro
Copy link
Member

LordAro commented Sep 16, 2018

As for commit messages, should probably be a Remove followed by a Fix, given those are the first words following the update :p

(Only need the issue number referenced once as well)

@Hemaolle
Copy link
Contributor Author

Can you clarify the comment about the commit messages? Are you referring to a specific commit message out of the 3 here? Do you mean that the commits themselves should be in a different order? Or are you referring to the message on the pull request perhaps?

@Hemaolle
Copy link
Contributor Author

I removed now the duplicated issue number from the PR message at least.

@LordAro
Copy link
Member

LordAro commented Sep 16, 2018

The commit messages themselves - "Update" is rarely used, and when it's immediately followed by another keyword (Fix, Remove) you can probably do a minor amount of rewording to use that instead. The order is fine

@Hemaolle
Copy link
Contributor Author

And only the first commit should have the issue number?

@LordAro
Copy link
Member

LordAro commented Sep 16, 2018

It doesn't really matter - the name of the PR itself should be enough

@Hemaolle Hemaolle force-pushed the issue#6742 branch 4 times, most recently from 6f68f4e to 1915172 Compare September 16, 2018 09:13
@Hemaolle
Copy link
Contributor Author

Hemaolle commented Sep 16, 2018

Addressed everything else besides "there are no more than" (is->are). For that I read: https://english.stackexchange.com/questions/35389/there-is-are-more-than-one-whats-the-difference and it seems to me based on that that my version would be correct.

LordAro
LordAro previously approved these changes Sep 16, 2018
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Ah, the English language, always with the edge cases (I say this as a native speaker)

I wonder about using the CompanyByte type instead of OwnerByte, but since they're identical and I don't know the different uses of them (there's probably some existing (mis)use of them anyway), I won't worry about it

src/station_cmd.cpp Outdated Show resolved Hide resolved
@Hemaolle
Copy link
Contributor Author

Changed the parameter new parameter to GetStationAround still from OwnerByte to CompanyID. How does that look?

@frosch123
Copy link
Member

frosch123 commented Sep 20, 2018

Are the first two commits needed?
To me it looks like the third one fixes everything on its own.

@Hemaolle
Copy link
Contributor Author

@frosch123, you are probably right in that the third commit would fix the issue on its own. Nevertheless, I think the 2 other commits are good cleanup since BuildStationPart won't anymore be passed other companies' stations to attach to. For that reason it's kind of redundant to check in that method if the passed station is owned by the current company actually.

@Hemaolle
Copy link
Contributor Author

@LordAro would you mind reviewing this again. The previous review got discarded when I addressed michicc's comment.

Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

Seems to work correctly, however please merge the two code changes and do a force-push, so there's just one "Fix bug" and one "Remove unused string" commit.

nielsmh
nielsmh previously approved these changes Oct 30, 2018
@Hemaolle
Copy link
Contributor Author

Hemaolle commented Oct 30, 2018

@nielsmh, done.

While at it, I removed the check for the Station **st parameter in BuildStationPart() being owned by the _current_company and replaced with an assert in the beginning as we won't be calling it anymore with other companies' stations to attach to.

@@ -671,14 +671,12 @@ static void UpdateStationSignCoord(BaseStation *st)
*/
static CommandCost BuildStationPart(Station **st, DoCommandFlag flags, bool reuse, TileArea area, StationNaming name_class)
{
assert(*st == NULL || (*st)->owner == _current_company);
Copy link
Member

Choose a reason for hiding this comment

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

I think this assertion can be triggered by a malicious client to crash the server.
When distant-joining an existing station, the client sents the station id of the station to join.
At this point the id has been checked for existence, but not for ownership.

So, this must not be an "assert(...)" but a "if (..) return CMD_ERROR;"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thinking, looks like that could be the case indeed. Fixed it.

Hopman and others added 2 commits October 31, 2018 19:48
…+click

Fix by checking only for stations owned by the current company when
inspecting if there are multiple adjoining stations to the one being built.

When building next to 2 or more owned stations we don't know which
station should be extended. For other companies' stations that's not a
problem since our station won't merge with theirs anyway.

Calling to BuildStationPart should never have another company's station
as a parameter to attach to unless the client is malicious, so just returning
a generic error in that case.
@frosch123 frosch123 merged commit b3b8925 into OpenTTD:master Oct 31, 2018
@Hemaolle Hemaolle deleted the issue#6742 branch October 31, 2018 18:42
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.

None yet

6 participants