On Tue, 5 Aug 2025 at 05:06, Kacper Michajlow <kaspe...@gmail.com> wrote: > > On Mon, 4 Aug 2025 at 23:38, Marton Balint <c...@passwd.hu> wrote: > > > > > > > > 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. > > I don't think this is accurate. Committer field is set to the person > who clicks the "merge" button. Same as they would manually git push > the patches. > > Slightly related, I don't like how simple the web ui commit log of > forgejo is, it doesn't show commiter at all. For me this information > is as important as the author. I'm keeping notes on forgejo usage and > will share it when the time comes, it has some annoying limitations > compared to other forges. > > > - 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. > > There is a timeline of events. If PR is approved, you can see if there > were new events (comments/pushes) after that. This is close to the > "merge" button and you should see "approve" as the last event in the > timeline to be sure nothing changes. > > Also if there were rebases after approval, the approved tick mark (in > Reviewers list) becomes yellow instead of green. > > It is also possible to configure to completely discard previous > approval if any push happened. But this hinders the workflow if there > is trivial rebase done to run CI on the latest merge base. Reviewer > would have to approve again. This works if the reviewer is the same > person who will merge, but if both author and reviewer are > maintainers, we should trust each other and respect approver to not > include unwanted changes after this point. > > Saying that, I think (if possible) it should be configured to clear > the approval status if the **user** (not contributor) pushes to this > branch. This way the reviewer has to recheck it, before merge. > > - Kacper
Looking quickly at forgejo source, it seems to do a diff against merge base, so it won't dismiss approvals on trivial rebases. Only if something actually changes. In this case I think enabling the "dismiss stale approvals" protection option is a good idea to ensure the approved state is always valid. Of course this would clear approve if someone fixes even a typo, but I guess it is expected. - Kacper _______________________________________________ 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".
