Mineclonia
/
Mineclonia
Archived
12
17
Fork 13
Code Issues 105 Pull Requests 19 Projects Releases Wiki Activity

CORE/_mcl_autogroup: Prevent server crash on digging unknown #85

Merged
cora merged 1 commits from fix-dig-unknown-crash into master 2021-06-17 23:51:56 +02:00
Conversation 17 Commits 1 Files Changed 1 +4
cora commented 2021-06-15 01:33:31 +02:00
Member
Copy Link
Problem

TRACKING ISSUE: #84

Solution

The can_harvest function of autogroup gets the node defintion from mt api (as i understand it) and in some conditions that can return nil.
The pr makes the can_harvest function return false (node is dug, nothing is dropped) in those cases.

Testing Steps
  1. Start Mineclone2, commit d5434bf008 on mt-clamity (https://github.com/ClamityAnarchy/minetest)
  2. grant yourself give with /grantme give
  3. give yourself logs and an axe /giveme mcl_core:tree -1, /giveme mcl_tools:axe_diamond
  4. place a few logs and debark them by rightclicking them while wielding the axe
  5. switch to mineclonia commit 3ee2ef0618
  6. (try to) break one of the now unknown tree logs, confirm the crash
  7. switch to this pr
  8. try breaking it again, confirm it does not crash
  9. mine some other normal nodes to confirm that still works.
##### Problem TRACKING ISSUE: #84 ##### Solution The can_harvest function of autogroup gets the node defintion from mt api (as i understand it) and in some conditions that can return nil. The pr makes the can_harvest function return false (node is dug, nothing is dropped) in those cases. ##### Testing Steps 1. Start Mineclone2, commit d5434bf008 on mt-clamity (https://github.com/ClamityAnarchy/minetest) 2. grant yourself give with /grantme give 3. give yourself logs and an axe /giveme mcl_core:tree -1, /giveme mcl_tools:axe_diamond 4. place a few logs and debark them by rightclicking them while wielding the axe 5. switch to mineclonia commit 3ee2ef0618 6. (try to) break one of the now unknown tree logs, confirm the crash 7. switch to this pr 8. try breaking it again, confirm it does not crash 9. mine some other normal nodes to confirm that still works.
erlehmann requested changes 2021-06-15 12:41:50 +02:00
erlehmann left a comment
Owner

Please replace the commit with one that does not change unrelated stuff to reduce future merge conflicts, then add an explanatory comment.

Please replace the commit with one that does not change unrelated stuff to reduce future merge conflicts, then add an explanatory comment.
mods/CORE/_mcl_autogroup/init.lua Outdated
@ -207,6 +207,10 @@ end
function mcl_autogroup.can_harvest(nodename, toolname)
local ndef = minetest.registered_nodes[nodename]
if not ndef then
erlehmann commented 2021-06-15 12:39:05 +02:00
Owner

Please add a comment here for context:

The comment should specify:

  • how to get to the branch and why this is bad (e.g. “breaking unknown nodes in clamity minetest at commit so and so causes a server crash, doing the same with minetest upstream at commit so and so does not”)
  • why this change actually fixes the bug (i.e. explain what returning false means)
Please add a comment here for context: The comment should specify: - how to get to the branch and why this is bad (e.g. “breaking unknown nodes in clamity minetest at commit so and so causes a server crash, doing the same with minetest upstream at commit so and so does not”) - why this change actually fixes the bug (i.e. explain what returning false means)
ryvnf commented 2021-06-15 15:29:29 +02:00
Owner

I disagree with these requested changes.

  • I do not think we should have references to specific Minetest servers in the code.
  • Doing not ndef is a very common way to check for unknown nodes and does not require a comment.
  • Making a detailed comment here would be inconsistent with all the other places similar checks are made.
  • What returning false means is documented in a few lines above in the function comment.

The most I think should be commented is something like -- Unknown nodes are not harvestable but I do not think a comment is necessary at all.

I think context and motivation about why a certain change was made should be explained in the commit message and not in the committed code. The only exception is if the code is hard to understand by itself so information about why it does things a certain way is necessary. I do not consider this to be one of those cases.

I think it would be better if @cora would do git commit --amend and add this type of context information to the commit message (while fixing the other things you pointed out).

I disagree with these requested changes. - I do not think we should have references to specific Minetest servers in the code. - Doing `not ndef` is a very common way to check for unknown nodes and does not require a comment. - Making a detailed comment here would be inconsistent with all the other places similar checks are made. - What returning `false` means is documented in a few lines above in the function comment. The most I think should be commented is something like `-- Unknown nodes are not harvestable` but I do not think a comment is necessary at all. I think context and motivation about why a certain change was made should be explained in the commit message and not in the committed code. The only exception is if the code is hard to understand by itself so information about why it does things a certain way is necessary. I do not consider this to be one of those cases. I think it would be better if @cora would do `git commit --amend` and add this type of context information to the commit message (while fixing the other things you pointed out).
erlehmann commented 2021-06-15 18:06:30 +02:00
Owner

I do not think we should have references to specific Minetest servers in the code.

In this case the crash happened because clamity runs a different version of minetest.
Without that knowledge it is impossible to reproduce the crash locally (@cora tried).

I think it would be better if @cora would do git commit --amend and add this type of context information to the commit message (while fixing the other things you pointed out).

I agree, you changed my mind about it.

> I do not think we should have references to specific Minetest servers in the code. In this case the crash happened because clamity runs a different version of minetest. Without that knowledge it is impossible to reproduce the crash locally (@cora tried). > I think it would be better if @cora would do git commit --amend and add this type of context information to the commit message (while fixing the other things you pointed out). I agree, you changed my mind about it.
ryvnf commented 2021-06-15 18:23:18 +02:00
Owner

In this case the crash happened because clamity runs a different version of minetest.
Without that knowledge it is impossible to reproduce the crash locally (@cora tried).

Yes, I see your point and agree that it is worth mentioning for future reference. Just disagreed about where we should put that information. :)

> In this case the crash happened because clamity runs a different version of minetest. > Without that knowledge it is impossible to reproduce the crash locally (@cora tried). Yes, I see your point and agree that it is worth mentioning for future reference. Just disagreed about where we should put that information. :)
mods/CORE/_mcl_autogroup/init.lua Outdated
@ -272,3 +276,3 @@
-- efficiency - The efficiency level the tool is enchanted with (default 0)
--
-- NOTE:
-- NOTE:
erlehmann commented 2021-06-15 12:33:37 +02:00
Owner

This has nothing do do with the bugfix. Please do not change unrelated stuff.

I think you could use “git commit --amend” and force push to replace the commit.

This has nothing do do with the bugfix. Please do not change unrelated stuff. I think you could use “git commit --amend” and force push to replace the commit.
mods/CORE/_mcl_autogroup/init.lua Outdated
@ -289,3 +293,3 @@
-- diggroup - The name of the diggroup the tool is used on
--
-- NOTE:
-- NOTE:
erlehmann commented 2021-06-15 12:34:04 +02:00
Owner

This has nothing do do with the bugfix. Please do not change unrelated stuff.

I think you could use “git commit --amend” and force push to replace the commit.

This has nothing do do with the bugfix. Please do not change unrelated stuff. I think you could use “git commit --amend” and force push to replace the commit.
erlehmann commented 2021-06-15 12:44:07 +02:00
Owner
Copy Link

Btw, “add nil check in can_harvest” is not a very good commit message in my opinion, as it states what changed on a purely mechanical level. Something like “Do not crash when players dig unknown nodes on Clamity minetest” would help someone wanting to understand why this was done a lot more.

Btw, “add nil check in can_harvest” is not a very good commit message in my opinion, as it states what changed on a purely mechanical level. Something like “Do not crash when players dig unknown nodes on Clamity minetest” would help someone wanting to understand *why* this was done a lot more.
cora commented 2021-06-15 13:02:08 +02:00
Author
Member
Copy Link

Well I thought that was a bit redundant since the PR is named that but yeah maybe you have a point

Well I thought that was a bit redundant since the PR is named that but yeah maybe you have a point
cora force-pushed fix-dig-unknown-crash from 651a477113 to 4c9de30545 2021-06-15 22:37:01 +02:00 Compare
cora requested review from erlehmann 2021-06-16 11:15:41 +02:00
erlehmann commented 2021-06-16 15:09:31 +02:00
Owner
Copy Link

If I understand the code correctly, the intent is to make unknown blocks drop nothing.

Which sounds good anyway, even without a crash.

Testing this now.

If I understand the code correctly, the intent is to make unknown blocks drop nothing. Which sounds good anyway, even without a crash. Testing this now.
erlehmann commented 2021-06-16 16:00:07 +02:00
Owner
Copy Link

I can confirm the crash happens as expected.

I can confirm the crash happens as expected.
erlehmann commented 2021-06-16 16:19:08 +02:00
Owner
Copy Link

I can confirm that the bugfix does prevent the crash.

Code looks okay. From where can_harvest() is used it seems to me that it is safe to assume that this does not prevent any legitimate tool use.

I can confirm that the bugfix does prevent the crash. Code looks okay. From where can_harvest() is used it seems to me that it is safe to assume that this does not prevent any legitimate tool use.
erlehmann commented 2021-06-16 16:41:39 +02:00
Owner
Copy Link
What I did
  • Start Mineclone2, commit d5434bf008 on mt-clamity (https://github.com/ClamityAnarchy/minetest)
  • grant yourself give with /grantme give
  • give yourself logs and an axe /giveme mcl_core:tree -1, /giveme mcl_tools:axe_diamond
  • place a few logs and debark them by rightclicking them while wielding the axe
  • switch to mineclonia commit 3ee2ef0618
  • (try to) break one of the now unknown tree logs, confirm the crash
  • switch to this pr
  • try breaking it again, confirm it does not crash
  • mine some other normal nodes to confirm that still works.
##### What I did - [x] Start Mineclone2, commit d5434bf008 on mt-clamity (https://github.com/ClamityAnarchy/minetest) - [x] grant yourself give with /grantme give - [x] give yourself logs and an axe /giveme mcl_core:tree -1, /giveme mcl_tools:axe_diamond - [x] place a few logs and debark them by rightclicking them while wielding the axe - [x] switch to mineclonia commit 3ee2ef0618 - [x] (try to) break one of the now unknown tree logs, confirm the crash - [x] switch to this pr - [x] try breaking it again, confirm it does not crash - [x] mine some other normal nodes to confirm that still works.
erlehmann approved these changes 2021-06-16 16:42:23 +02:00
erlehmann left a comment
Owner

I approve this, let's see what @ryvnf has to say about it.

I approve this, let's see what @ryvnf has to say about it.
erlehmann commented 2021-06-17 10:17:03 +02:00
Owner
Copy Link

Hey @ryvnf do you think this is okay now?

Hey @ryvnf do you think this is okay now?
erlehmann requested review from ryvnf 2021-06-17 10:17:40 +02:00
ryvnf commented 2021-06-17 13:34:23 +02:00
Owner
Copy Link

The code changes look good to me. I haven't tested them but I trust that both of you have.

The commit message is better but I would still change it slightly. This is because it breaks the "50 column" rule for the commit summary. I would have written it something like this:

Do not crash when players dig unknown nodes

Player digging unknown nodes caused crashes on the Clamity Minetest server. The problem is solved by inserting a nil check for unknown nodes.

The code changes look good to me. I haven't tested them but I trust that both of you have. The commit message is better but I would still change it slightly. This is because it breaks the ["50 column" rule](https://www.midori-global.com/blog/2018/04/02/git-50-72-rule) for the commit summary. I would have written it something like this: > Do not crash when players dig unknown nodes > > Player digging unknown nodes caused crashes on the Clamity Minetest server. The problem is solved by inserting a nil check for unknown nodes.
erlehmann commented 2021-06-17 13:57:18 +02:00
Owner
Copy Link

The code changes look good to me. I haven't tested them but I trust that both of you have.

We have, but the relevant information here is "I haven't tested" – regardless of reason.

IMO reviewers should never assume that anything works as advertised unless they tested it.

I think cleaning up the commit message before merging would be better, but is not necessary.

@cora @ryvnf anyone of you wants to amend the commit message before the merge?

> The code changes look good to me. I haven't tested them but I trust that both of you have. We have, but the relevant information here is "I haven't tested" – regardless of reason. IMO reviewers should *never* assume that anything works as advertised unless they tested it. I think cleaning up the commit message before merging would be better, but is not necessary. @cora @ryvnf anyone of you wants to amend the commit message before the merge?
ryvnf commented 2021-06-17 17:23:43 +02:00
Owner
Copy Link

We have, but the relevant information here is "I haven't tested" – regardless of reason.

IMO reviewers should never assume that anything works as advertised unless they tested it.

You asked for my opinion. I see no point in repeating the same tests that both you and @cora have already done. The PR does not need another approval to be merged. I think it must be fine to point out things which can be improved without doing a full review of the PR.

@cora @ryvnf anyone of you wants to amend the commit message before the merge?

I would prefer if @cora would amend the commit message and then merge it.

> We have, but the relevant information here is "I haven't tested" – regardless of reason. > > IMO reviewers should never assume that anything works as advertised unless they tested it. You asked for my opinion. I see no point in repeating the same tests that both you and @cora have already done. The PR does not need another approval to be merged. I think it must be fine to point out things which can be improved without doing a full review of the PR. > @cora @ryvnf anyone of you wants to amend the commit message before the merge? I would prefer if @cora would amend the commit message and then merge it.
cora commented 2021-06-17 18:07:17 +02:00
Author
Member
Copy Link

ammend the commit message and then merge it.

Again ? To what exactly should I change it now!?

>ammend the commit message and then merge it. Again ? To what exactly should I change it now!?
cora commented 2021-06-17 18:09:24 +02:00
Author
Member
Copy Link

Also I agree that 2 people having done the tests should be enough at least for trivial (and kinda critical) things like this

Also I agree that 2 people having done the tests should be enough at least for trivial (and kinda critical) things like this
👍 1
erlehmann commented 2021-06-17 18:22:01 +02:00
Owner
Copy Link

think it must be fine to point out things which can be improved without doing a full review of the PR.

yes, true

> think it must be fine to point out things which can be improved without doing a full review of the PR. yes, true
erlehmann commented 2021-06-17 18:36:05 +02:00
Owner
Copy Link

@ryvnf @cora orry if I might appear rude today, I am just very tired right now

@ryvnf @cora orry if I might appear rude today, I am just very tired right now
erlehmann commented 2021-06-17 18:39:29 +02:00
Owner
Copy Link

I think code review and playtesting being separate steps might motivate people to help more and also frustrate others way less. I will ask aroud, maybe we can get one or two playtesters to confirm bugs and fixes (basically QA). With the amoutn of people doing speedruns I am totally sure those people exist.

I think code review and playtesting being separate steps might motivate people to help more and also frustrate others way less. I will ask aroud, maybe we can get one or two playtesters to confirm bugs and fixes (basically QA). With the amoutn of people doing speedruns I am totally sure those people exist.
cora commented 2021-06-17 19:05:46 +02:00
Author
Member
Copy Link

Please tell me hte exact wording you want it to be

Please tell me hte exact wording you want it to be
ryvnf commented 2021-06-17 20:54:17 +02:00
Owner
Copy Link

Please tell me hte exact wording you want it to be

I posted my suggested wording here.

> Please tell me hte exact wording you want it to be I posted my suggested wording [here](https://git.minetest.land/Mineclonia/Mineclonia/issues/85#issuecomment-24694).
ryvnf refused to review 2021-06-17 20:54:55 +02:00
erlehmann force-pushed fix-dig-unknown-crash from b7cfb3185c to 79766bff23 2021-06-17 23:41:18 +02:00 Compare
cora merged commit 6c311d5528 into master 2021-06-17 23:51:56 +02:00
erlehmann deleted branch fix-dig-unknown-crash 2021-06-18 00:18:30 +02:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
erlehmann
Labels
Clear labels   
blocker

blocks release until resolved

  
bug

   
code quality

   
confirmed

   
critical

crashes, catastrophic bugs

  
discussion

   
high priority

   
incompatibility

incompatible with mcl2

  
incomplete feature

   
invalid

   
low priority

   
missing feauture

   
needs testing

   
packet spam

   
performance

   
project

meta-issues

  
regression

   
translations

   
unconfirmed
  
in review

   
ready for review
No Label
blocker
bug
code quality
confirmed
critical
discussion
high priority
incompatibility
incomplete feature
invalid
low priority
missing feauture
needs testing
packet spam
performance
project
regression
translations
unconfirmed
in review
ready for review
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Mineclonia/Mineclonia#85
No description provided.
Delete Branch "fix-dig-unknown-crash"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?