1. Introduction
This message expands on my remarks at the Cauldron (especially the
patch review and maintenance BoF, and the Sourceware infrastructure
BoF) regarding desired features for a system providing pull request
functionality (patch submission via creating branches that are then
proposed using some kind of web interface or API, with a central
database then tracking the status of each pull request and review
comments thereon automatically), for use by the GNU toolchain (or one
or more components thereof - there is no need for each component to
make the same decision about moving to such software and workflow, and
indeed we have no mechanism to make such decisions for the toolchain
as a whole).
This does not advocate a particular choice of software for such
functionality (though much of the discussion seemed to suggest Forgejo
as the most likely starting point), or a particular choice of where to
host it. Hosting would of course need to meet appropriate security
requirements, and to achieve a passing grade on the GNU Ethical
Repository Criteria, and the software would need to be entirely free
software. Where relevant features are not already supported, it's
important that the software is receptive to the addition of such
features (including cases where part of the functionality is provided
by software specific to the GNU toolchain or parts thereof - such as
for the custom checks currently implemented with git hooks - and the
underlying software provides appropriate interfaces to allow
integration of such external pieces). The list of features here may
be a good basis for reviewing what particular forge software supports
and whether other features can be added, directly or through use of
appropriate APIs.
Forge software may provide other pieces such as bug tracking or wikis
that we currently handle separately from git hosting. In such cases,
we should be able to disable those pieces and keep using the existing
bug tracking and wiki software (while having the option to decide
independently to migrate those if desired).
I consider the overall benefits of such a move to be having more
structured data about all changes proposed for inclusion and their
status (needing review, needing changes from the author, under
discussion, needing merge from mainline, etc.), to help all people
involved in the patch submission and review process to track such
information and to find patches needing review as applicable, along
with providing a more familiar workflow for many people that avoids
many of the problems with email (which affect experienced contributors
working around corporate email systems, not just new contributors).
It would not of course by itself turn people with no interest in or
understanding of systems software development into contributors (for
example, people without knowledge of directories and hierarchical file
storage, or people who only understand software development as web
development). Nor would it prevent the accumulation of large backlogs
of unreviewed patches, as is evident from many large and active
projects using PR workflows with large numbers of open PRs.
As Richard noted in his BoF, email sucks. As I noted in reply, so do
the web and web browsers when trying to deal with large amounts of
patch review state (when one wishes to apply one's own view, not the
forge's, of what is resolved and what needs attention). As I also
noted, in the Sourceware infrastructure BoF, tools such as patchwork
and b4 are the right answer to the wrong question: trying to get
structured data about patch submissions when working from the axiom
that emails on a mailing list should be the primary source of truth
for everything needing review, rather than starting from more
structured data and generating emails as one form of output.
Moving to a pull request system is not expected to change policies
regarding who can approve a change for inclusion, or the technical
limits on who can cause a change to enter mainline (e.g. all people
with commit access would be expected to be able to use a button in a
web interface to cause to PR to be merged, though policy might limit
when they should do so). We can of course choose to change policies,
either as part of adopting a PR system or later.
2. Key features
(a) Some forges have a design that emphasises the tree you get after a
proposed contribution, but not the sequence of commits to get there.
For the toolchain, we care about the clean, logical sequence of
commits on the mainline branch. (We also use linear history on
mainline, but that's a secondary matter - though certainly we'd want
any forge used to support such linear history so that property doesn't
need to change as part of adopting pull request workflow.) Having a
clean sequence of commits has some implications for forge support:
* Support for reviewing the proposed commit message, not just the
diffs, is important (and it should be clear what commit message
would result from merging any pull request).
* Patch series and dependencies between patches are important. In
such cases, fixes for issues from review should go into the
appropriate logical commit in the PR (with rebasing as necessary),
and it should be possible at all times to see what the sequence of
commits on mainline would look like. (E.g. people use some
workarounds on GitHub to manage PR dependencies, but those result in
second and subsequent PRs in a series showing the full set of diffs
from a PR and those it depends on, rather than just the logical
diffs for one PR.)
I consider patch series and dependencies to be separate but related
things: a patch series may not have strictly linear dependencies
(and it's often useful to merge the more straightforward patches
from a series while still discussing and revising others), while a
patch may depend on other patches that it's not logically part of
the same series as. They are, however, closely related, and a
sufficient solution for dependencies might also be adequate for many
cases of series.
Note that series can sometimes have hundreds of patches; any
solution for patch series and dependencies needs to scale that far.
There is of course the common case of a single-patch submission,
where the patch is ready for inclusion after some number of fixes.
In those cases, it's probably convenient if it's not necessary to
rebase - provided it's clear that a particular PR would be
squash-merged, and also what the commit message would be for the
final fixed commit.
* Given the need for rebasing when working with patch series, it's
important to have good support for rebasing. In particular, all
revisions of the changes for a PR that was rebased need to remain
permanently available (e.g. through appropriate documented refs to
fetch to get all revisions of all PRs).
(b) Various people have built efficient workflows for going through
all patch submissions and comments (or all in a particular area), even
when only reviewing a small proportion, and have concerns about
efficiency of a web interface when working with many patches and
comments. It's important to have good API support to allow people to
build tools supporting their own workflow like this without needing to
use the browser interface (and following their own logic, not the
forge's, for what changes are of interest). Good API support might,
for example, include a straightforward way to get all changes to PR
and comment data and metadata since some particular point, as well as
for actions such as reviewing / commenting / approving a PR. Such API
support might be similar to what's needed to ensure people can readily
get and maintain a local replica of all the key data and its history
for all PRs.
Replication like that is also important for reliably ensuring key data
remains available even if the forge software subsequently ceases to be
maintained. Consider, for example, the apparent disappearance of the
data from www-gnats.gnu.org (we occasionally come across references to
old bug reports from that system in glibc development, but don't have
any way to resolve those references).
Another use of such an API would be to allow maintaining local copies
of all PR comments etc. in a plain text form that can be searched with
grep.
(c) Given that a transition would be from using mailing lists, it's
important to have good plain text outward email notifications for all
new merge requests, comments thereon and other significant actions
(changing metadata, approvals / merges, etc.) - through whatever
combination of built-in support in a forge and local implementation
using the API. Such notifications would go to the existing mailing
lists (note that the choice of mailing lists for a patch can depend on
which parts of the tree it changes - for example, the choice between
binutils and gdb-patches lists, or some GCC patches going to the
fortran or libstdc++ lists) - as well as to individuals concerned with
/ subscribed to a given PR. Some very routine changes, such as
reports of clean CI results, might be omitted by default from
notifications sent to the mailing lists.
"good" includes quoting appropriate diff hunks being comments on.
It's OK to have an HTML multipart as well, but the quality of the
plain text part matters. Diffs themselves should be included in
new-PR emails (and those for changes to a PR) unless very large.
I do not however suggest trying to take incoming email (unstructured)
and turn it into more structured comments on particular parts of a PR
in the database.
Similarly, commit emails should continue to go to the existing mailing
lists for those.
(d) Rather than the forge owning the mainline branch in the sense of
every commit having to go through an approved PR, at least initially
it should be possible for people to push directly to mainline as well,
so the transition doesn't have to be instantaneous (and in particular,
if a change was posted before the transition and approved after it, it
shouldn't be necessary to turn it into a PR). Longer term, I think it
would be a good idea to move to everything going through the PR system
(with people able to self-approve and merge their own PRs immediately
where they can commit without review at present), so there is better
structured uniform tracking of all changes and associated CI
information etc. - however, direct commits to branches other than
mainline (and maybe release branches) should continue to be OK long
term (although it should also be possible to make a PR to such a
branch if desired).
Beyond putting everything through PRs, eventually I'd hope to have
merges to mainline normally all go through a CI system that makes sure
there are no regressions for at least one configuration before merging
changes.
(e) All existing pre-commit checks from hooks should be kept in some
form, to maintain existing invariants on both tree and commit contents
(some hook checks make sure that commits don't have commit messages
that would cause other automated processes to fall over later, for
example).
(f) Existing cron jobs that read from or commit to repositories should
use normal remote git access, not filesystem access to the repository,
including using APIs to create and self-merge PRs when pushing to the
repository is involved.
