Hi everybody,
I was recently made aware that Gerrit now supports adding metadata to commit messages in the "rebase" strategy. The "cherry-pick" strategy that we're using has been chosen over everything else primarily due to this capability.
Some details: Gerrit supports different ways of committing changes into branches when developers submit them, with different effects on which changes are considered ready and how they show up in the commit history.
The two main contenders are "cherry-pick" and "rebase always".
Both create a linear history (unlike some "merge" strategies that can create merge commits. Both add metadata to the commits, all that "Reviewed-on", "Reviewed-by" etc in the commit message.
Cherry-pick considers every change on Gerrit on its own. Even if you push a patch train of 50 commits, it allows submitting commit #49 individually. That gives some flexibility (if these commits are truly independent) but also creates risk: if changes are merged out of order, we might break the tree with otherwise perfectly good commits, for example when a new API is used that is only added by a previous commit.
Rebase always considers patch trains: To submit commit #49, the commit itself and all its 48 ancestors need to be ready for submission. It becomes a one-click operation in the UI and ordering is honored. On the other hand, picking an individual commit is a manual operation now (cherry-pick to become its own patch train, push, re-review).
Speaking of UI, depending on the strategy the "readiness" marker in the UI changes behavior, too: With "cherry-pick", each commit is compared against the branch individually, with a "merge conflict" notification if the commit on its own would fail to merge. With "rebase always" the entire patch train up to that point is considered.
It's a matter of trade-offs, but given that "rebase always" can now add the metadata that was the deal breaker for anything but cherry-pick back in the day, I wanted to know how y'all feel about changing or keeping the submission strategy.
David proposed that we could try out "rebase always" for a while (maybe a month) to see how it feels.
Patrick
Patrick Georgi via coreboot wrote:
I was recently made aware that Gerrit now supports adding metadata to commit messages in the "rebase" strategy.
That's cool!
It's a matter of trade-offs, but given that "rebase always" can now add the metadata that was the deal breaker for anything but cherry-pick back in the day, I wanted to know how y'all feel about changing or keeping the submission strategy.
I find the benefits quite desirable.
David proposed that we could try out "rebase always" for a while (maybe a month) to see how it feels.
Good idea!
Thanks
//Peter
I like it. We do this on most of my other projects.
On Thu, Jul 14, 2022, 6:12 AM Peter Stuge peter@stuge.se wrote:
Patrick Georgi via coreboot wrote:
I was recently made aware that Gerrit now supports adding metadata to commit messages in the "rebase" strategy.
That's cool!
It's a matter of trade-offs, but given that "rebase always" can now add
the
metadata that was the deal breaker for anything but cherry-pick back in
the
day, I wanted to know how y'all feel about changing or keeping the submission strategy.
I find the benefits quite desirable.
David proposed that we could try out "rebase always" for a while (maybe a month) to see how it feels.
Good idea!
Thanks
//Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Hi again,
On 2022-07-13 I wrote:
David proposed that we could try out "rebase always" for a while (maybe a month) to see how it feels.
This idea has been positively received on the list and nobody voiced objection to such a change.
As a server admin I declare that August will be "'Rebase Always' test month".
On August 1, I'll change the coreboot repo to use that submission strategy and then we'll have a month to see how it works for us. Late August we can make a decision whether to stay with Rebase Always or to go back to Cherry-Pick.
Regards, Patrick
Hi,
even though I'm not completely convinced that the advantages of this new submit strategy will outweigh the disadvantages for me, I agree that it would be good to try the always rebase strategy for a month and see how well it'll work in practice.
Having a commit queue or even running a build test for every patch before it gets submitted would catch more breakages before the patches land in the tree than switching to the new submit strategy, but since we don't break the tree that often (maybe every 2-3 months and it gets fixed again quickly), that might not be worth the effort.
Regards, Felix
Felix Held wrote:
Having a commit queue or even running a build test for every patch before it gets submitted would catch more breakages before the patches land in the tree than switching to the new submit strategy,
Hmm, I'm not sure about that? I understand the effective difference with the new strategy to merely be that a later commit in a pushed changeset/branch can't be submitted before an earlier one. I think each commit is still build tested and verified?
And different pushed changesets continue to be build tested individually and can be submitted individually - right?
Hmm, what happens when commit 1/2 in changeset 1 is submitted and someone then wants to submit commit 1/2 in changeset 2 before the other commit in the first changeset? Let's try in August I guess. :)
I'm sure the change could be rolled back quickly in case there's any kind of annoying disaster problem in the repo.
Kind regards
//Peter
Hi Peter!
Hmm, I'm not sure about that? I understand the effective difference with the new strategy to merely be that a later commit in a pushed changeset/branch can't be submitted before an earlier one.
That is my understanding of this too. What I wanted to point out in my last email is that switching form cherry-pick to always rebase will only catch a small part of the things that resulted in a broken tree in the last year or two. The majority of tree breakages was caused by two independent patches doing something that won't result in a git conflict, but in compile failures. For example when some devicetree register name was changed in the chip.h file and all devicetrees that were in tree at the time and in a separate patch a new mainboard was introduced, but that mainboard was only build-tested on top of the tree before the devicetree change was submitted. Submitting both the board and the devicetree change won't cause a git conflict, but compiling the board will fail causing the tree to be broken. Since this is usually a quick fix that can be submitted via the fast patch with 3x +2, it's not too bad IMHO.
Cherry-pick is useful for longer patch trains where the earlier parts are independent, but some later part needs all the earlier parts. In that case some parts that are independent from the patches before them and are already reviewed and ready to be submitted would need to wait until the patches before them are submittable too. And yes, submitting patches out of order has already resulted in a broken tree in a very small percentage of cases, but most of the time it was also easy to fix.
The main advantage of the always rebase strategy is that with the currently used cherry-pick approach Gerrit shows a merge conflict on later patches in a patch train that depend on some earlier change until the previous patches are submitted, which is a bit confusing when you're not aware that this isn't some error to worry about. The always rebase strategy doesn't have this oddity.
I think each commit is still build tested and verified?
Not right before it gets submitted. It still gets build-tested like before when the patch is pushed to Gerrit and after it got submitted.
And different pushed changesets continue to be build tested individually and can be submitted individually - right?
Correct.
I'm sure the change could be rolled back quickly in case there's any kind of annoying disaster problem in the repo.
I don't expect the always rebase submit strategy to cause any major problem.
Regards, Felix
Hi everybody,
22. Juli 2022 16:56, "Patrick Georgi via coreboot" coreboot@coreboot.org schrieb:
On August 1, I'll change the coreboot repo to use that submission strategy and then we'll have a month to see how it works for us. Late August we can make a decision whether to stay with Rebase Always or to go back to Cherry-Pick.
This is now live, let's see how it goes.
(And if it's too horrible, it's trivial to change back at a moment's notice)
Regards, Patrick
Hi Patrick,
When going through the list of submittable patches i ran into a two problems and also got a question:
When a patch before other patches gets abandoned, those patches can no longer be submitted. Example: CB:66324
When submitting the first patch of a patch series, somehow the following patches now have merge conflicts even though they shouldn't have any merge conflict. Example: I submitted CB:66272 and now CB:66273 and following have merge conflicts which they didn't have before. I definitely want to submit a patch train patch by patch and not just the last submittable patches including all patches before to make sure that I looked at each individual patch when submitting.
When part of a patch train gets re-pushed due to some small fixes that won't affect patches after a certain patch and then all patches from the patch series are reviewed and submittable, how will the case that later patches are on top of outdated patches be handled? Previously the latest version of each individual patch was used which is the correct behavior, but I'm not sure if this is still the case with the new approach.
Especially the second problem is a dealbreaker for me. Not sure if this can be fixed by some configuration change; if not, I'd like to go back to the previously used cherry-pick submit method.
Regards, Felix
Felix Held wrote:
I definitely want to submit a patch train patch by patch and not just the last submittable patches including all patches before to make sure that I looked at each individual patch when submitting.
Maybe the patch train is too long if it becomes hard to keep an overview?
When part of a patch train gets re-pushed due to some small fixes that won't affect patches after a certain patch and then all patches from the patch series are reviewed and submittable, how will the case that later patches are on top of outdated patches be handled?
Hmm, I don't understand what you mean here..
If you have pushed a branch with 3 commits, use interactive rebase locally to fix up the middle commit and then push the branch again, doesn't Gerrit always recognize that the first commit remains unchanged, ie. with one generation less than the middle and last commit?
Previously the latest version of each individual patch was used which is the correct behavior,
If the patches aren't actually linked (ie. can be submitted individually) then I think they shouldn't be pushed as a single branch, right?
//Peter
Hi Peter,
I definitely want to submit a patch train patch by patch and not just the last submittable patches including all patches before to make sure that I looked at each individual patch when submitting.
Maybe the patch train is too long if it becomes hard to keep an overview?
I'd say that it always better to have to look at all the individual patches instead of just looking at the bigger picture like it's usually done in the Github workflow that is focused on whole branches instead of the individual patches. This is also why I strongly prefer the Gerrit workflow that focuses on individual patches.
When part of a patch train gets re-pushed due to some small fixes that won't affect patches after a certain patch and then all patches from the patch series are reviewed and submittable, how will the case that later patches are on top of outdated patches be handled?
Hmm, I don't understand what you mean here..
If you have pushed a branch with 3 commits, use interactive rebase locally to fix up the middle commit and then push the branch again, doesn't Gerrit always recognize that the first commit remains unchanged, ie. with one generation less than the middle and last commit?
To reduce unnecessary load on the build servers, it has become common practice to only push up to the changed patch and not the patches after that if the changes if the fixes are unrelated and there's more than maybe one patch after the one being updated. This is for example the case when fixing a typo in a commit message or comment in some commit that isn't right at the end of a patch train.
If the patches aren't actually linked (ie. can be submitted individually) then I think they shouldn't be pushed as a single branch, right?
Yes, but that's not the case I tried to outline; I hope that the text above makes it a bit more clear which case I was thinking of here.
Regards, Felix
Hi,
I reread the documentation and the thing I missed in there is that Gerrit will only automatically rebase a patch when submitting when none of the files it changed have been changed between the parent commit of the to be submitted patch and the current top of tree. At least this explains the difficulties I ran into so far; I was expecting Gerrit to always try a rebase and only require a manual rebase when there is a conflict it can't resolve on its own like it was the case before. This makes both treewide changes and submitting patch trains patch by patch more difficult.
Regards, Felix
Felix Held wrote:
Maybe the patch train is too long if it becomes hard to keep an overview?
I'd say that it always better to have to look at all the individual patches instead of just looking at the bigger picture
I agree completely, and don't want to suggest otherwise!
By "keep an overview" I mean to know which commits in a pushed branch that have been reviewed and which not.
If you have pushed a branch with 3 commits, use interactive rebase locally to fix up the middle commit and then push the branch again, doesn't Gerrit always recognize that the first commit remains unchanged, ie. with one generation less than the middle and last commit?
To reduce unnecessary load on the build servers, it has become common practice to only push up to the changed patch and not the patches after
I don't understand this at all..?
I'm all for skipping unneeded build load, but pushing only commits 1 and 2 in my case would still create build load (because commit 2 is new), right?
Later pushing 3 will also always create build load, because that too is a new commit.
What am I missing here?
that if the changes if the fixes are unrelated and there's more than maybe one patch after the one being updated.
..
If the patches aren't actually linked (ie. can be submitted individually) then I think they shouldn't be pushed as a single branch, right?
Yes, but that's not the case I tried to outline;
Hm, but "fixes are unrelated" seems to describe exactly this case?
Felix Held wrote:
I reread the documentation and the thing I missed in there is that Gerrit will only automatically rebase a patch when submitting when none of the files it changed have been changed between the parent commit of the to be submitted patch and the current top of tree.
That seems like an improvement to me. By the way, I don't think Gerrit has ever actually rebased anything, nor will it with the new setting, I understand it to always do an equivalent of "git cherry-pick" but with different requirements with the new strategy.
This makes both treewide changes and submitting patch trains patch by patch more difficult.
Hmm, how so? Submitting one commit at a time should make top of tree always match the parent commit of the to-be-submitted commit, right?
Kind regards
//Peter
Hi Peter,
By "keep an overview" I mean to know which commits in a pushed branch that have been reviewed and which not.
Ah, ok; I'd assume that Gerrit won't allow pushing a series of patches if one patch in there doesn't have +2 and +1 verified yet. Haven't verified this though.
I'm all for skipping unneeded build load [...] What am I missing here?
Let's assume that we have a patch train with 10 patches and during the review a typo fix in the first patch is requested and all other patches got +2s without any change requests. Until now the solution was to update the first patch and only push the first patch with the typo fix and just leave the following 9 patches at their initial version, so Jenkins won't build those 9 patches again. When re-pushing the full patch train, the other 9 patches will get build tested again after the unnecessary update and again when the patches get submitted. I don't think that this is a huge problem, but still something to think about if we want to change this from what we've done in the recent past.
I reread the documentation and the thing I missed in there is that Gerrit will only automatically rebase a patch when submitting when none of the files it changed have been changed between the parent commit of the to be submitted patch and the current top of tree.
That seems like an improvement to me. By the way, I don't think Gerrit has ever actually rebased anything, nor will it with the new setting, I understand it to always do an equivalent of "git cherry-pick" but with different requirements with the new strategy.
Rebases have always been manual, but before it didn't matter too much, since the cherry-pick strategy just tries to cherry-pick a commit and only requires a manual rebase when there's a git conflict that makes the cherry-pick fail. Now it requires a manual rebase when any of the files the commit changes have been changed in the meantime and not only when there are git conflicts, which happens way more often; especially when there are tree-wide changes or in the case of patch trains where the patches change the same files; see my next answer on the latter.
Hmm, how so? Submitting one commit at a time should make top of tree always match the parent commit of the to-be-submitted commit, right?
That was my assumption, but that turned out to not be the case which was the main reason for my first email today. Gerrit adds some lines to the commit message which will change the git hash; maybe that's part of the reason?
There was a patch train with at least the first few commits being submittable and after i submitted the first patch, Gerrit now shows merge conflicts on all following patches that should still be mergeable which is a dealbreaker for me. If you want to have a look yourself: CB:66272 was the first patch i submitted and now CB:66273 and following suddenly have merge conflicts which they didn't have before.
Regards, Felix
Felix, If you've formed an opinion, maybe you want to take a break from submitting for a bit to let others test out the new submission strategy?
For everyone else, I'd like to note that over the past year, Felix has hit submit on more than 45% of all of the coreboot patches that have been merged. The next highest submitter merged just over 10% and the person after that merged 5.5%. I'd weigh Felix's opinions on this matter very strongly.
If we continue with this method (and for the rest of this month), I think we want to encourage shorter patch trains, consisting of only related patches. Currently, it seems like projects will occasionally build up enormous trains of patches that aren't necessarily related so that they can pull all of their patches at once just by grabbing the last one.
It also sounds like rebasing all of the patches so that the entire train is contiguous is really needed - no more patches based on older versions of the previous patch. I think the same is true for abandoned patches - rebase everything so that the patch train isn't broken in the middle. Martin Aug 2, 2022, 14:02 by felix-coreboot@felixheld.de:
Hi Peter,
By "keep an overview" I mean to know which commits in a pushed branch that have been reviewed and which not.
Ah, ok; I'd assume that Gerrit won't allow pushing a series of patches if one patch in there doesn't have +2 and +1 verified yet. Haven't verified this though.
I'm all for skipping unneeded build load [...] What am I missing here?
Let's assume that we have a patch train with 10 patches and during the review a typo fix in the first patch is requested and all other patches got +2s without any change requests. Until now the solution was to update the first patch and only push the first patch with the typo fix and just leave the following 9 patches at their initial version, so Jenkins won't build those 9 patches again. When re-pushing the full patch train, the other 9 patches will get build tested again after the unnecessary update and again when the patches get submitted. I don't think that this is a huge problem, but still something to think about if we want to change this from what we've done in the recent past.
I reread the documentation and the thing I missed in there is that Gerrit will only automatically rebase a patch when submitting when none of the files it changed have been changed between the parent commit of the to be submitted patch and the current top of tree.
That seems like an improvement to me. By the way, I don't think Gerrit has ever actually rebased anything, nor will it with the new setting, I understand it to always do an equivalent of "git cherry-pick" but with different requirements with the new strategy.
Rebases have always been manual, but before it didn't matter too much, since the cherry-pick strategy just tries to cherry-pick a commit and only requires a manual rebase when there's a git conflict that makes the cherry-pick fail. Now it requires a manual rebase when any of the files the commit changes have been changed in the meantime and not only when there are git conflicts, which happens way more often; especially when there are tree-wide changes or in the case of patch trains where the patches change the same files; see my next answer on the latter.
Hmm, how so? Submitting one commit at a time should make top of tree always match the parent commit of the to-be-submitted commit, right?
That was my assumption, but that turned out to not be the case which was the main reason for my first email today. Gerrit adds some lines to the commit message which will change the git hash; maybe that's part of the reason?
There was a patch train with at least the first few commits being submittable and after i submitted the first patch, Gerrit now shows merge conflicts on all following patches that should still be mergeable which is a dealbreaker for me. If you want to have a look yourself: CB:66272 was the first patch i submitted and now CB:66273 and following suddenly have merge conflicts which they didn't have before.
Regards, Felix _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Martin Roth via coreboot wrote:
If we continue with this method (and for the rest of this month), I think we want to encourage shorter patch trains, consisting of only related patches.
Ouch! Yes please, for sure, regardless of the submit strategy.
Currently, it seems like projects will occasionally build up enormous trains of patches that aren't necessarily related so that they can pull all of their patches at once just by grabbing the last one.
I can understand such a desire, it does simplify things for the pushing developer and their testers, but then that shouldn't be done in upstream Gerrit.
I guess for contributors who can only commit lots of unrelated changes to a single internal branch (e.g. because they don't use git internally) would have an extra TODO in the export to git commits. :\
It's of course unfortunate to require that extra work but I find the benefit for the community significant!
The different "topics" in that train of commits become directly visible and can receive individual outside engagement much easier. I think that's unlikely to happen with commits in the middle of a long branch.
On a personal note I find unrelated commits in a branch quite confusing, others can of course be different there! :)
It also sounds like rebasing all of the patches so that the entire train is contiguous is really needed - no more patches based on older versions of the previous patch. I think the same is true for abandoned patches - rebase everything so that the patch train isn't broken in the middle.
May be - but that shouldn't be too bad?
I agree that it would be great for Jenkins to recognize when new commits are effectively unchanged since last test so that many "rebase builds" can be skipped but I don't have a great suggestion for how to make it happen. Sorry. :\
Kind regards
//Peter
Hi Martin and Peter,
sure, I can let other developers with submit right try out the new submit strategy and not submit anything that's not directly related to what I'm working on right now. I mainly started looking through the submittable patches and submit them when I don't spot anything that should be changed, since noone else was doing that on a regular basis maybe 2.5 years ago and some patches that could be submitted were just sitting in Gerrit for more than a few days. I wouldn't mind if more submitters would regularly look through the submittable patches and send the patches upstream. Please look at the code of the patch first though even though it already has a +2; sometimes you'll catch some bugs or see some improvements for later.
enormous trains of patches that aren't necessarily related so that they can pull all of their patches at once just by grabbing the last one.
At least for me getting all patches with just checking out the last commit isn't the reason why I occasionally push rather long patch trains. For me the reason is that for example when adding support for a new SoC there will be mostly short patch trains for different unrelated features, but pushing those different short patch trains isn't a good option, since despite working on different features there will be a lot of merge conflicts between the different short branches, especially in the Kconfig and Makefile. When all separate patch trains get reviewed and are submittable, you can likely only submit exactly one branch and then need to manually rebase the other branches, repush those and ask the reviewers to get back the +2 the patches already had and basically do that after every short branch got submitted. So rebasing the different short feature branches into one branch to push to Gerrit makes this much easier for both the developers and the reviewers. In cases where this isn't a problem, I of course try to only have directly related patches in one patch train. I agree that patch trains with more than 20 patches might become a bit confusing and a bit of a pain to deal with, so it's always good to try to stay below that. In the last few years I've only seen very few patch trains that were significantly longer and those were usually related patches.
It also sounds like rebasing all of the patches so that the entire train is contiguous is really needed - no more patches based on older versions of the previous patch. I think the same is true for abandoned patches - rebase everything so that the patch train isn't broken in the middle.
May be - but that shouldn't be too bad?
I agree that it would be great for Jenkins to recognize when new commits are effectively unchanged since last test so that many "rebase builds" can be skipped but I don't have a great suggestion for how to make it happen. Sorry. :\
The additional workload for Jenkins is my main concern here although it's also a bit annoying to need to do the extra rebases. This is especially the case when you're the reviewer who gave the patch a +2 and want to submit it after the 24h after the last significant change; in that case you'd either have someone else to do the rebase or find someone else to give the patch another +2, since after doing a rebase Gerrit considers the developer who did the rebase as new author and authors aren't allowed to +2 their own changes and so Gerrit will ignore this +2.
With the knowledge that when there is a patch train that is all submittable, the submit together with previous patches button on the last submittable patch in the train should be used to not need to rebase most of the patches after the previous patch got submittet, the rebases might not be as much of a problem than I initially thought. I'm certainly more used to how things worked before, so this has a bit of a learning curve for me, but I hope that writing about the things I ran into might help others to not run into those themselves.
Regards, Felix
Felix Held wrote:
I can let other developers with submit right try out the new submit strategy and not submit anything that's not directly related to what I'm working on right now.
I want to apologize if it seemed that I asked you to hold back on any effort, especially some not directly related to day-to-day tasks, I did not mean anything like that.
I understood Martin's point to be that you spend a lot of time working with Gerrit so have great insight, which I agree with.
That said, of course it would be great if more developers try the new submit strategy and see how they feel about it.
At least for me getting all patches with just checking out the last commit isn't the reason why I occasionally push rather long patch trains. For me the reason is that for example when adding support for a new SoC there will be mostly short patch trains for different unrelated features, but pushing those different short patch trains isn't a good option, since despite working on different features there will be a lot of merge conflicts between the different short branches, especially in the Kconfig and Makefile.
Nod, that's very understandable. For this, do you think it could work to not push everything at once, but only one "topic" at a time?
In the local repo it would be still be one long series of commits, but each "topic" could have its own branch name locally, although there is no branch in the commit graph. A bit like using tags, but with branch names so that rebase works well locally.
The workflow would then be to push the first "topic" (short patch train) to Gerrit, have that enter master, pull and then rebase the following local branches onto master, push next "topic" and so on.
This has both benefits and drawbacks; a benefit is that there are more frequent but much smaller patch sets to review, a drawback is that each patch set requires one "Gerrit turnaround" which for a large project could take a while.
The additional workload for Jenkins is my main concern here although it's also a bit annoying to need to do the extra rebases.
This would be mitigated by only pushing smaller patch sets at a time.
the submit together with previous patches button on the last submittable patch in the train should be used to not need to rebase most of the patches after the previous patch got submittet, the rebases might not be as much of a problem than I initially thought.
That's a good point - but that only applies to the "perfect patch set" case, right? It's cool if we can optimize that but I wouldn't want to make things a lot worse for patch sets needing some turnarounds.
I hope that writing about the things I ran into might help others to not run into those themselves.
Thanks a lot for that!
Kind regards
//Peter
Hi Peter,
I want to apologize if it seemed that I asked you to hold back on any effort, especially some not directly related to day-to-day tasks, I did not mean anything like that. [...]
That said, of course it would be great if more developers try the new submit strategy and see how they feel about it.
No worries, that's not how I interpreted your emails. Like Martin said, it would be good if more developers with submit rights would try the new submit strategy and I'd also appreciate if more submitters would regularly go through the submittable patches, do another quick review and if it's all good submit the patches; otherwise request some change. I don't mind doing this regularly, but it's not too great if submittable patches will just sit there for a week or two when I'm out of office or too busy with work. This has already been the case a few times.
Nod, that's very understandable. For this, do you think it could work to not push everything at once, but only one "topic" at a time? [...]
The workflow would then be to push the first "topic" (short patch train) to Gerrit, have that enter master, pull and then rebase the following local branches onto master, push next "topic" and so on.
This has both benefits and drawbacks; a benefit is that there are more frequent but much smaller patch sets to review, a drawback is that each patch set requires one "Gerrit turnaround" which for a large project could take a while.
The drawback of more Gerrit 24h turnarounds is at least to me a very significant one. The 24h turnaround is a good compromise between still keeping things moving relatively fast and giving more developers a chance to review things, so I consider this to be a good thing. It is however necessary to work with this in a way that doesn't slow down the development and upstreaming too much.
When upstreaming some longer patch train or directly developing new SoC support on upstream, I try to not have more than 2-3 topics in one patch train, but that allows me to upstream things at a similar rate as I write code and the upstreaming won't stall the development. When the upstreaming is far ahead of the development an additional problem would be that some requested changes in the review might affect all local patches that aren't in review yet which makes it more difficult to address requests in the review. Also when developing directly on upstream, I don't want to hold back too many patches locally to have a backup of what I already wrote.
the submit together with previous patches button on the last submittable patch in the train should be used to not need to rebase most of the patches after the previous patch got submittet, the rebases might not be as much of a problem than I initially thought.
That's a good point - but that only applies to the "perfect patch set" case, right? It's cool if we can optimize that but I wouldn't want to make things a lot worse for patch sets needing some turnarounds.
I'd guess that this only applies to the cases where the full patch train has been pushed and not only partial repushes after fixes. Haven't verified this though.
In cases where the rebases are mostly trivial and only the surroundings of the added or changed code have changed due to another patch train being submitted, I probably wouldn't consider the rebase a change that would require restarting the 24h cool-down time, but for a non-trivial rebase, I'd likely wait another 24h after the non-trivial rebase. I try to push things forward quite a bit, but still don't want to do this at the expense of possibly missing bugs that could have been caught.
What if Jenkins does not build until human has given +2 ?
That would make it much more difficult to see what should be reviewed. Right now a Jenkins +1 is a good indicator for me that the code is ready to be reviewed; if Jenkins doesn't like a patch, I don't think that a reviewer should already spend much time on already reviewing it. So at least for me the Jenkins +1 is rather important information before I start reviewing patches.
Regards, Felix
Felix Held wrote:
The additional workload for Jenkins is my main concern here
I understand that it is helpful to make also quite long patch trains with multiple "topics" visible in Gerrit early, for context and for discussion.
I had an idea today:
What if Jenkins does not build until human has given +2 ?
//Peter
-
Felix Held -
Martin Roth -
Patrick Georgi -
Patrick Georgi -
Peter Stuge -
ron minnich