On Mon, 4 Aug 2025, Alexander Strasser via ffmpeg-devel wrote:
Hi Michael,
hi all!
I think it's a good time to bring stuff like this up for discussion.
On 2025-08-03 21:02 +0200, Michael Niedermayer wrote:
On Sun, Aug 03, 2025 at 05:31:39PM +0200, Michael Niedermayer wrote:
[...]
The solutions are obvious:
1. ignore security and supply chain attacks
2. use merges not rebases on the server
3. rebase locally, use fast forward only
4. verify on server rebases
Maybe not everyone understood the problem. So let me try a different
explanation. Without any signatures.
In the ML workflow: (for simplicity we assume reviewer and commiter is the same
person)
1. someone posts a patch
2. patch is locally applied or rebased
3. commit is reviewed
4. commit is tested
5. commit is pushed
Here the only way to get bad code in, is through the reviewer
If the reviewer doesnt miss anything and his setup is not compromised
then what he pushes is teh reviewed code
if its manipulated after its pushed git should light up like a christmess tree
on the next "git pull --rebase"
With the rebase on webapp (gitlab or forgejo) workflow
1. someone posts a pull request
2. pr is reviewed
3. pr is approved
4. pr is rebased
5. pr is tested
6, pr is pushed
now here of course the same reviewer trust or compromised scenarios exist
but we also have an extra one and that is the server
because the server strips the signatures during rebase it can modify the
commit. And this happens after review and because a rebase was litterally
requested by the reviewer its not likely to be noticed as something out of
place
If I understand the original point you wanted to discuss correctly,
than this is not a question of rebase or merge but one of letting
**commits happen on the forge**. If it happens it bears the
possibility of modification on the server the forge is running on.
TL;DR: I think it's fine the way it's setup now.
I'm not against letting rebase/merges happen on the server because
otherwise we would lose a lot of advantages and comfort we get by
using a forge for PRs.
Only alternative I see is to do PRs on the forge and doing merging
manually by the same person that ensures reviewed PR is not changed
and pushes (after rebase or with a clean merge commit) from their
machine.
Two things came to my mind about the current forgejo workflow.
- Previously it was pretty clear from git history who actually committed
a change from the comitter field. With using forgejo the comitter
field no longer shows the person who actually *committed* the change to
the main repo, but it is inherited from the original pull request commit
instead, so it simply shows the original author of the patch.
- A pull request is writable both by the reviewer and the author up to
the point when it is actually committed to the main repo. So force
pushes from an author can happen anytime during this timeline:
- reviewer reads changes
- approves the changes
- rebases the branch
- sets it up to auto merge
- CI actually runs
- forgejo auto-merge
A reviewer may not realize the new force push from the author. Maybe
forgejo handle some force pushes in this timeline gracefully and aborts,
or ignores them, I am not sure. It still looks a bit fragile, my
expectation as a reviewer would be that what I saw when I finished the
review and clicked on the Approve button will get comitted, when I later
click on the merge button.
Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
