[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: Loosening the commit limit for work branches
From:       Harald Sitter <sitter () kde ! org>
Date:       2022-08-26 9:40:59
Message-ID: CAEc+18HFnbM9C2RGpZfFiVEau-cuOdqiUePVNNra6t_fV_VNkQ () mail ! gmail ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]