Loosening the commit limit for work branches

[Harald Sitter]

On Wed, Aug 24, 2022 at 8:11 PM Ben Cooksley <bcooksley at kde.org> wrote:
>
> On Thu, Aug 25, 2022 at 2:16 AM Harald Sitter <sitter at kde.org> wrote:
>>
>> On Wed, Aug 24, 2022 at 3:31 PM Noah Davis <noahadvs at 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