Let's make 2025 a year when code reviews became common in Debian

[Otto Kekäläinen]

Hi,

Numerous people are posting Merge Requests on Salsa. Please help review them!

There is no single dashboard to show all Merge Requests for all Debian
packages, but here are the largest teams listed to show how many they
have open (and total count in parentheses):

938 (9657) https://salsa.debian.org/groups/debian/-/merge_requests
91 (3058) at https://salsa.debian.org/groups/go-team/-/merge_requests
78 (2336) https://salsa.debian.org/groups/python-team/-/merge_requests
32 (1686) https://salsa.debian.org/groups/kernel-team/-/merge_requests
84 (1491) https://salsa.debian.org/groups/gnome-team/-/merge_requests
619 (1242) https://salsa.debian.org/groups/java-team/-/merge_requests
93 (980) https://salsa.debian.org/groups/multimedia-team/-/merge_requests
29 (964) https://salsa.debian.org/groups/rust-team/-/merge_requests
15 (882) https://salsa.debian.org/groups/DebianOnMobile-team/-/merge_requests
137 (761) https://salsa.debian.org/groups/installer-team/-/merge_requests
31 (725) https://salsa.debian.org/groups/games-team/-/merge_requests
24 (605) https://salsa.debian.org/groups/Mobian-team/-/merge_requests
15 (444) https://salsa.debian.org/groups/js-team/-/merge_requests
14 (343) https://salsa.debian.org/groups/libvirt-team/-/merge_requests
3 (282) https://salsa.debian.org/groups/med-team/-/merge_requests

If you have some spare time for Debian today, please consider
collaborating with another maintainer by providing them
review/feedback on an open Merge Request.

Some of them might be stale for various reasons, but there are for
sure many that are genuinely pending review/feedback. You don't need
to be a subject-matter expert. Any feedback is surely appreciated by
the author.

I hope more people do code reviews as I believe it can have these
benefits for Debian:
- Better social dynamics, fewer lonely maintainers and more collaborative spirit
- Better documentation of changes as authors write commit messages
thinking about the reviewers who will read them
- Higher average quality of changes (assuming Linus' Law)
- Hopefully higher quality also via CI results that integrate into the MR review
- Faster spread of best practices as more maintainers read code
written by multiple people in multiple packages
- More opportunities for new contributors to become active in Debian
as Merge Requests offer a easily relatable workflow to most software
developers


I personally feel strongly that code reviews and discussing optimal
solutions with other people makes Debian packaging way more fun than
simply working solo.

- Otto

[Chris Hofstaedtler]

Post by Otto Kekäläinen
Numerous people are posting Merge Requests on Salsa. Please help review them!
There is no single dashboard to show all Merge Requests for all Debian
packages, but here are the largest teams listed to show how many they
938 (9657) https://salsa.debian.org/groups/debian/-/merge_requests

[..]

Post by Otto Kekäläinen
If you have some spare time for Debian today, please consider
collaborating with another maintainer by providing them
review/feedback on an open Merge Request.

I gave this, specifically reviewing MRs in the debian namespace, a
try after your last message on this topic. Unfortunately I have to
say, it feels like a huge waste of time and is mostly frustrating.

I haven't noted down hard numbers, but my feeling is that 40%+ of the
MRs are from the janitor-bot, and mostly outdated. Anyone looking at
the list should immediately filter them out, because they are not
actionable in any way.

Then a lot of the MRs I looked at were "cleary good idea", but were
being ignored for _years_. I'm guessing this is because the
maintainer of the respective package is just AWOL, and we don't
have a process for dealing with that. In that case one can opt to
make their own life more miserable by doing a review, apply, test
build, test and "team upload", and at some point one will see the MR
submitter didn't bother testing the change.

The other big category of MRs in the debian namespace was and still
is: MRs where the maintainers don't get mails from salsa. If one is
active with the project, one can know who is currently around and
assign / ping them in the MR, and hope they'll respond after a few
days. The original submitter obviously is long gone, because these
MRs also sit there for years.

Another is MRs for packages that were removed from unstable a long
time ago. I've closed them when I encountered them, but did not file
a ticket to get the repo archived (*).

Having said that, my actions did get some MRs merged and a few
people were happy about that (thanks for the feedback!). But
overall, I still think it was a waste of time. The numbers are just
not in the favor of reviewers (and probably also not in favor of
maintainers).

Maybe a viable option for the debian namespace is to blanket close
any MR that is older than 6 months. But I don't know how that will
fare for the Janitor MRs.

Frustrated,
Chris


(*) there's a limit to "boring but someone needs to do it" where
I'll step in.

[Otto Kekäläinen]

Post by Chris Hofstaedtler

Post by Otto Kekäläinen
938 (9657) https://salsa.debian.org/groups/debian/-/merge_requests

[..]

Post by Otto Kekäläinen
If you have some spare time for Debian today, please consider
collaborating with another maintainer by providing them
review/feedback on an open Merge Request.

I gave this, specifically reviewing MRs in the debian namespace, a
try after your last message on this topic. Unfortunately I have to
say, it feels like a huge waste of time and is mostly frustrating.

Thanks! Seems we are now down to 838 open MRs in the Debian namespace.

Post by Chris Hofstaedtler
I haven't noted down hard numbers, but my feeling is that 40%+ of the
MRs are from the janitor-bot, and mostly outdated. Anyone looking at
the list should immediately filter them out, because they are not
actionable in any way.

I would recommend to just skip the janitor ones for now and focus on
providing feedback to humans to facilitate collaboration (among
humans).

Eventually the person making an upload of a package would be the best
person to merge/reject whatever janitor submitted ones are open.

Post by Chris Hofstaedtler
The other big category of MRs in the debian namespace was and still
is: MRs where the maintainers don't get mails from salsa. If one is
active with the project, one can know who is currently around and
assign / ping them in the MR, and hope they'll respond after a few
days. The original submitter obviously is long gone, because these
MRs also sit there for years.

I don't know exactly how Salsa is configured regarding notifications.
I agree it should be optimized but don't know exactly how.

Meanwhile, you can configure your own notification at
https://salsa.debian.org/-/profile/notifications and at least ensure
you are "watching" the pacakges you maintain.

Post by Chris Hofstaedtler
Another is MRs for packages that were removed from unstable a long
time ago. I've closed them when I encountered them, but did not file
a ticket to get the repo archived (*).

Thanks!

Post by Chris Hofstaedtler
Having said that, my actions did get some MRs merged and a few
people were happy about that (thanks for the feedback!). But
overall, I still think it was a waste of time. The numbers are just
not in the favor of reviewers (and probably also not in favor of
maintainers).

For sure, the first people going through the long list of pending MRs
will wade through a lot of accumulated cruft.

As a recurring MR review habit I'd expect people to only look at the
MRs that are recent and focus on them.

Post by Chris Hofstaedtler
Maybe a viable option for the debian namespace is to blanket close
any MR that is older than 6 months. But I don't know how that will
fare for the Janitor MRs.

I would not recommend doing any blanket close. I'd rather err on the
side of having some open in vain for a long time than throwing away
something useful. I think my personal record is a patch I sent to
Redis and it eventually got merged after being pending for 6 years.

However, I would support the idea of bulk closing MRs and complete
repositories *if* the package has been removed from Debian for the
same reason we bulk close their bug reports.

Post by Chris Hofstaedtler
Frustrated,
Chris

Thanks for your efforts! I think you can now with a good conscience
ignore old MRs and just focus on new ones next time you have time to
review.

The more DDs help with common namespace MR reviews, the less
frustrating it will be to individual DDs.

[Andrey Rakhmatullin]

Post by Otto Kekäläinen

Post by Chris Hofstaedtler
The other big category of MRs in the debian namespace was and still
is: MRs where the maintainers don't get mails from salsa. If one is
active with the project, one can know who is currently around and
assign / ping them in the MR, and hope they'll respond after a few
days. The original submitter obviously is long gone, because these
MRs also sit there for years.

I don't know exactly how Salsa is configured regarding notifications.

Opt-in.
See also https://salsa.debian.org/dep-team/deps/-/merge_requests/8#note_512610

[Andreas Tille]

Hi,

Post by Otto Kekäläinen

Post by Chris Hofstaedtler
I gave this, specifically reviewing MRs in the debian namespace, a
try after your last message on this topic. Unfortunately I have to
say, it feels like a huge waste of time and is mostly frustrating.

Thanks! Seems we are now down to 838 open MRs in the Debian namespace.

Nice.

Post by Otto Kekäläinen

Post by Chris Hofstaedtler
I haven't noted down hard numbers, but my feeling is that 40%+ of the
MRs are from the janitor-bot, and mostly outdated. Anyone looking at
the list should immediately filter them out, because they are not
actionable in any way.

I would recommend to just skip the janitor ones for now and focus on
providing feedback to humans to facilitate collaboration (among
humans).
Eventually the person making an upload of a package would be the best
person to merge/reject whatever janitor submitted ones are open.

Regarding Janitor: I admit I love these automated tools polishing
packages regarding so many issues reported by lintian and beyond. Its
really great and I really appreciate what Jelmer did. I use
lintian-brush (which is as far as I know the script behind janitor)
regularly (= a couple of times per day statistically). A big thank you
to Jelmer!

However, in all teams I'm deeply involved we asked Jelmer to not run
Janitor automatically on the Git repositories. The rationale is, that
if a package is not uploaded commits by the Janitor might become
outdated - well, finally what is described above is happening. We
simply run either lintian-brush (mostly triggered by routine-update
which included lintian-brush) right when working / before uploading a
package. This makes sure the package is polished *at the right time*.

I'm not sure if it is good that Janitor runs on Debian/ team packages if
it turns out that people do not care for those MRs created by Janitor.
Well, as far as I'm informed Janitor is not creating MRs any more but is
commiting directly so the non-human MRs will not be an issue any more
(Jelmer, please correct me if I'm wrong). However, if we have somehow
structured teams the way Janitor behaves can be written inside the team
policy where team members either like those commits or are running the
tools manually. For the Debian/ team I'm not really sure what might be
the best solution.

Post by Otto Kekäläinen
I don't know exactly how Salsa is configured regarding notifications.
I agree it should be optimized but don't know exactly how.
Meanwhile, you can configure your own notification at
https://salsa.debian.org/-/profile/notifications and at least ensure
you are "watching" the pacakges you maintain.

Thank you for the reminder. For my taste its suboptimal that users need
to actively switch on notifications but I'm aware that we have lots of
different tastes and it might make sense to stick to rather less than
more notifications default to not bother volunteers with to much
information. On the other hand this leads to the observed effect of
very many unattended MRs.

Post by Otto Kekäläinen

Post by Chris Hofstaedtler
Another is MRs for packages that were removed from unstable a long
time ago. I've closed them when I encountered them, but did not file
a ticket to get the repo archived (*).

Thanks!

+1

Regarding archiving the repo: In some teams this is done by moving the
repository to some folder named "attic" (or similar). I'm not sure
whether removed packages might consume a severe amount of disk space on
Salsa. IMHO having the repository around is rather a feature than a bug
so I'd tend to keep them even in case of removed packages.

Post by Otto Kekäläinen
However, I would support the idea of bulk closing MRs and complete
repositories *if* the package has been removed from Debian for the
same reason we bulk close their bug reports.

I tend to keep the repository for several reasons:
* Users might like to create their own local packages from the last
status of the repository.
* Vcs fields are published on lots of places and would become broken
links for no good reason.
* Someone might decided to re-introduce a package and likes to have
the history
* Repositories might be a great resource for "software-history"

What advantages would you see in removing repositories of removed
packages except saving some disk space?

Post by Otto Kekäläinen

Post by Chris Hofstaedtler
Frustrated,
Chris

Thank you for taking over frustrating work. Its really appreciated.

Kind regards
Andreas.

[Gioele Barabucci]

Post by Andreas Tille

Post by Otto Kekäläinen
However, I would support the idea of bulk closing MRs and complete
repositories *if* the package has been removed from Debian for the
same reason we bulk close their bug reports.

* Users might like to create their own local packages from the last
status of the repository.
* Vcs fields are published on lots of places and would become broken
links for no good reason.
* Someone might decided to re-introduce a package and likes to have
the history
* Repositories might be a great resource for "software-history"
What advantages would you see in removing repositories of removed
packages except saving some disk space?

Indeed, archiving the project [1], as suggested by Chris, seems a more
sensible course of action. Everything remains available, but users are
given a clear indication of the status of the repository.

[1]
https://docs.gitlab.com/ee/user/project/working_with_projects.html#archive-a-project

Regards,

[Andreas Tille]

Hi,

Post by Gioele Barabucci
Indeed, archiving the project [1], as suggested by Chris, seems a more
sensible course of action. Everything remains available, but users are given
a clear indication of the status of the repository.
[1] https://docs.gitlab.com/ee/user/project/working_with_projects.html#archive-a-project

Got it - shame on me that I was not aware and just thought we were
talking about moving to attic or something else. Just archived the
first batch of packages removed from Debian Med - will continue once
time permits.

Thank you for the hint
Andreas.

[Julien Plissonneau Duquène]

Post by Chris Hofstaedtler
Maybe a viable option for the debian namespace is to blanket close
any MR that is older than 6 months. But I don't know how that will
fare for the Janitor MRs.

Given the "Debian time" scale, a much longer delay would be appropriate
IMO. I would suggest 2 years with no activity on the MR before closing
for "staleness", but ideally some data points should be extracted from
Salsa before discussing this.

Cheers,

[Julien Plissonneau Duquène]

Hi,

Post by Otto Kekäläinen
619 (1242) https://salsa.debian.org/groups/java-team/-/merge_requests

FWIW I took a look at that and most of them are Janitor merge requests.
Please just skip them as some are outdated, and I'm planning to take
care of them later this year with some housekeeping tasks.

Identifying merge requests that are clearly outdated and posting a
comment with a short explanation (e.g. "Obsolete MR: already fixed in
current package version") will also help in getting the count down.

Cheers,

[Andreas Tille]

Hi Gioele,

Post by Otto Kekäläinen
Numerous people are posting Merge Requests on Salsa. Please help review them!

Could we do the same for BTS patches?
There are ~5000 patches that have been sitting in the BTS for years, some
trivial (examples from my own: [1,2]),

To my experience from Bug of the Day there are lots of very obvious
patches and easy bugs. I fully agree with you that we have the
obligation to care for patches in BTS which is our prefered way to
report issues. As I wrote in previous mails I consider it worth
discussing how we can make this easier for developers who are not listed
as Maintainer / Uploader. Feel free to join the DebCamp project
suggested by Tobias Frost[3].

others less so. Most of the time they
are in the BTS because the associated packages have no Git/salsa repo.
https://bugs.debian.org/tag:patch

I'm not sure that these packages are on BTS only because the associated
packages have no Git/salsa repo. The BTS is our prefered form of
reporting issues. Adding an MR and linking to it as a patch as you did
in [1] is perfect and as a maintainer (who has all packages on Salsa)
I'm absolutely happy about this.

[1] https://bugs.debian.org/1051861

I've merged two of your gzip patches into the gzip repository. I also
upgraded debhelper compat level and Standards-Version for the kind
review of the maintainer (in BCC, since I know him personally I have no
doubt he will act soon). The repository is featuring the new upstream
version which IMHO is worth uploading to experimental. At least all
tests by Salsa CI are passing[4].

In principle I would tend to do a Debian/ team upload but I'm hesitating
for "Priority: required" packages. Thus pinging the maintainer. I will
tag the bugs closed by your MRs pending in a separate mail.

[2] https://bugs.debian.org/1057101

Its harder to do anything here. The package is actively maintained and
I would simply assume the maintainer (in BCC) has somewhere some Git
repository since this is the prefered way to maintain code these days.
It would be great to have packages of "Priority: required" on Salsa to
enable some team work on all our packages with high priority. This
might be some topic for the DebCamp project[3] as well.

Kind regards
Andreas.

[3] https://lists.debian.org/debian-devel/2025/01/msg00065.html
[4] https://salsa.debian.org/debian/gzip/-/pipelines/798305

[Andreas Tille]

Hi Gioele,

Please note that although the package has a repo on Salsa, MRs there
are/were explicitly disabled, at least for non-DDs (see the postscriptum in
[1], I see they are available now). Therefore were the commits in my
personal repo linked in the report in the BTS.

I guess this is due to some unfortunate default. MRs are possible now in
this repository.

Kind regards
Andreas.