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