There are too many PRs

[paramat]

It seems Wuzzy is suggesting no codestyle at all, apart from indentation, and is not suggesting autolinting. This is what i am objecting to.

[Linuxdirk]

It seems Wuzzy is suggesting no codestyle at all, apart from indentation, and is not suggesting autolinting. This is what i am objecting to.

Yes, this is nonsense, of course. But codestyle should not be part of reviewing a PR.

[Hume2]

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

To be honest, how many contributors know what is rebase and how to do it right?

> Have actual rules / a procedure for rejection

We do.

Can you please provide us a link to these rules?

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

How wide is your screen?

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.

Then please explain me why the ping-pong happens. Do the core devs always have time to report only one minority and never more at once?

For code style: You insist on the 80-character maximum but there is something way more serious like:

• Multiple classes defined in one header
• Class definitions in .cpp files
• Multiple .cpp files sharing one header file
• Class and files named way differently

Why is this serious? If you are looking for a class definition, you don't know what file you have to look at. If you don't find the class definition, you have to scan through all the files. Fixing this issue would do way better business than insisting on the 80-character maximum.

[rubenwardy]

but there is something way more serious like:

• Multiple classes defined in one header
• Class definitions in .cpp files
• Multiple .cpp files sharing one header file
• Class and files named way differently

Why is this serious? If you are looking for a class definition, you don't know what file you have to look at. If you don't find the class definition, you have to scan through all the files. Fixing this issue would do way better business than insisting on the 80-character maximum.

Minetest's code base is a mess already - refactors are needed to clean it up

I agree with fixing these. Except for:

Multiple .cpp files sharing one header file

There's one case where this is ok - in porting code, where the cpp file used depends on the platform

[Wuzzy]

Actually, the 80 char limit is the code style rule that I hate the most. I can almost live with the other ones, but they still annoy me. I am pretty sure it all can be automated away somehow, but I'm too lazy to figure out that for myselves.
Yes, I know that eliminating almost ALL code style rules is extreme, I wasn't really serious anyway. :P

> No real priotization of PRs

There is, see the various labels and milestones.

It appears they are only used in a few exceptional cases, however. There's only “low priority” and “high priority” and that's it. Using milestones is a bit better, but this too is rare.

It is not really clear on which criteria one PR (or issue) “earns” a high priority or a milestone assignment. And rarely do I see a justification when it happens. It just appears arbitrary to me. More transparency would be nice.

> Have actual rules / a procedure for rejection

We do.

Where???

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

The correct/expected procedure has never been written down so far, wasn't it? Minetest is the ONLY FOSS project that I know of that makes use of such a label.
It's not only about me. It's about all other potential contributors.

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

The data suggests otherwise. The oldest open PR is from 2016 and there are close to 100 PRs that are at least 6 months old. I don't know how many of them are still active, but the high number of old PRs is very concerning.

It is concerning because it means that a large amount of contributor time is basically wasted, which can be very demoralizing.

[Hume2]

I agree with fixing these. Except for:

Multiple .cpp files sharing one header file

There's one case where this is ok - in porting code, where the cpp file used depends on the platform

I agree but then the filenames should be chosen in a way so it is obvious which .cpp files belong to which .h files.

[Wuzzy]

Suggestion: Rename “rebase needed” to “conflicts need solving” or something like that. Because that's what seems to be actually desired.

[sfan5]

Multiple classes defined in one header

This would massively inflate the amount of header files for little benefit. C++ isn't Java and you don't have to turn it into that.

Class definitions in .cpp files

Where's the issue here? Those classes are private to the file, there is no use creating a header for it.

Why is this serious? If you are looking for a class definition, you don't know what file you have to look at. If you don't find the class definition, you have to scan through all the files.

There are IDEs and text editors that can help you with this and even if you don't use one you can still run grep over the entire source.

The correct/expected procedure has never been written down so far, wasn't it? Minetest is the ONLY FOSS project that I know of that makes use of such a label.
It's not only about me. It's about all other potential contributors.

We expect contributors to be familiar enough with git to rebase their changes/fix conflicts, but I understand that many people are not familiar with this.
Either way here's how you'd do a rebase:

Code: Select all

git checkout the_branch_with_your_changes
git remote add upstream https://github.com/minetest/minetest # needed once if you haven't done this before
git fetch upstream
git rebase upstream/master
git mergetool # in case of conflicts; installing a GUI tool such as meld can be useful
git push -f origin the_branch_with_your_changes

If you want to do a merge instead (for whatever reason) run git merge --no-edit upstream/master instead of git rebase.

[Lone_Wolf]

There are lots of different ways to rebase it seems. I've had no trouble with it though.

To be honest, how many contributors know what is rebase and how to do it right?

I haven't seen anyone take the trouble to ask so if there are people like that I'm not sure knowing how to rebase is the issue

[mercor]

What are some concrete tools or processes that could be used to improve the situation?

Automatic linting and code formatting

in javascript world, prettier has been a tremendous tool, as it automatically formats the code to match the code conventions, on save (if using an editor with a plugin for prettier available) or using CLI. There seems to be a plugin for lua, but it appears to be in alpha stage without any recent development.

Is there a such tool currently in use in minetest/minetest? Are there any auto formatters available for lua or c++?

Processes

Would it help if there would be a dedicated repository for RFCs as files, with PRs as means for creating them? All of the processes could be declared using RFCs, and they would all be in one place, where they would be publicly visible. As they would be created using PRs, the discussion related to them would be easy to find and go through.

There are other open source projects that do this, among them OBS Project.

The problem with keeping code conventions and other process descriptions in places like wikis is that 1) modification access is usually restricted, 2) discussion related to changes is not in the same place as the change, and 3) its easy to make small changes, without proper review process taking place before the change occurs. Then there's also the problem that 4) a wiki is an external site, that contributors need to be aware of, where as an RFC repository is just a sibling repository to the projects other repositories.

Other tools

Could something like octotree be of help in the review process?

[Zughy]

After spending some time lurking in the repo (well, I've been partecipating with a few PRs too actually), imho the problem is not specifically the amount of PRs, but the slowness of the whole process.

For instance, this was a PR of mine where I literally changed TWO lines, more precisely two strings: https://github.com/minetest/minetest/pull/9789

This modification had been requested since 2017 with a "Supported by core dev" tag, and the whole issue had been created back in 2014: https://github.com/minetest/minetest/issues/1612

I'm not fond on C++ and understanding Minetest code is honestly a pain in where-the-sun-doesn't-shine, but this issue didn't require nor coding skills nor knowledge of the code.

The point is:
1) it took 12 days to merge a PR which changed 2 strings
2) a 3-year-old issue super easy to fix and with a core dev approval was still lying there


About point #1, that's plain too slow: something in the management of the PR isn't right, but I can't of course tell what, not being part of the staff.
About point #2, I think devs (not core devs, I mean ALL devs) should focus more on solving old issues than focusing on new features and, consequently, on creating more issues (nobody codes perfectly). There is a huuuuge list of things devs could focus on: the "Supported by core devs" issues are 8 pages long (!!!). You literally don't even have to think about it, just pick whatever you like the most and work on it. Stop rushing towards the future.

[LMD]

We should probably start by closing some of the 8 PRs marked "possible close" and mark more PRs as "possible close" if they are unlikely to be merged.

[Linuxdirk]

… if they are unlikely to be merged.

Everything older than Q1 2019 is unlikely to be merged. On a more radical side: Everything older than 2-3 months is unlikely to be merged.

[KGM]

Why is https://github.com/minetest/minetest/pull/9907 not merged yet?
Its a PR allowing servers to send shaders to the client, as media...
This means that a server can greatly enhance the client's graphical effects in a way that best fits the server's overall ambiance...
I don't have to tell you how much it could enhance gameplay to have custom graphical effects for each game...
The (if earlier versions of the PR were used) possible (but unlikely) issue that breaking changes in the c++-shader-interface could render shaders of some servers incompatible with some clients (if they were developed for another version of that interface) got eliminated by adding "shader versioning".
Basically, each shader first checks whether it is compatible or not, and if it is not, it prohibits its real contents from being executed.

-The PR is high quality
-The PR is really small and thus easy to review (It adds just 121 lines)
-There is no consensus that it is "not for MT"
-The PR does not lack any requested improvements
-The PR is not ancient at all
-There is no rebase needed
-The PR passed all automatic checks
-The code is written according to the coding style rules
-It just adds a feature, it breaks absolutely nothing
-It also changes absolutely nothing as long as the new feature suggested isn't used
-Noone found any reason why it should not be merged
So why isn't that PR merged yet?

[rubenwardy]

because it hasn't been reviewed yet

[KGM]

But it's so small... (only 121 lines are added)
why didn't you review it yet?
Come on... 121 lines... that's like a short story.

[rubenwardy]

It's not the highest on my priorities.

But I just have, given you asked so nicely :)

[KGM]

Nice. Thank you for your time.

[Wuzzy]

Remember: You are competing against >150 PRs for attention of <10 core devs. It has become very hard to get anything merged nowadays, no matter the quality.

I think the high number of open PRs alone is already a reason why progress is so slow. Because the important PRs simply drown in the masses.
Bringing this number down significantly would be a big help. If Minetest would only have, let's say, up to 20 open PRs at a time, things would be much more managable.

Has there been any strategy tried out so far? It all looks like business as usual to me … It seems we still have many open PRs around that have no chance of ever getting merged, yet they stay open forever. Something needs to be done. But the question is: What?

Basically the question is: How do we manage a huge flood of PRs with only extremely limited reviewer time? If we find the answer to this question, we win. :D

[rubenwardy]

Has there been any strategy tried out so far? It all looks like business as usual to me … It seems we still have many open PRs around that have no chance of ever getting merged, yet they stay open forever.

We've done review sprints, where a few of us have spent more time than usual trying to get PRs down below a certain number. This works until the sprint stops, where the number of PRs increase again

Something needs to be done. But the question is: What?

Basically the question is: How do we manage a huge flood of PRs with only extremely limited reviewer time? If we find the answer to this question, we win. :D

This is the million dollar question

[LMD]

I'm saying more (specialized?) core devs. I'd tend to nominate Wuzzy. I know from his mods that he certainly has the skill to review all changes on the Lua side of things (builtin, mainmenu, etc.).

More reviewers is the solution in general. Not only core dev reviews should count, all reviews of reputable community members, which are specialized in the field of the PR, should count.

[Zughy]

Remember: You are competing against >150 PRs for attention of <10 core devs.

10? I've been here for about three months and the only ones I've seen active as today are ruben, sfan, paramat and krock (and Clobber, I wasn't aware he was a core dev). I also actually wondered what's the role of celeron55 nowadays, as his last commit it's from 2017 and I've never seen him around (EDIT: I guess he's using some other account to not be bothered every second)

[Zughy]

Time to add another (personal) piece to this PR story and how it simply doesn't work

More than a month ago I worked with a friend of mine on a PR about an on_wield callback. After three days of (useful) discussion and a few changes, sfan5 tested it and approved it, till we opt to add an on_unwield callback too and, long story short, on May 30, sfan5 tested it and approved it again. Also, we had been added for the 5.3.0 milestone.

After 12 days of silence waiting for a second approval and the 5.3.0 which would have been shipped that very day, due to ClobberXD spamming its similar PR asking to be considered, someone noticed that the callback wasn't actually called in every situation. This, again, only because Clobber kept asking for a review for his PR; if he hadn't done it, nobody would have theoretically noticed it.
That meant being removed from the 5.3.0 milestone and sayounara. When I expressed my disappointment for the PR system, sfan5 answered me with the same old refrain of "it's disappointing [...] but how do you want to fix the issue of dev time?"

As I stated before his message, it's not disappointing, it's far worse: it's disrespecting. I've spent a few words with ruben already a month ago about my personal view of the MT management, but I guess it's time to go public without sweetening up anything:

1) Trying comprehending your code is a pain in the ass as there is no official documentation, not even a file describing the hierarchy of it or a few guidelines. This is definitely a buzz-killer for anyone who'd like to help, so when you ask for more core devs and/or contributes, this is the main reason no new entries (like me) help you: your code is a fucking maze and no, "use an editor which allows you to jump between functions" (another sfan5 refrain) is not a proper solution. Also, let's be clear about one thing: I'm a volunteer in real life for teaching people about FOSS, privacy and such, so I DO want to help you because I DO believe in your project, its cultural value and its potential, more than a casual dev or Linux user who stumbled upon MT by accident. I've been learning C++ to help you (yes, JUST you), I've spent days (and nights) to reconstruct a raw mental map of how the engine works, I've been translating the modding book in my language to make MT more accessible, I've been trying to help you concentrating all the most active users opinions in one single topic and if there is a tiny Italian MT community (H4ml3t aside) is only because of me, therefore I'm definitely not the average person passing by. So please consider my wall of text, scale down the effort by ten and you'll obtain what an average new member of the community is willing to do in order to help you. Just try to imagine how much time they're willing to sacrifice in order to understand the code: they'll be gone before saying "hello". And this is YOUR fault, not theirs.

2) You're terribly disorganised. Like, for real. Wuzzy spent enough words about it, so let's keep the perspective from an external point of view only: 160+ PR, 950+ issues, milestones saying nothing about priorities. What are we going to see in 5.4.0? Or in 5.5.0? Will there actually be one or it'll go directly to 6.0.0? Where is the roadmap? The short answer is: there is none. Sure, there is a document saying MT goal is to be as generic and optimised as possible, but that's it. What happens right now is "huh, this seems important. Yeah, let's add it to the next release... for now". The end. Nobody is telling you to schedule EVERY single issue and every aspect of the game, because nobody can schedule life, unexpected events will always happen. But for fuck sake show us AT LEAST a sketched roadmap. Take some time to understand where you want to go before even going, show some respect BOTH to your mental health and to anyone who'd like to help. Temporarily close issues and PR if you feel overwhelmed, who fucking cares, because without a lucid captain a ship won't last long. PLEASE share where you'd like to go with Minetest once and for all. What do YOU want to see first (because at the end of the day, the final decision is yours)?

3) Some of you lack focus, massively wasting time and energies. I can't stress this enough, starting from MTG. You're complaining you don't have enough dev time, yet you keep developing and supporting a modding base advertised as a (horrible) game. I'm sorry if someone might feel offended, but that's what MTG looks like (have a look at the survey, that's not me saying it). But let's remain on topic: there is, what, 4 of you? 5? Start cleaning that huge mess that are MT issues, because it looks evident you can't support both the engine and the game, so why keep bothering? Also, rubenwardy is actively following more projects than three normal persons put together. Why do you hate yourself so much, man? I'd like to develop more too for my games, as I'd need a team/party mod, an abstract classes mod, an achievement mod (where all three could fit in any kind of game) and a fork of SkinsDB for my personal needs. But I'm one single human being already following three mods and a server, I don't want to end up worn out and stressed out because of my projects. I prefer to focus on arena_lib till it needs attentions, because I want to appear reliable to people, rather than be surrounded 360 by whatever thing I wanted to do. I feel responsible for it, and I want to keep it tidied up (these are my milestones). This is definitely not what Minetest repo looks like at the moment and this is definitely not an incentive to help.

If you don't fix these aspects, PRs and issues will keep increasing no matter how many sprints you do trying to contain them, and no other people will come to help you. Words won't fix anything, gestures will. I hope I answered (again) the question in full, in order to spare me more bullshit in future

[LMD]

1) Trying comprehending your code is a pain in the ass
2) You're terribly disorganised
3) Some of you lack focus, massively wasting time and energies

I second all of this except for the third point.

[Astrobe]

Has there been any strategy tried out so far? It all looks like business as usual to me … It seems we still have many open PRs around that have no chance of ever getting merged, yet they stay open forever.

We've done review sprints, where a few of us have spent more time than usual trying to get PRs down below a certain number. This works until the sprint stops, where the number of PRs increase again

Something needs to be done. But the question is: What?

Basically the question is: How do we manage a huge flood of PRs with only extremely limited reviewer time? If we find the answer to this question, we win. :D

This is the million dollar question

Simple: reject PRs for things you did not ask for -- e.g. refuse anything not related to an issue that has been marked "help wanted" or something.