On Wed, Aug 24, 2022 at 8:11 PM Ben Cooksley <bcooks...@kde.org> wrote: > > On Thu, Aug 25, 2022 at 2:16 AM Harald Sitter <sit...@kde.org> wrote: >> >> On Wed, Aug 24, 2022 at 3:31 PM Noah Davis <noaha...@gmail.com> wrote: >> > >> > A week ago, I ran into an unexpected issue while working on a QML port >> > of Spectacle. There is an undocumented pre-receive hook that sets a >> > 100 commit limit for all branches of official repos, including work >> > branches. The error message it gave me told me to file a sysadmin >> > ticket, so I did that. >> > >> > The sysadmin ticket link: https://phabricator.kde.org/T15683 >> > >> > I don't think I can make the ticket public, so here's the conversation: >> > >> > --- Start of conversation >> > >> > ndavis (me): >> > For normal branches, I would understand this since there were issues >> > with spamming #kde-devel in the past. This isn't necessary for work >> > branches, right? I thought the point of the work/ naming convention >> > was to prevent this issue. >> > >> > bcooksley: >> > Work branches weren't meant to be used for large feature developments >> > with 100+ commits in them, which is why this is being blocked. >> > In it's current state - even if rebased - the branch will not be able >> > to be merged without Sysadmin intervention. >> > Will this be squashed prior to being merged? >> > >> > ndavis: >> > > Will this be squashed prior to being merged? >> > >> > Yes >> > >> > > Work branches weren't meant to be used for large feature developments >> > > with 100+ commits in them, which is why this is being blocked. >> > >> > Is this documented anywhere? I don't understand why work branches >> > would block this. The git message says notifications are the reason >> > why the push was blocked, but I thought work branches didn't send >> > notifications? >> > >> > bcooksley: >> > The message is a little misleading, but that is mostly because this >> > situation isn't supposed to really occur. You are correct that work >> > branches don't send notifications. >> > As such it is not documented anywhere. >> > From a policy perspective the reason why we don't allow work branches >> > to grow beyond 100 commits is because if we did then you would never >> > be able to merge them in - not without squashing anyway. >> > This therefore makes you "squash as you go". >> > I would generally recommend that major features be developed in an >> > ordinary branch to allow those that monitor kde-commits and other >> > commit feeds to chime in with real-time reviews, and then merged using >> > a traditional Git merge (vs. our more typical rebase) >> > >> > ndavis: >> > > The message is a little misleading, but that is mostly because this >> > > situation isn't supposed to really occur. You are correct that work >> > > branches don't send notifications. As such it is not documented anywhere. >> > >> > If we are going to keep this limitation, we should document it so that >> > nobody else makes the same mistake as me. We can't assume that >> > something that isn't supposed to happen won't happen because there's >> > no reason to assume this limitation would exist. >> > >> > > From a policy perspective the reason why we don't allow work branches to >> > > grow beyond 100 commits is because if we did then you would never be >> > > able to merge them in - not without squashing anyway. >> > This therefore makes you "squash as you go". >> > >> > I don't understand why we have this policy. If we can't merge an MR >> > with over 100 commits, why can't we just tell the person making an MR >> > when they post the MR to squash it? I think it would make more sense >> > for this policy to apply only when pushing to master or version >> > branches. >> > >> > > I would generally recommend that major features be developed in an >> > > ordinary branch to allow those that monitor kde-commits and other commit >> > > feeds to chime in with real-time reviews, >> > >> > In my experience, nobody does that. Nobody has time to review unless >> > you explicitly request help or you post an MR. >> > I don't know the protocol for discussing these kinds of things, so let >> > me know if this discussion should be moved elsewhere. >> > >> > --- End of conversation >> > >> > After this, I decided it would be better to discuss this with the >> > broader KDE community and I closed the ticket. >> > >> > I did try to switch to a normal branch as Ben Cooksley suggested, but >> > that had the same limitation. I have not tested using a fork, but some >> > of the people I've talked to casually (I can't remember who) seemed to >> > be saying that fork branches don't have this limitation, which I think >> > would make the limit on work branches kind of pointless if it's true. >> > >> > I understand the concern about making unmergeable merge requests, but >> > I don't think a hard 100 commit limit pre-recieve hook is the right >> > way to deal with that. That's something that should be handled by >> > reviewers, not automated systems, because sometimes info needs to be >> > kept until it is time to merge. Instead of having lots of small >> > commits to keep track of each change as much as possible until it's >> > time to review the MR, I've had to squash them into mega commits with >> > lines in the commit message for each commit that got squashed. >> > Normally, I would squash at the end of the review process to ensure >> > that all relevant info is kept and irrelevant info is discarded. >> > >> > What does the broader KDE development community think? Should there be >> > a commit limit and how large should it be? Only KDE devs can make work >> > branches, so the pool of people who can make branches with large >> > amounts of commits is already fairly limited. >> >> Yeah, I don't see why there needs to be a limit at all on work >> branches, let alone one that low. > > > The limitation is aligned with the maximum number of new commits you are > allowed to introduce to a standard branch. > We have those limits because the commit hooks carry out processing on a per > commit basis for all new commits introduced to standard branches - and are > there to protect all the other systems downstream from Gitlab.
Protect them from doing the work they were built to do? :P > > The reason behind applying those same limits to work branches is because > there is an expectation that you would be able to merge a work branch into a > standard branch at the conclusion of the work. Which you can do if you squash first. I mean, I get that we likely need a limit on the standard branches, but for work branches I fail to see the value they add - considering we can and do regularly squash-on-merge. HS