Re: Loosening the commit limit for work branches
On Thu, Aug 25, 2022 at 11:44 AM Ben Cooksley wrote: > > On Thu, Aug 25, 2022 at 9:27 PM Harald Sitter wrote: >> >> On Wed, Aug 24, 2022 at 8:11 PM Ben Cooksley wrote: >> > >> > On Thu, Aug 25, 2022 at 2:16 AM Harald Sitter wrote: >> >> >> >> On Wed, Aug 24, 2022 at 3:31 PM Noah Davis 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
Re: Loosening the commit limit for work branches
On Thu, Aug 25, 2022 at 9:27 PM Harald Sitter wrote: > On Wed, Aug 24, 2022 at 8:11 PM Ben Cooksley wrote: > > > > On Thu, Aug 25, 2022 at 2:16 AM Harald Sitter wrote: > >> > >> On Wed, Aug 24, 2022 at 3:31 PM Noah Davis 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
Re: Loosening the commit limit for work branches
On Thu, Aug 25, 2022 at 7:13 AM David Hurka wrote: > On Wednesday, August 24, 2022 8:11:14 PM CEST Ben Cooksley wrote: > > 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. > > Do I understand it correctly like this? > > Standard branches have a merge hook. > The merge hook runs when commits are added to the branch. > (It is assumed that standard branches are not force pushed.) > The merge hook runs for every individual commit. > There is a limit that only 100 commits can be added at once, > to prevent system overloads caused by many merge hooks being ran at once. > It is a little simpler than that. For any push to a repository hosted on invent.kde.org, a Git hook known as "pre-receive" is invoked by Git on the server. This hook is provided by Gitlab itself and first carries out a number of checks needed for Gitlab itself to operate before it then invokes our pre-receive hook. Our pre-receive hook as part of it's setup determines whether it is on a mainline or personal repository and what in the repository is being modified (release branch, work branch, tag, etc). It checks the content of the push (an operation which is very streamlined) and then if necessary carries out the appropriate notification of our other systems. The systems themselves can handle the load fine, however the humans in the spaces they communicate to cannot as: a) Bugzilla receives an email for every single mention of BUG / CCBUG within every new commit (which notifies every person CCed on those bugs in turn) b) An email is sent to kde-comm...@kde.org for every new commit, which floods the list with excessive traffic c) A notification is sent to what used to be #kde-commits and #commits on Libera for each new commit (as well as some other channels for certain repositories) so those respective IRC/Matrix channels get flooded and rendered unusable by our announce bots. Further, as some email providers impose limits on the number of emails people can receive in short succession this can block delivery of email to people at certain providers (which affects all KDE.org mailing lists, as well as their personal KDE.org/KDEMail.net alias traffic). This essentially denies these people the ability to participate in the community for a period of time. Our hook can be found at https://invent.kde.org/sysadmin/repo-management/-/blob/master/hooks/invent.pre-receive for those curious as to what it does. Regards, Ben
Re: Loosening the commit limit for work branches
On Wed, Aug 24, 2022 at 8:11 PM Ben Cooksley wrote: > > On Thu, Aug 25, 2022 at 2:16 AM Harald Sitter wrote: >> >> On Wed, Aug 24, 2022 at 3:31 PM Noah Davis 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 >>
Re: Loosening the commit limit for work branches
On woensdag 24 augustus 2022 17:12:51 CEST Milian Wolff wrote: > sooner or later you should hit a small mile stone that you can merge already Um, no? Like, no way, not at all? As in quite often, a rewrite is way bigger than that. I merged one this year that took three years of coding...
Re: Loosening the commit limit for work branches
On Mittwoch, 24. August 2022 18:57:12 CEST Noah Davis wrote: > I didn't want to leave the master branch in a semi-broken state or > present what I had come up with in an MR until I could present it as > an improvement. It took a while to figure out what I wanted and > whether some of what I wanted was even achievable. I haven't been > working alone, so some people such as Marco and Nate are aware of the > current state of the branch and have been testing it periodically. I'm > aiming for a 22.12 release, so I intend to finish the patch over the > next month (it's already getting close), leaving 2 or 3 months for > additional testing after the merge. I see, these are very valid reasons. Eventually though the situation should settle down, at which point some squashing is going to be required anyways to ensure that the commit history stays atomic. I.e. no single commit should yield a semi-broken state, as that would totally destroy `git bisect` and similar workflows. Anyhow, enough of this, as it's really not the point of this thread as you are writing below :) > I don't want this discussion to turn into whether or not only my > reasons for wanting a higher commit limit/no limit are right, but I > recognize it is important to question my reasons since those are a > factor in the discussion. I totally agree with that. And just to make it clear again: I'm only trying to shine a light on how one could potentially handle such discussions differently. In the end it's you who does the work, and that counts infinitely more so don't let me deter you from your goals here! And as I said previously, I'm totally in favor of raising the commit limit, if possible. Cheers -- Milian Wolff http://milianw.de
Re: Loosening the commit limit for work branches
On Thu, Aug 25, 2022 at 2:16 AM Harald Sitter wrote: > On Wed, Aug 24, 2022 at 3:31 PM Noah Davis 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
Re: Loosening the commit limit for work branches
I didn't want to leave the master branch in a semi-broken state or present what I had come up with in an MR until I could present it as an improvement. It took a while to figure out what I wanted and whether some of what I wanted was even achievable. I haven't been working alone, so some people such as Marco and Nate are aware of the current state of the branch and have been testing it periodically. I'm aiming for a 22.12 release, so I intend to finish the patch over the next month (it's already getting close), leaving 2 or 3 months for additional testing after the merge. I don't want this discussion to turn into whether or not only my reasons for wanting a higher commit limit/no limit are right, but I recognize it is important to question my reasons since those are a factor in the discussion. On Wed, Aug 24, 2022 at 6:42 PM Milian Wolff wrote: > > On Mittwoch, 24. August 2022 17:26:33 CEST Noah Davis wrote: > > On Wed, Aug 24, 2022 at 5:12 PM Milian Wolff wrote: > > > Without any knowledge of your work on the QML port of Spectacle, I would > > > also claim that there is bound to be a lot of generic work that should be > > > possible to land directly in the main branch, no? You are probably > > > splitting up the data representation and UI layer, which can happen > > > early. Then you add a second UI implementation in tandem to the widget > > > one, which can be opt-in until it's ready. Once all is done, you can > > > remove the old widget UI. There's no need to wait a long time for all of > > > this to hit the main branch and work only in a feature branch, no? > > > > The UI is already fairly well separated from the backend simply > > because Spectacle already has a CLI. I simply went through a lot of > > iterations over the past few months in collaboration with Marco while > > trying to come up with the right UI/UX. The branch contains a lot of > > new UI related code that uses Qt Quick/QML. It would be useful for me > > to keep track of these changes so that if anything breaks in the > > process, I can more easily figure out which change did it and ask > > questions if I wasn't the one who made the change. > > Right, as I said I only worked on assumptions in my statement above. > > What prevents you though from merging the partial state of the Qt Quick/QML UI > into the main branch? If you say the code history as it is now is useful, it > should be useful in the future too - and as such could be merged more > regularly? You don't have to enable the new UI for now in the main branch? > > -- > Milian Wolff > m...@milianw.de > http://milianw.de
Re: Loosening the commit limit for work branches
On Mittwoch, 24. August 2022 17:26:33 CEST Noah Davis wrote: > On Wed, Aug 24, 2022 at 5:12 PM Milian Wolff wrote: > > Without any knowledge of your work on the QML port of Spectacle, I would > > also claim that there is bound to be a lot of generic work that should be > > possible to land directly in the main branch, no? You are probably > > splitting up the data representation and UI layer, which can happen > > early. Then you add a second UI implementation in tandem to the widget > > one, which can be opt-in until it's ready. Once all is done, you can > > remove the old widget UI. There's no need to wait a long time for all of > > this to hit the main branch and work only in a feature branch, no? > > The UI is already fairly well separated from the backend simply > because Spectacle already has a CLI. I simply went through a lot of > iterations over the past few months in collaboration with Marco while > trying to come up with the right UI/UX. The branch contains a lot of > new UI related code that uses Qt Quick/QML. It would be useful for me > to keep track of these changes so that if anything breaks in the > process, I can more easily figure out which change did it and ask > questions if I wasn't the one who made the change. Right, as I said I only worked on assumptions in my statement above. What prevents you though from merging the partial state of the Qt Quick/QML UI into the main branch? If you say the code history as it is now is useful, it should be useful in the future too - and as such could be merged more regularly? You don't have to enable the new UI for now in the main branch? -- Milian Wolff m...@milianw.de http://milianw.de signature.asc Description: This is a digitally signed message part.
Re: Loosening the commit limit for work branches
On Wed, Aug 24, 2022 at 5:12 PM Milian Wolff wrote: > Without any knowledge of your work on the QML port of Spectacle, I would also > claim that there is bound to be a lot of generic work that should be possible > to land directly in the main branch, no? You are probably splitting up the > data representation and UI layer, which can happen early. Then you add a > second UI implementation in tandem to the widget one, which can be opt-in > until it's ready. Once all is done, you can remove the old widget UI. There's > no need to wait a long time for all of this to hit the main branch and work > only in a feature branch, no? The UI is already fairly well separated from the backend simply because Spectacle already has a CLI. I simply went through a lot of iterations over the past few months in collaboration with Marco while trying to come up with the right UI/UX. The branch contains a lot of new UI related code that uses Qt Quick/QML. It would be useful for me to keep track of these changes so that if anything breaks in the process, I can more easily figure out which change did it and ask questions if I wasn't the one who made the change.
Re: Loosening the commit limit for work branches
On Mittwoch, 24. August 2022 15:30:06 CEST Noah Davis 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. > 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. I have also run into this but got done at around ~90ish patches and was notified by Nicolas in time about this limitation. Generally, such arbitrary limitations are always a bad sign imo and we should ideally try to remove or increase the limit. Lots of small patches are far easier to work with than few mega patches. That said, from my personal knowledge it's usually a bad sign to start mega rewrites or refactors in a branch - sooner or later you should hit a small mile stone that you can merge already. If you don't do that and pile up tons of patches in your work branch, you might get into rebase or merge conflict hell when the main branch continues to see development. I've been there and just started over, wasting quite a bit of time. Since then, I'm trying to look for small work packages that can be hoisted out of the larger work branches and merged early. This then allows to reduce the size of the work branch, without having to use large squashed mega patches. Without any knowledge of your work on the QML port of Spectacle, I would also claim that there is bound to be a lot of generic work that should be possible to land directly in the main branch, no? You are probably splitting up the data representation and UI layer, which can happen early. Then you add a second UI implementation in tandem to the widget one, which can be opt-in until it's ready. Once all is done, you can remove the old widget UI. There's no need to wait a long time for all of this to hit the main branch and work only in a feature branch, no? Cheers -- Milian Wolff m...@milianw.de http://milianw.de signature.asc Description: This is a digitally signed message part.
Re: Loosening the commit limit for work branches
On Wed, Aug 24, 2022 at 3:31 PM Noah Davis 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. > >
Loosening the commit limit for work branches
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
