michaelni changed the topic of #ffmpeg-devel to: Welcome to the FFmpeg development channel | Questions about using FFmpeg or developing with libav* libs should be asked in #ffmpeg | This channel is publicly logged | FFmpeg 8.1 has been released! | Please read ffmpeg.org/developer.html#Code-of-conduct
<andrewrk>
I think you should simply disable forks and suggest for contributors to use the agit workflow
<andrewrk>
that's what we would do anyway. but I think we're going to stick with codeberg for a little while longer
<BtbN>
specially if you want to have a lengthy description
<andrewrk>
it's just `git push origin HEAD:refs/for/main` and then you can go edit the PR in the web ui
<BtbN>
At which point notifcation mails with incomplete title and body have already gone out, and will forever be what I see in my mail history...
<andrewrk>
if we count steps it's 1 step (agit) vs 3 steps (forking)
<andrewrk>
it uses the first commit for title and body by default
<BtbN>
Looking at my mail history, it uses the branch name
<andrewrk>
I stand corrected
<BtbN>
And ironically, agit also server side makes the storage problem worse :D
<BtbN>
Cause each agit pr is a fork
<andrewrk>
anyway I don't have any point to prove, I'm just trying to collect information
<BtbN>
To avoid commit-smuggling
<BtbN>
It's a shallow one I think though, so not full storage use
<andrewrk>
I mean if it reaps them on closure that should be fine
<BtbN>
I hope it does, I never checked
<BtbN>
though realistically, if we enforced agit, we'd by now have more of those than actual forks
<BtbN>
open ones
<andrewrk>
interesting point
<andrewrk>
it really should only need to store the files edited, not anything else from the main repo...
<BtbN>
yeah, it's not a full on fork like a forked repo is
<BtbN>
but it is not stored inside of the repo
<andrewrk>
security schmecurity
<BtbN>
Well, it was decided, by vote, that it's a security concern if a fork can inject objects into your repo
<BtbN>
So that's what's being acted on
<BtbN>
hm, it also seems like only a handful of forks are responsible for the vast majority of storage use
<BtbN>
Nothing suspicious in them at first glance though. Just active, so very diverged pack files
<andrewrk>
handful of codecs are responsible for the vast majority of videos encoded
<andrewrk>
pareto principle strikes again
<BtbN>
It's also not like we're talking about unfathomable storage sizes here
<andrewrk>
how much, if you don't mind sharing?
<BtbN>
right now it's around 30GB deduplicated via hardlinks
<BtbN>
And would be ~120GB if the hardlinks suddenly turned into independent files
<andrewrk>
and that's with about 405 forks?
<BtbN>
393 ffmpeg.git directories at least
<BtbN>
It's more that the current server has 160GB of disk space. And upgrading it has gotten prohibitively expensive
<BtbN>
So I'd kinda like to keep fitting into that
<BtbN>
I could move them out into a block storage volume. But those are not part of the automatic snapshots of the whole server, so using it would instantly neccesitate a much more involved recovery strategy
<fjlogger>
[FFmpeg/FFmpeg] Issue #23451 opened: MTV demuxer: video_fps zero due to integer truncation causes invalid timebase (incomplete fix for #755) (https://code.ffmpeg.org/FFmpeg/FFmpeg/issues/23451) by 51511
tufei has quit [Remote host closed the connection]
<haasn>
BtbN: where was the upstream issue that forgejo loses associates between review comments and commits? I also want to add that there's no intuitive way to go from a review comment to the commit it's attached to
<BtbN>
Losing association?
<BtbN>
You mean if the code box is empty?
<BtbN>
That happens sometimes if you comment on a deleted line
<haasn>
BtbN: I mean that gitlab shows "Ramiro Polla @ramiropolla started a thread on commit 83b62355 1 week ago " on every review comment in the summary
<haasn>
ForgeJo has no such indication, and I can't click on anything to get to the commit that the comment is referring to
<haasn>
I've started increasingly staging my own PRs on code.videolan.org for this reason, and I regret not doing so every time don't and open a forgejo PR instead
<haasn>
it's kind of annoying though because MRs on code.videolan.org have even less visibility than PRs on forgejo so I only do it when I expect a specific person to review the change (as in the case of swscale / ramiro)
<haasn>
(and because our CI doesn't work on gitlab, only forgejo)
<BtbN>
There is a commit in the DB field, the problem is it can be outside of the PR, if you commented on a line not touched by the PR, so that'd be incredibly confusing.
<BtbN>
It might need deeper changes, like also explicitly tracking which commit a user was looking at when commenting
<haasn>
yes, that's exactly the point I'm trying to make
<BtbN>
at the moment it only tracks that in the form of the context snippet
<BtbN>
the commit it stores is the result of git blame, so it can be outside of the PR
<haasn>
commenting on the commit is simply not the same as commenting on the commit that `git blame` happens to assign to the line you're looking at in the overall summary
<BtbN>
So no, that's not possible to change
<haasn>
never mind that this is simply not possible if one commit introduces code that another commit removes again
<haasn>
(how would that even work?)
Traneptora has quit [Quit: Quit]
<BtbN>
If that happens, the review commit is simply not displayed anymore outside of the summary, where it's marked as stale
<haasn>
even if the PR was never updated?
<BtbN>
Since if you force-push, the commit might be garbage collected at any time
<haasn>
i.e. I create a PR which first adds code and then removes it in a later commit
<BtbN>
Oh, you mean without force push
<haasn>
and then I attach a review comment on the transient code while looking at it in the per-commit view
<BtbN>
Not sure what happens in that case. Probably just does not show the comment
<BtbN>
looking at the code, it should be entirely impossible for that to be empty, but the comment successful
<haasn>
doesn't surprise me if there are massive bugs here because forgejo has a well-known tendency to completely misattribute review comments to the wrong lines even in the absence of force pushes
<BtbN>
No, it simply lacks the reverse-line-blame at the moment
Traneptora has joined #ffmpeg-devel
<BtbN>
Someone thought "hey, if we line-blame each comments line, we can more easily track where it is in the current HEAD", which is true
<haasn>
I've made reviews on PRs in the past and had the context snippet be completely wrong
<BtbN>
so they implemented the line blame
<BtbN>
but completely forgot to ever undo it again
<BtbN>
hence the utter chaos when viewing the comments
<BtbN>
That has finally been added now, or at least is in the process of being added
<BtbN>
So v16 should be a lot better, and also display already existing comments in the correct place
<haasn>
I suspect whoever designed forgejo only does reviews by looking at the overall diffstat and doesn't care about intermediate states
<haasn>
otherwise this design could never have appeared
<BtbN>
It's just been fixed and fixes have been backported cross multiple projects by various authors
<BtbN>
and it's generally not an easy problem
<haasn>
(I am personally the opposite; I ignore the overall diffstat and only do reviews by commit)
<BtbN>
Like, often there simply is no good answer where and if to display a comment
<haasn>
:shrug: gitlab has handled this perfectly for a few years now
<haasn>
but I digress
<BtbN>
gitlab also can only guess
<BtbN>
Like, in a lot of cases, there simply is no correct answer
<haasn>
for example?
<BtbN>
make a comment, force push the code it's on away. Where to put it?
<haasn>
gitlab marks it as "outdated" and links to the version of the commit it was attached to at the time
<haasn>
but doesn't auto-resolve or hide it until manually resolved
<BtbN>
And if that commit doesn't exist anymore?
fjlogger has quit [Quit: fjlogger]
fjlogger has joined #ffmpeg-devel
<haasn>
I'm not sure how to test that. Looking at 5-year old MRs on videolan/libplacebo I can still see the outdated commits from past iterations of PRs
<haasn>
It sometimes says " The source project of this merge request has been removed. " though
<haasn>
but the commit itself is preserved, it seems
<haasn>
I think gitlab associates MR commits with the project the MR is made against
<haasn>
i.e. protects them against `git gc`
<haasn>
it probably does this by internally taking a ref of each push state; because it also uses them to generate diffs between versions of MRs
Traneptora has quit [Quit: Quit]
<BtbN>
There is a ref to the HEAD of each PR
<BtbN>
but it'd a bit crazy to keep a ref to every HEAD each PR ever had
<BtbN>
Yeah, most of the issues are fixed for v16
<BtbN>
Still no link to the specific commit though, since that is not tracked.