On Thu, Aug 25, 2022 at 11:44 AM Ben Cooksley <bcooks...@kde.org> wrote: > > On Thu, Aug 25, 2022 at 9:27 PM Harald Sitter <sit...@kde.org> wrote: >> >> 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. > > > The value they add is to ensure the commit history does not deviate too far > from release branches (which you can then never merge without squashing)
But we **do** squashing. Being able to deviate is the point of a work branch. Otherwise we could just do all our work in master. > Also, it was never anticipated that people would undertake significant and > substantial refactor work within a work branch. > (because then you miss out on community review that comes from having commits > notified to kde-commits@, etc) Well, clearly that turned out different in practice. HS