Fix #6742: Only possible to build station next to competitor by using CTRL+click #6906
Conversation
|
I feel like the code would be cleaner if Also, "there are no more than" (is->are) |
|
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) |
|
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? |
|
I removed now the duplicated issue number from the PR message at least. |
|
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 |
|
And only the first commit should have the issue number? |
|
It doesn't really matter - the name of the PR itself should be enough |
Hemaolle
force-pushed
the
issue#6742
branch
4 times, most recently
from
6f68f4e to
1915172
Compare
|
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. |
There was a problem hiding this 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
Hemaolle
force-pushed
the
issue#6742
branch
from
1915172 to
ec46d13
Compare
|
Changed the parameter new parameter to |
|
Are the first two commits needed? |
|
@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 |
|
@LordAro would you mind reviewing this again. The previous review got discarded when I addressed michicc's comment. |
|
@nielsmh, done. While at it, I removed the check for the |
src/station_cmd.cpp
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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;"
There was a problem hiding this comment.
Good thinking, looks like that could be the case indeed. Fixed it.
…+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.
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