On Tue, May 07, 2024 at 16:17:24 +, Joseph Myers via Gcc wrote:
> I'd say we have two kinds of patch submission (= two kinds of pull request
> in a pull request workflow) to consider in the toolchain, and it's
> important that a PR-based system supports both of them well (and supports
> a submission changing from one kind to the other, and preferably
> dependencies between multiple PRs where appropriate).
The way I'd handle this in `ghostflow` is with a description trailer
like `Squash-merge: true` (already implemented trailers include
`Backport`, `Fast-forward`, `Backport-ff`, and `Topic-rename` as
description trailers, so this is a natural extension there).
Alternatively a label can be used, but that is not directly editable by
MR authors that are not also members of the project. There's also a
checkbox either at MR creation and/or merge time to select between them,
but I don't find that easily discoverable (I believe only those with
merge rights can see the button state in general).
> * Simple submissions that are intended to end up as a single commit on the
> mainline (squash merge). The overall set of changes to be applied to the
> mainline is subject to review, and the commit message also is subject to
> review (review of commit messages isn't always something that PR-based
> systems seem to handle that well). But for the most part there isn't a
> need to rebase these - fixes as a result of review can go as subsequent
> commits on the source branch (making it easy to review either the
> individual fixes, or the whole updated set of changes), and merging from
> upstream into that branch is also OK. (If there *is* a rebase, the
> PR-based system should still preserve the history of and comments on
> previous versions, avoid GCing them and avoid getting confused.)
>
> * Complicated submissions of patch series, that are intended to end up as
> a sequence of commits on the mainline (non-squash merge preserving the
> sequence of commits). In this case, fixes (or updating from upstream)
> *do* involve rebases to show what the full new sequence of commits should
> be (and all individual commits and their commit messages should be subject
> to review, not just the overall set of changes to be applied). Again,
> rebases need handling by the system in a history-preserving way.
There's been a long-standing issue to use `range-diff` in GitLab. I
really don't know why it isn't higher priority, but I suppose having
groups like Sourceware and/or kernel.org interested could move it up a
priority list for them.
https://gitlab.com/gitlab-org/gitlab/-/issues/24096
FWIW, there's also a "comment on commit messages" issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/19691
That said, I've had little issues with rebases losing commits or
discussion on GitLab whereas I've definitely seen things get lost on
Github. I'm unfamiliar with other forges to know there (other than that
Gerrit-likes that track patches are generally workable with rebases).
> GitHub (as an example - obviously not appropriate itself for the
> toolchain) does much better on simple submissions (either with squash
> merges, or with merges showing the full history if you don't care about a
> clean bisectable history), apart from review of commit messages, than it
> does on complicated submissions or dependencies between PRs (I think
> systems sometimes used for PR dependencies on GitHub may actually be
> third-party add-ons).
The way I've tended to handle this is to have one "main MR" that is the
"whole story" with component MRs split out for separate review. Once the
separate MRs are reviewed and merged (with cross references), the main
MR is rebased to incorporate the merged code and simplify its diff. This
helps to review smaller bits while also having the full story available
for viewing.
> Pull request systems have obvious advantages over mailing lists for
> tracking open submissions - but it's still very easy for an active project
> to end up with thousands of open PRs, among which it's very hard to find
> anything.
In CMake, the mechanism used to keep the queue manageable is to have a
`triage:expired` label for closed-for-inactivity (or other reasons) so
that closed-but-only-neglected MRs can be distinguished from
closed-because-not-going-to-be-merged MRs. The "active patch queue"
tends to stay under 20, but sometimes swells to 30 in busy times (as of
this writing, it is at 10 open MRs).
--Ben
