There are too many PRs

[Wuzzy]

The Minetest development procedure has a huge problem: There are too many open PRs (pull requests). We recently have passed the 150 PR mark. There are over 150 PRs simultanously open. And it seems the number of open PRs is going to stay above 150 for a long time.

There's a flood of incoming code from contributors like me, but the code lies there, around, unused for months or even years.
It's great we have so many contributors to Minetest, but I'm afraid a lot of contributions are essentially going to waste.

The number of open PRs has been steadily increasing over the years. Contributors are posting PRs faster than core devs can react to them. This is very bad. There are many potential contributors but only a small handful of people with merge rights. Therefore, all PRs have to go through a very tiny bottleneck.

There are also many PRs for which, frankly, I believe they have no snowball's chance in hell to ever get merged. That's not the problem, the problem is that such PRs are still kept open for some reason. Zombie PRs. Oldest open PRs are from 2016, yes, 2016, and they are kept open for some reason.

Another problem with letting PRs rot for so long is that it naturally becomes harder and harder to merge them as time progresses. The much-feared label “rebase needed” has effectively killed quite a few PRs. As a contributor, you need a VERY long breath, otherwise you have no chance. Many contributors just disappear before a PR is merge-ready. That's so frustrating! There are a lot of hard-working contributors whose work basically goes to waste, effectively.

Paramat keeps complaining that “dev time” is very limited, and that might be true. But it is also true that there are a LOT of people who actively want to contribute to Minetest. It's just that those people don't happen to be coredevs. So this might be huge missed opportunity for Minetest.

I think something needs to happen. The PR problem will continue to become worse and worse, with so many PRs. Contributors are given false hopes that their contribution has a chance of getting merged, but in reality, the chances are not very good (they compete with >100 other PRs).
I know that reviewer time is a VERY limited resource, but I like to discuss about whether there are methods to deal with PRs more effectively. This is important, precisely *because* time is very limited.

But I think, currently, the way how PRs are dealt with is extremely time-inefficient. I have identified a couple of problems:

• A LOT of time is wasted with talking about irrelevant details before considering the bigger picture. If a PR is unsound on principle, it's pointless to discuss details
• PRs are very rarely rejected. Instead, they are just kept alive indefinitely and are forgotten, but still technically open. Rejection of PRs needs to be done much more aggressively. PRs for which there is core dev consensus that it is not for MT, should be closed. Low-quality PRs for which the author is unwilling or unable to make the requested improvements should be closed. Ancient PRs for which the author has disappeared a long time ago should be closed (consider adding a label “for historic interest”).
• Coding style is analyzed before the overall soundness of the PR is analyzed. It's pointless to discuss coding style if it turns out a PR is garbage
• Code rot: The longer a PR lays around unmerged, the harder it becomes to merge it. Some cases are so bad, it's easier for the PR author to throw the PR away and start over …
• “Rebase needed”: An ugly side effect of code rot. Many contributors cop out when this happens. This label can be a death sentence to a PR. It's also very frightening. You are basically asking the PR author to risk their repo integrity.
• Ridiculous coding style rules: Honestly, I hate MT's coding style, it's just a waste of time to deal with it every time. The core devs enforce the coding style religiously, even if it makes the code uglier. OK, this is just my personal opinion, but it just annoys me so much! :D
• No real priotization of PRs. Most PRs are treated equally, and it's difficult to navigate the jungle.
• There are just too many open PRs in general. This means many good PRs just drown in all the noise.
• “Ping-pong”: Basically, core devs do a superficial review (coding style, etc.) and ignore the bigger picture, then disappear for good. It is implied the author must react. Nothing will happen until the PR author reacts, even if it's just about a typo. Then the game repeats, the core devs review another small thing until they bump in another minor problem, then immediately stop the review again, and disappear again.. I call it “ping-pong” because each party is throwing the ping-pong ball to the other side. I think ping-ponging is EXTREMELY time-efficient, because there's always a delay until the other party is able to react (due to timezones). And the PR author is forced to be active at multiple points in time to basically only do trivial fixes, just to hit the ball again! But it would be much more efficient if the PR author receives all complaints at once, so they can be all fixed at once.

I have a couple of suggestions to reorganize the dealing with PRs:

• Shut down off-topic discussions when they arise.
• Be more aggreesive, and faster with rejecting PR that don't go anywhere. Make actual decisions instead of keeping every PR open for all eternity. Don't delay a decison. If you can't come to a decision for months, close the PR.
• Have actual rules / a procedure for rejection so everything is more straight-forward. This also helps contributors and will prevent tears because nobody can't say they haven't been warned.
• Get rid of the stupid coding style rules. The 80 char line length is annoying af. IMO the only thing a coding style needs is indents, everything else is BS.
• Hold back your review before you can do it more in-depth. If you expect a reaction from the PR author, it's way more efficient when you give them all complaints at once. Avoid useless mini-reviews
• Start to priotize PRs. Sort by complexity. Use multiple priority levels if you must. Deal with high-impact high-complexity PRs first. Those are the PRs that will attract code rot first.
• If you don't have time for a proper (!) review, don't bother. It's of no help for the PR author if there are only superficial or incomplete reviews. Come back when you have time.
• Consider adding more reviewers.

[CalebJ]

There's a flood of incoming code from contributors like me, but the code lies there, around, unused for months or even years.
It's great we have so many contributors to Minetest, but I'm afraid a lot of contributions are essentially going to waste.

Exactly! We don't have enough core devs which can test the PRs. It delays good minetest features.

Another problem with letting PRs rot for so long is that it naturally becomes harder and harder to merge them as time progresses. The much-feared label “rebase needed” has effectively killed quite a few PRs. As a contributor, you need a VERY long breath, otherwise you have no chance. Many contributors just disappear before a PR is merge-ready. That's so frustrating! There are a lot of hard-working contributors whose work basically goes to waste, effectively.

Yes... that is very true. Several PRs I had been excited about have died this way and the people are just .. gone. :(

A LOT of time is wasted with talking about irrelevant details before considering the bigger picture. If a PR is unsound on principle, it's pointless to discuss details

This is spot-on. I constantly see, for example, people pointing out slight code flaws (improvements that can be made), and then after the fact a coredev decides not to merge it, after perhaps weeks of work improving the code. Terrible.

PRs are very rarely rejected. Instead, they are just kept alive indefinitely and are forgotten, but still technically open. Rejection of PRs needs to be done much more aggressively. PRs for which there is core dev consensus that it is not for MT, should be closed. Low-quality PRs for which the author is unwilling or unable to make the requested improvements should be closed. Ancient PRs for which the author has disappeared a long time ago should be closed (consider adding a label “for historic interest”).

Yeah.

“Ping-pong”: Basically, core devs do a superficial review (coding style, etc.) and ignore the bigger picture, then disappear for good. It is implied the author must react. Nothing will happen until the PR author reacts, even if it's just about a typo. Then the game repeats, the core devs review another small thing until they bump in another minor problem, then immediately stop the review again, and disappear again.. I call it “ping-pong” because each party is throwing the ping-pong ball to the other side. I think ping-ponging is EXTREMELY time-efficient, because there's always a delay until the other party is able to react (due to timezones).

A very inefficent system and it leads to a heap of good, quality PRs being overlooked, and other, bad PRs being left around.

Get rid of the stupid coding style rules. The 80 char line length is annoying af. IMO the only thing a coding style needs is indents, everything else is BS.

The coding style rules are really annoying. I feel like testing should be the bigger concern.

Consider adding more reviewers.

Maybe you should volunteer ;-)

[Lone_Wolf]

Code style is easy if you create your PR with that in mind, and from what I've seen (1 C++ PR IIRC and quite a lot of small->large lua ones) rebasing is also easy and low-risk if you know what you're doing.

Counting certain people's approvals as something you can merge by (even if they're not a core dev) might work

[Zughy]

I agree 100% with the problems listed above. If core devs are not enough, just do a list with things you'd like to focus on and just trash all the rest. It's respectful both for yourselves and for people who wouldn't have to wait forever. In two months I've been here, I still can't understand if:
- Minetest is an engine or not. You say it is, but it still lacks of some core features such as input handling (yes, I know there is a mod for that, but it shouldn't be up to a mod)
- Minetest Game will be removed or not. The game is horrible AND a bad Minecraft clone, yet core devs aren't sure about what to do even if a large part of the community would like to see it gone

The question should be very easy: is this feature useful for now or should I at least consider it because the community is kicking for it? Yes/No
If no, trash it

If you don't focus, you'll end scattering (and wasting) energies all around the place without accomplishing much

[Hume2]

I think, the reviewers have no time so they try to shoot down the pull-request by the first little issue they find. Code-style issues are easy to spot so it's an easy way to shoot down a PR.
I think, the most active contributors should be invited to the Minetest organisation. I think that Wuzzy should be invited at least. If you look here, it's a way smaller project than Minetest but they have 12 more members! Why? When a contributor is active, he is being given rights over time and can be even given the owner status. And after a time, he can pass the rights to more active contributors. From the original members, almost nobody is still active. And the project is still running.

[texmex]

Agreed. Thanks Wuzzy for addressing this huge issue.

Another argument I’ve seen in multiple PR discussions is about feature code maintenance. The argument goes that since the core developer group is that spread thin already, the possible merge of the PR in question still risk basically code rot from inside the code base because nobody can/wants to maintain it.

I suppose that’s more common with code on engine parts that’s unfamiliar to the devs themselves, such as shader code. This argument may have good reasons for being applied to the argument but at the same time: how to improve important parts nobody is familiar with unless by accepting complex but qualified code?

I think this speaks to a certain conservancy unbeneficial to project advancement (while at the same time fully understandable given resources, don’t get me wrong!) that maybe should be addressed if more people recognize it.

[rubenwardy]

Also see: Help us review and merge PRs

core devs enforce the coding style religiously, even if it makes the code uglier
--
Get rid of the stupid coding style rules. The 80 char line length is annoying af. IMO the only thing a coding style needs is indents, everything else is BS.

I don't. In the last few years, I've avoided doing code style reviews as much as possible. Code style should be done using automated tools, not reviews. I disagree with removing the code style, but the reviewer and the contributor shouldn't care about it. It would also be nicer if the code style was more consistent, and had the ability to be captured by the auto-formatter

Hold back your review before you can do it more in-depth. If you expect a reaction from the PR author, it's way more efficient when you give them all complaints at once. Avoid useless mini-reviews

I've been guilty of this, in future I'll try to leave them as drafts until I complete the review. The problem with this is that I don't always have time to do a full review, and the comments may become stale or never posted

Have actual rules / a procedure for rejection so everything is more straight-forward. This also helps contributors and will prevent tears because nobody can't say they haven't been warned.

Rejecting is incredibly difficult due to lack of defined scope, and generally provokes a negative response

Start to priotize PRs. Sort by complexity. Use multiple priority levels if you must. Deal with high-impact high-complexity PRs first. Those are the PRs that will attract code rot first.

GitHub makes it hard to do this, it would be nice if there was a priority system based on complexity and impact

[cuthbertdoublebarrel]

- Minetest Game will be removed or not. The game is horrible AND a bad Minecraft clone, yet core devs aren't sure about what to do even if a large part of the community would like to see it gone

i had no idea this was the case and assumed that it was the default game to customize via the mod selection
would like to read a more detailed discussion about this in a seperate thread .

[GreenXenith]

- Minetest is an engine or not. You say it is, but it still lacks of some core features such as input handling (yes, I know there is a mod for that, but it shouldn't be up to a mod)
- Minetest Game will be removed or not. The game is horrible AND a bad Minecraft clone, yet core devs aren't sure about what to do even if a large part of the community would like to see it gone

Offtopic:

Spoiler

Minetest is not lacking input handling? It lacks full keyboard mapping, but it is in no way lacking input handling.
Besides, lacking features doesn't make it not an engine. It would be an engine even if all it did was register blocks and let the player run around and place/break things.

And there is support from multiple core devs to strip the default status from MTG, so the likelihood of it happening is high.

[Zughy]

- Minetest is an engine or not. You say it is, but it still lacks of some core features such as input handling (yes, I know there is a mod for that, but it shouldn't be up to a mod)
- Minetest Game will be removed or not. The game is horrible AND a bad Minecraft clone, yet core devs aren't sure about what to do even if a large part of the community would like to see it gone

Offtopic:

Spoiler

Minetest is not lacking input handling? It lacks full keyboard mapping, but it is in no way lacking input handling.
Besides, lacking features doesn't make it not an engine. It would be an engine even if all it did was register blocks and let the player run around and place/break things.

And there is support from multiple core devs to strip the default status from MTG, so the likelihood of it happening is high.

Have a look at this

[GreenXenith]

<snip>

Have a look at this

Its a 63loc hook using the already-existing controls API. Any mod that needs more control handling could roll it themselves (probably in less loc, too). A lot of game engines don't provide hooks (I'm not saying putting it in the engine is a bad idea, I'm just saying we do have proper control support).

[Linuxdirk]

A LOT of time is wasted with talking about irrelevant details before considering the bigger picture. […] Coding style is analyzed before the overall soundness of the PR is analyzed. […] Basically, core devs do a superficial review (coding style, etc.) and ignore the bigger picture, then disappear for good. It is implied the author must react. Nothing will happen until the PR author reacts, even if it's just about a typo. Then the game repeats, the core devs review another small thing until they bump in another minor problem, then immediately stop the review again, and disappear again

This seems to be the largest issue here. PR creators cannot be happy with this reaction to their PRs so most of them disappear.

It makes much more sense to first review what the PR is for and then implement it if it does not cause bugs and then make it fit into the code style and stuff. Code styling should be done silently in the background by an autolinter anyways.

As soon as a rebase is needed something went wrong with the PR. Either it was ignored for too long or the PR creator made an unfinished PR and needs too much time to finish it.

And lets be honest: Every PR older than 3-6 months likely never will be implemented and should be closed. (Same with the nearly one thousand issues, but that’s another story.)

[Hume2]

I would summarise it like this: Instead of finding a way to shot the PR down, do your best to merge it. Just be more optimistic ;)

And trusting the community more (at least the most active contributors) will also help.

[Walker]

[...]Every PR older than 3-6 months likely never will be implemented and should be closed. (Same with the nearly one thousand issues, but that’s another story.)

So that some good-sounding requests do not go under, I have a list here of which PRs should be added again because they are too good to simply drop them:

Code: Select all

Digging and placing: Send nodes instead of mapblocks to undo prediction #7630
Add register_on_chatcommand to SSM and CSM #7862
Allow binding dig, place actions to keys; remove LMB/RMB hardcoding #7924
Move death formspec and respawn to server builtin #8014
Disable auto-setting server as favorite on connect; display "Add Favorite" button #8034
Add fatal error on missing dependency #8059
Rewrite camera modes; implement per-player camera modes #8409
Reimplement particles using the Irrlicht particle system and improve collision handling #8461
Give the online lua mainmenu also the client_list and mods #8691
Add mapgen settings in world creation dialog #9254
Add compass HUD element #9312
Allow FormSpec elements to be focused with `set_focus` #9353
Fix Media...0% on loading screen #9478

[jas]

So, a fork then.

[celeron55]

All valid points. However, they're more about culture and less about rules (except for a few). Everyone can take what they can from these.

But did you know? Anyone can become a core dev by suggesting themselves or their friend to me.

I hate headhunting and I don't have an HR department so you have to do it for me.

[Walker]

[...]But did you know? Anyone can become a core dev by suggesting themselves or their friend to me.[...]

- Wuzzy ?

i would vote him

[Wuzzy]

Would adding a new coredev to the team actually solve anything? As far I know, even core devs have to endure the nerve-racking time-consuming review process as anyone else, under the same rules as anyone else. Right? So I think the status of being a core dev will be nothing more than symbolic.

[Hume2]

Would adding a new coredev to the team actually solve anything? As far I know, even core devs have to endure the nerve-racking time-consuming review process as anyone else, under the same rules as anyone else. Right? So I think the status of being a core dev will be nothing more than symbolic.

I think, you are right in that you will need to follow the same rules as the other core devs.

However, nobody would prevent you from doing it properly. Please, correct me if I am wrong. The rules don't forbide the reviewers to make a proper review. The reviewers do partial reviews not because of the rules but because of other reasons like lack of time. Even if your only activity was making proper reviews, it would speed merging of the PRs a lot because the contributors could focus on all bugs at once and not only at one at a time.

Next thing is that you could close the PRs which are very unlikely to be merged or you can actually rebase them and help them to get merged.

Even though you complain a lot at the current rules, I think, there is a lot of things you could actually do to improve Minetest.

[Wuzzy]

I think it would be a good idea to have an actual procedure/rules regarding PR rejection, just something on which we agree on. Could be as simple as a list of no-nos.
I feel many core devs are too scared of rejecting PRs, I believe because they are afraid of pissing people off.

The benefit of writing down on some rejection rules is that we have something on which we agree. But more importantly: This will be something for contributors so they don't have to waste time working on things that have zero chance. This will prevent lots of tears and anger later on. And should hopefully make core devs less scared of rejecting things.

For example:
“We don't want PRs that integrate Boost/Microsoft Edge/NodeJS/<insert horrible bloatware here>.” ;-)

Next thing is that you could close the PRs which are very unlikely to be merged or you can actually rebase them and help them to get merged.

Oh, you said another thing: Rebase. We don't really have a written-down procedure on what exactly is wanted from the PR author when a rebase is needed. It is an unwritten rule currently that this MUST be done only by the PR author, and never the reviewer. It seems there are many unwritten rules regarding rebase, and I think I didn't understand this even today. So, best to write down what “rebase needed” actually means and what is expected from reviewers/PR authors when it happens.


To be clear: It's not just about adding random rules just for rules' sake. It's not even really about rules themselves. It's just that I think there is still a lot of confusion on how the review process actually has to work, many things are still unclear to outsiders. Simply writing down all the unwritten rules will be a big help. I think it's important so we can actually get things done.

[sfan5]

Would adding a new coredev to the team actually solve anything? As far I know, even core devs have to endure the nerve-racking time-consuming review process as anyone else, under the same rules as anyone else. Right? So I think the status of being a core dev will be nothing more than symbolic.

Coredevs only need one other dev to approve their PRs to be allowed to merge them. Also since coredevs are the ones who review PRs and whose opinion counts, more coredevs means more time to spend on each PR, making it more likely that any PR is "successful".
I don't know how you can arrive at the conclusion that coredev status is purely symbolic.

“We don't want PRs that integrate Boost/Microsoft Edge/NodeJS/<insert horrible bloatware here>.” ;-)

Refer to here.

It is an unwritten rule currently that this MUST be done only by the PR author, and never the reviewer.

Not really, reviewers usually just don't have the time to resolve merge conflicts in your code. The other reason is that the submitter knows their own code best and is thus in the best position to resolve any conflicts.

It seems there are many unwritten rules regarding rebase, and I think I didn't understand this even today. So, best to write down what “rebase needed” actually means and what is expected from reviewers/PR authors when it happens.

It's pretty simple really. All you need to do is make the following go away:

The means are relatively irrelevant though using git rebase is preferred over git merge.

[Hume2]

Good point, the rules should be written somewhere. However, a more effective way of suggesting to write down the rules might be this: Write down the rules you have discovered so far or you think that they make sense and then ask to add/remove/change some of them.

[Sokomine]

As soon as a rebase is needed something went wrong with the PR. Either it was ignored for too long or the PR creator made an unfinished PR and needs too much time to finish it.

I've watched the development of the PR regarding html-like formspecs a bit. That PR needed a lot of rebase. It was rather complex in itself, but that wasn't the main factor: Other devs/contributors worked at the same part of the code with slightly diffrent goals at the same time. It is not uncommon for mods with the same/similar theme/similar goal (e.g. villagers) beeing released here on the forum withhin a very short time span. It even happenes RL: Sometimes it's not so certain who developed/discovered a new technology first. Perhaps the time is just ripe for something. And that sadly makes rebase necessary without anyone taking too long - they're just working on the same parts of the code simultaneously.

It's pretty simple really. All you need to do is make the following go away:

,-)

[paramat]

> There are too many PRs

Yes =D

> Paramat keeps complaining that “dev time” is very limited

Well, 'complaining' is the wrong word to use, i am just stating the fact =)

> PRs for which there is core dev consensus that it is not for MT, should be closed. Low-quality PRs for which the author is unwilling or unable to make the requested improvements should be closed. Ancient PRs for which the author has disappeared a long time ago should be closed

All these altready happen, we close everything we possibly can, and after 6 months of no activity.

> “Rebase needed”: [...] You are basically asking the PR author to risk their repo integrity.

I seem to remember you claiming this before. There is no reason why rebasing should damage your repository, you must be doing it wrong.

> Ridiculous coding style rules: Honestly, I hate MT's coding style, it's just a waste of time to deal with it every time. The core devs enforce the coding style religiously

We have to have a codestyle, whether you like the chosen codestyle or not.

> No real priotization of PRs

There is, see the various labels and milestones.

> Have actual rules / a procedure for rejection

We do.

> Get rid of the stupid coding style rules. The 80 char line length is annoying af. IMO the only thing a coding style needs is indents, everything else is BS.

This is a ridiculous suggestion. Code would become unreadable and undevelopable.

Zughy,

MTG is not a Minecraft clone.
It is not a case of 'aren't sure about what to do' with MTG, with that being a negative thing as you imply. We are early in the process of discussing and considering whether to bundle it or not. All development is a process of discussion and consideration, this is normal and fine.
You have no idea what proportion of the community wants to see MTG 'gone'.
The core devs are very focussed, we are forced to focus and prioritise due to having little time.

Hume2,

> I think, the reviewers have no time so they try to shoot down the pull-request by the first little issue they find. Code-style issues are easy to spot so it's an easy way to shoot down a PR.

> I would summarise it like this: Instead of finding a way to shot the PR down, do your best to merge it.

That is a nasty and offensive accusation.

> Next thing is that you could close the PRs which are very unlikely to be merged

No, if Wuzzy was a core dev they would have to close PRs according to the agreed method that all core devs use.

[Linuxdirk]

Code would become unreadable and undevelopable.

This is why autolinters/beautifiers exist. Code style should be the least thing a tester should care about.