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

Issue3136 #3137

Merged
merged 1 commit into from Apr 13, 2016
Merged

Issue3136 #3137

merged 1 commit into from Apr 13, 2016

Conversation

bearbin
Copy link
Member

@bearbin bearbin commented Apr 10, 2016

This fixes the crash portion of #3136

I couldn't get the ports set on the command line to actually override those set in the ini file though, maybe someone else could give me a hand?

@bearbin bearbin force-pushed the issue3136 branch 2 times, most recently from 994acbe to 9c7dffa Compare April 11, 2016 10:52
@@ -371,7 +371,7 @@ static std::unique_ptr<cMemorySettingsRepository> ParseArguments(int argc, char
// Parse the comand line args:
TCLAP::CmdLine cmd("Cuberite");
TCLAP::ValueArg<int> slotsArg ("s", "max-players", "Maximum number of slots for the server to use, overrides setting in setting.ini", false, -1, "number", cmd);
TCLAP::MultiArg<int> portsArg ("p", "port", "The port number the server should listen to", false, "port", cmd);
TCLAP::MultiArg<int> portsArg ("p", "ports", "The port numbers the server should listen to", false, "port", cmd);

Choose a reason for hiding this comment

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

This should probably stay as --port as its a MultiArg - ie ./Cuberite --port 25565 --port 25566, would need something added to the description to indicate it can be set multiple times.

@bearbin
Copy link
Member Author

bearbin commented Apr 11, 2016

@johnmccabe I fixed your comments.

@tigerw
Copy link
Member

tigerw commented Apr 12, 2016

I couldn't get the ports set on the command line to actually override those set in the ini file though, maybe someone else could give me a hand?

It certainly looks like it should work: the default is overridesrepository; but if something is present in main but not in overrides, add it. If this analysis is correct then something's probably subtly wrong.

continue;
if (returnpair.first == mainpair.first)
{
found = true;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you can do the breakficiation here?

@tigerw
Copy link
Member

tigerw commented Apr 12, 2016

Any crash fix is a good fix. Merge while the sun still shines! 🌞

@bearbin
Copy link
Member Author

bearbin commented Apr 12, 2016

It certainly looks like it should work: the default is overridesrepository; but if something is present in main but not in overrides, add it. If this analysis is correct then something's probably subtly wrong.

I recommend you look at the old code and try and draw some sense from it. I have absolutely no idea what it was trying to do.

@bearbin
Copy link
Member Author

bearbin commented Apr 12, 2016

In any case, I responded to your comments.

@bearbin bearbin merged commit 8447e4e into master Apr 13, 2016
This was referenced Apr 14, 2016
@bearbin
Copy link
Member Author

bearbin commented Apr 14, 2016

@LogicParrot Could you review the changes so as to see what went wrong here? In particular the old code, which I couldn't understand at all (I just replaced it as it seemed to make no sense).

@SafwatHalaby
Copy link
Member

I did not review the code yet. This is what I know so far:

  • You modified OverridesSettingsRepository.cpp, and that made cRoot::LoadWorlds malfunction.
  • I think that your changes made identical INI keys collide.
    e.g. If we have such a config:
World=Hello1
World=Hello2

Only one of the worlds would be read (I do not recall which) and the other is completely ignored.

@bearbin
Copy link
Member Author

bearbin commented Apr 14, 2016

@LogicParrot If it was simply doubled INI values being ignored, then the fix is rather simple and I can push it back in a few mins if you want.

@SafwatHalaby
Copy link
Member

Sure. Please do not push it straight to master though. Let us review it, and let us try to understand what the old code does and see if the new code replicates the full functionality.

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

4 participants