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 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

Hemaolle commented Sep 16, 2018

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

Hemaolle commented Sep 16, 2018

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

Hemaolle commented Sep 16, 2018

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 Hemaolle:issue#6742 branch 4 times, most recently from 6f68f4e to 1915172 Sep 16, 2018
@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.

Copy link
Member

LordAro left a comment

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 Hemaolle force-pushed the Hemaolle:issue#6742 branch from 1915172 to ec46d13 Sep 16, 2018
@Hemaolle
Copy link
Contributor Author

Hemaolle commented Sep 16, 2018

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

Hemaolle commented Oct 21, 2018

@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

Hemaolle commented Oct 21, 2018

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

Copy link
Contributor

nielsmh left a comment

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.

@Hemaolle Hemaolle force-pushed the Hemaolle:issue#6742 branch from ec46d13 to b737573 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);

This comment has been minimized.

Copy link
@frosch123

frosch123 Oct 31, 2018

Member

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;"

This comment has been minimized.

Copy link
@Hemaolle

Hemaolle Oct 31, 2018

Author Contributor

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

Hopman and others added 2 commits Jun 20, 2018
…+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.
@Hemaolle Hemaolle force-pushed the Hemaolle:issue#6742 branch from b737573 to f27bcb7 Oct 31, 2018
@frosch123 frosch123 merged commit b3b8925 into OpenTTD:master Oct 31, 2018
6 checks passed
6 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
openttd/commit-checker The commit looks good
Details
openttd/linux-amd64-clang-3.8 The commit looks good
Details
openttd/linux-amd64-gcc-6 The commit looks good
Details
openttd/linux-i386-gcc-6 The commit looks good
Details
openttd/osx-10.9 The commit looks good
Details
@Hemaolle Hemaolle deleted the Hemaolle:issue#6742 branch Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.