Loosening the commit limit for work branches

[Harald Sitter]

On Thu, Aug 25, 2022 at 11:44 AM Ben Cooksley <bcooksley at kde.org> wrote:
>
> On Thu, Aug 25, 2022 at 9:27 PM Harald Sitter <sitter at kde.org> wrote:
>>
>> 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.
>
>
> 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