Loosening the commit limit for work branches

[Ben Cooksley]

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)

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)


>
> HS
>

Cheers,
Ben
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-devel/attachments/20220825/12a00974/attachment-0001.htm>