On Wed, 18 Jun 2025 at 14:29:05 +0200, IOhannes m zmölnig wrote:
discussion on lines of code within the MR can become stale when you push new commits that
change the very lines (but the history is kept), and the discussion can be
"resolved" (hidden, as no longer relevant; but still there).
my experience comes mostly from GitHub (for whatever reasons my
gitlab/salsa/... interaction has less reviewing), but I don't remember
significant differences.
If anything, Gitlab is better than Github at doing patch-by-patch
review. By default it presents the diff for the entire MR, which is
convenient for small changes or if you don't care about individual
commits - I don't like that as a default, because I *do* care about
individual commits, but I acknowledge that not everything is designed
for me.
If you navigate to, for example,
https://salsa.debian.org/ci-team/autopkgtest/-/merge_requests/511 →
Commits → click on the oldest commit (at the bottom), then you can page
through the commits with Prev and Next buttons, and do patch-by-patch
review like you would do in an email workflow.
When working with "forges" I normally write my commits with the
assumption that they will be reviewed commit-by-commit (just like they
would be in the Linux kernel workflow), and I leave a comment "best
reviewed commit-by-commit" to remind the prospective reviewer about that
if the branch looks particularly hard-to-review when viewed as a whole.
A common pattern is for the commits in a complex MR to look like:
"refactor -> refactor -> partially fix the actual bug -> fully fix the bug"
if the bug fix wasn't straightforward until after the refactoring.
(Obviously simple MRs will often only have one commit, in which case the
finer details of the workflow hardly matter, because the change and its
justification will be relatively simple anyway.)
On Wed, 18 Jun 2025 at 14:51:02 +0200, Marc Haber wrote:
How about force pushes after doing rebase -i to squash fixups? I have
been told to NEVER do that, but the one time I accidentally did it, it
just worked.
The typical Github/Gitlab workflow encourages having a separation
between two categories of branches:
- stable, "public", safe to reference and will not be force-pushed
(for example "main" or "0.2.x" or "debian/latest")
- work-in-progress, unstable, expected to be force-pushed, only exist as
a collaboration point
In the email-based Linux-kernel workflow, if I understand correctly, the
convention is that only the first category ever get pushed anywhere
public, and the second category exist only on your laptop (because
they're sent to other developers as patches, rather than as a complete
branch).
In a typical Github/Gitlab workflow, the second category *does* get
pushed to public locations, but with some sort of indication that the
branch is unstable work-in-progress. These work-in-progress branches
often also get deleted when they are no longer relevant (either accepted
and merged, or superseded by a different/better proposal for fixing the
same issue, or explicitly rejected or abandoned).
Exactly what form is taken by the indication for a work-in-progress
branch is a local social convention in whatever project you're
contributing to. One common convention is for the centralized repository
to contain only stable/public/safe-to-reference branches, and to assume
that any branch that is in someone else's fork of the centralized
repository is unstable work-in-progress. Another common convention is
for branches with names that start with "wip/" and/or a developer's
username to be unstable work-in-progress, even if they exist in the
centralized repository. You might need to follow an existing project's
conventions, or work with your collaborators to establish a convention
for your own projects.
Either way, you should never force-push to a branch that is considered
to be stable / safe to reference, but in a typical "forge" workflow you
*will* often need to force-push to branches that are work-in-progress.
Generally a MR will be of the form "please merge my work-in-progress
branch into your stable branch", and in the typical "forge" workflow
you'll repeatedly force-push to your work-in-progress branch until the
reviewer is sufficiently happy with it that they are willing to hit
Merge (or merge and push manually if they prefer), at which point your
changes become part of the immutable history of the project, and any
subsequent changes need to be part of a new branch and a new MR.
For example
https://salsa.debian.org/ci-team/autopkgtest/-/merge_requests/511 was a
request to merge the unstable branch `wip/smcv/podman-docs` into the
centralized stable branch `master`. If my co-maintainers had had
objections to the text I initially proposed, I would have force-pushed
to wip/smcv/podman-docs to fix those objections, perhaps repeatedly, and
then merged `wip/smcv/podman-docs` into `master` when all problems had
been resolved.
smcv
