Re: [asterisk-dev] A Week with GIT/Gerrit
On Sat, Apr 18, 2015 at 8:26 PM, Matthew Jordan mjor...@digium.com wrote: On Thu, Apr 16, 2015 at 5:00 PM, George Joseph george.jos...@fairview5.com wrote: The Emails: Overall I think they're too verbose. Change in asterisk[master]: bridge.c: NULL app causes crash during attended transfer might be better as bridge.c: NULL app causes crash during attended transfer (asterisk[master]) It's not a lot shorter but it has the most valuable info at the front. That's pretty easy. This should now be fixed. Also, the mail generated by ReplacePatchSet which gets sent on a rebase, doesn't tell you why you're getting the message but it does have the entire original commit message which doesn't really help. I'd remove the commit message and add the text of the last change. I.E. Foo Bar: Patch Set 8: Patch Set 7 was rebased. That'll at least tell reviewers not to worry about it. I think that makes some assumptions about why a patch set was replaced. It doesn't just occur during a rebase - it could just as easily occur due to a decrease/increase in the patch scope from comments on the previous patch set, in which case reading the text of the new commit message may be useful. Reading through your suggestion, I'm not sure what in the e-mail templates would give what you're looking for. The ReplacePatchSet does include the number of the new patch set, as well as the change subject - but inferring why a patch set was updated doesn't feel possible. Removing (Code Review) from the sender would be good as well. I can do that. This should now be fixed as well. Cherry-picking: I think there are some issues we still need to figure out about the process and the system. Process first... While having all the reviews up at the same time is a good thing, I think we need to always start with the lowest branch the patch is expected to apply to and merge up towards master. I think the wiki says merge down for features and merge up for bugs. It should be up always. You don't want to start a feature in master only to discover that it can't go into 13 because some other feature is only in master. Same for review process. Start in the lowest branch and work up. This will hopefully minimize where different people are starting from different directions. It's hard to follow feedback and ensure you've got all the comments covered like that. Generally, I agree with starting the review on the lowest branch and working up. If a patch is substantially different enough between major versions that may not always work, but I could see people focussing on master and forgetting to properly review the release branches - which is where the most attention should generally be spent. Like Corey, I'm not sure we can always say cherry-pick up. New features feel like they *must* be written for master first, then cherry-picked back as appropriate. Something I've noticed though is that dependencies get messed up when cherry-picking a series of dependent patches. Look at my patches 46, 47, 48 for master. If I cherry-pick 48 to my local repo, I get all 3 which is good because I can't test any without all 3. BUT now look at 43, 44, 45 for 13. When I check out 45, I get only 45 which is an issue. Maybe this is just an artifact of this being the first time with this scenario but it's something to look out for. Other things: There's a rebase button on reviews which need it but if you click it, you lose you're votes. :( We probably need a policy around rebasing reviews. The +1's may go away on the current review, but they do still show up in the history. If someone has a +1 and has to rebase for the final merge, I don't think we need to go through another cycle. I've noticed a couple of times where doing a 'git review' regenerated the Change ID and I can't figure out why. I've resorted to running 'git review -n' to see the commands and just running the 'git push' part. I'm still investigating. If you move between major releases you'll find various orphaned files and directories that are part of one release but not antother. Use 'git clean -fd' to clean them up. BEWARE: this will remove all files/directories that aren't part of the current branch or in the .gitignore files. If you have local scripts or other stuff you want to keep between checkouts, add them to .git/info/exclude. If you're a Google Apps/Gmail user use the filter list:gerrit.asterisk.org to filter all messages regardless of repo. If you're switching between branches with the same major version a lot, install ccache! You're compiles will go a lot faster. When doing a 'git review' to update a review, you don't have to specify the topic branch again but you DO have to specify the target branch if it's not master. We could still use the links to the specific git/gerrit wiki pages on the gerrit menu. I'll get that taken care of this weekend as well. This is now the first link
Re: [asterisk-dev] A Week with GIT/Gerrit
On Fri, Apr 17, 2015 at 3:42 PM, Corey Farrell g...@cfware.com wrote: My additions to the list: 1) Procedure for 'git review' of security related patches. I think this could be done with an asterisk-security mirror repository setup in gerrit with restricted access. I know this is already being thought about, just wanted to make sure it on the list. I think there's a lot of things to think through with this, since Gerrit - being a Git repo - tends to expose all patches in some form or fashion. We could set up a completely separate server with extremely limited access, but then we would need a separate review server as well. In the meantime, we might be able to use review board for this, although it will mean porting it over to look at the Git repos. It's certainly on the list to think through more. 2) Is there a command similar to 'git show-branch 13 master' that will operate on Change-Id's? Something to show me the first line of all commits with change-id's that hit one branch but not the other. 3) It would be great if gerrit had the ability to detect the string ASTERISK-# in commit messages and review comments, convert it to a JIRA link. Apparently this is possible and is actually just a configuration issue: http://gerrit.googlecode.com/svn/documentation/2.2.1/config-gerrit.html#_a_id_commentlink_a_section_commentlink I'll try to get that set up this week. 4) 'git review -D' should not generate emails or allow the review to be seen on the web UI by anyone except the submitter. I know this is probably a bug in gerrit, but still worth having on the list. I will say that the last time there was a draft, it did show on the UI as a Draft. I upgraded to 2.10.3.1 this weekend - maybe we'll get lucky and it got fixed? :-) 5) We should add instructions for squashing commits before review somewhere on the Git or Gerrit usage page. Ack. I'll try to get to that today. Maybe on the Git usage or Commit message page, since it isn't explicitly tied to using Gerrit? -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] A Week with GIT/Gerrit
On Thu, Apr 16, 2015 at 5:00 PM, George Joseph george.jos...@fairview5.com wrote: The Emails: Overall I think they're too verbose. Change in asterisk[master]: bridge.c: NULL app causes crash during attended transfer might be better as bridge.c: NULL app causes crash during attended transfer (asterisk[master]) It's not a lot shorter but it has the most valuable info at the front. That's pretty easy. Also, the mail generated by ReplacePatchSet which gets sent on a rebase, doesn't tell you why you're getting the message but it does have the entire original commit message which doesn't really help. I'd remove the commit message and add the text of the last change. I.E. Foo Bar: Patch Set 8: Patch Set 7 was rebased. That'll at least tell reviewers not to worry about it. I think that makes some assumptions about why a patch set was replaced. It doesn't just occur during a rebase - it could just as easily occur due to a decrease/increase in the patch scope from comments on the previous patch set, in which case reading the text of the new commit message may be useful. Reading through your suggestion, I'm not sure what in the e-mail templates would give what you're looking for. The ReplacePatchSet does include the number of the new patch set, as well as the change subject - but inferring why a patch set was updated doesn't feel possible. Removing (Code Review) from the sender would be good as well. I can do that. Cherry-picking: I think there are some issues we still need to figure out about the process and the system. Process first... While having all the reviews up at the same time is a good thing, I think we need to always start with the lowest branch the patch is expected to apply to and merge up towards master. I think the wiki says merge down for features and merge up for bugs. It should be up always. You don't want to start a feature in master only to discover that it can't go into 13 because some other feature is only in master. Same for review process. Start in the lowest branch and work up. This will hopefully minimize where different people are starting from different directions. It's hard to follow feedback and ensure you've got all the comments covered like that. Generally, I agree with starting the review on the lowest branch and working up. If a patch is substantially different enough between major versions that may not always work, but I could see people focussing on master and forgetting to properly review the release branches - which is where the most attention should generally be spent. Like Corey, I'm not sure we can always say cherry-pick up. New features feel like they *must* be written for master first, then cherry-picked back as appropriate. Something I've noticed though is that dependencies get messed up when cherry-picking a series of dependent patches. Look at my patches 46, 47, 48 for master. If I cherry-pick 48 to my local repo, I get all 3 which is good because I can't test any without all 3. BUT now look at 43, 44, 45 for 13. When I check out 45, I get only 45 which is an issue. Maybe this is just an artifact of this being the first time with this scenario but it's something to look out for. Other things: There's a rebase button on reviews which need it but if you click it, you lose you're votes. :( We probably need a policy around rebasing reviews. The +1's may go away on the current review, but they do still show up in the history. If someone has a +1 and has to rebase for the final merge, I don't think we need to go through another cycle. I've noticed a couple of times where doing a 'git review' regenerated the Change ID and I can't figure out why. I've resorted to running 'git review -n' to see the commands and just running the 'git push' part. I'm still investigating. If you move between major releases you'll find various orphaned files and directories that are part of one release but not antother. Use 'git clean -fd' to clean them up. BEWARE: this will remove all files/directories that aren't part of the current branch or in the .gitignore files. If you have local scripts or other stuff you want to keep between checkouts, add them to .git/info/exclude. If you're a Google Apps/Gmail user use the filter list:gerrit.asterisk.org to filter all messages regardless of repo. If you're switching between branches with the same major version a lot, install ccache! You're compiles will go a lot faster. When doing a 'git review' to update a review, you don't have to specify the topic branch again but you DO have to specify the target branch if it's not master. We could still use the links to the specific git/gerrit wiki pages on the gerrit menu. I'll get that taken care of this weekend as well. -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org --
Re: [asterisk-dev] A Week with GIT/Gerrit
My additions to the list: 1) Procedure for 'git review' of security related patches. I think this could be done with an asterisk-security mirror repository setup in gerrit with restricted access. I know this is already being thought about, just wanted to make sure it on the list. 2) Is there a command similar to 'git show-branch 13 master' that will operate on Change-Id's? Something to show me the first line of all commits with change-id's that hit one branch but not the other. 3) It would be great if gerrit had the ability to detect the string ASTERISK-# in commit messages and review comments, convert it to a JIRA link. 4) 'git review -D' should not generate emails or allow the review to be seen on the web UI by anyone except the submitter. I know this is probably a bug in gerrit, but still worth having on the list. 5) We should add instructions for squashing commits before review somewhere on the Git or Gerrit usage page. On Thu, Apr 16, 2015 at 6:00 PM, George Joseph george.jos...@fairview5.com wrote: The Emails: Overall I think they're too verbose. Change in asterisk[master]: bridge.c: NULL app causes crash during attended transfer might be better as bridge.c: NULL app causes crash during attended transfer (asterisk[master]) It's not a lot shorter but it has the most valuable info at the front. +1, this would be great. I'd also like to vote for removing the mailing list name from subjects on asterisk-code-review. The list is high enough volume that any subscribers will have a rule putting all the messages in a folder, so we don't need the list name repeated in every subject. While having all the reviews up at the same time is a good thing, I think we need to always start with the lowest branch the patch is expected to apply to and merge up towards master. I think the wiki says merge down for features and merge up for bugs. It should be up always. You don't want to start a feature in master only to discover that it can't go into 13 because some other feature is only in master. Same for review process. Start in the lowest branch and work up. This will hopefully minimize where different people are starting from different directions. It's hard to follow feedback and ensure you've got all the comments covered like that. I disagree with the argument for merging up features. I'd prefer all new features be coded with the best API's of master. This makes optimized for master the default position of new features. Getting new features back-ported to 13 can be done if wanted, possible, well tested and compatible. I do agree with you about reviewing, always start with the oldest posted branch. If someone disapproves of the patch against 11, it seems reasonable to assume they also disapprove of the patch for 13 and master. For verification of testing, can we combine the Asterisk Testsuite - Full and Asterisk Unit Tests Bamboo jobs? The goal is to get a single coverage report showing data for as many tests as possible. It would be really nice if we had a permanent URL for browsing coverage reports of each branch. This is related to the question of new features in existing branches. If Bamboo says we don't have good coverage of a new feature I think that's enough reason to pull it from released branches, but to take that position we need Bamboo to produce a coverage report based on all available tests. There's a rebase button on reviews which need it but if you click it, you lose you're votes. :( We probably need a policy around rebasing reviews. I have mixed feelings on loss of votes after rebase. I think it's correct for the +1 votes to drop, but the -1 votes should be kept. The emails sent out should make it clear in the first line of the body that it's just a rebase. This way the reviewer can decide if the rebase could have introduced breakage knowing that the patch itself was unchanged. Once we have functional jenkins we might want it so all votes were left alone on rebase, and jenkins could just -1 if the rebase broke something. If you move between major releases you'll find various orphaned files and directories that are part of one release but not antother. Use 'git clean -fd' to clean them up. BEWARE: this will remove all files/directories that aren't part of the current branch or in the .gitignore files. If you have local scripts or other stuff you want to keep between checkouts, add them to .git/info/exclude. You can also use 'git clean -fxd' to clean all files, including those which are ignored. That is if you don't keep any files in the Asterisk source folder. One exception I've noticed, if you clone another repository into the Asterisk source dir, it will be untouched by 'git clean' from the Asterisk source dir. -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit:
[asterisk-dev] A Week with GIT/Gerrit
The Emails: Overall I think they're too verbose. Change in asterisk[master]: bridge.c: NULL app causes crash during attended transfer might be better as bridge.c: NULL app causes crash during attended transfer (asterisk[master]) It's not a lot shorter but it has the most valuable info at the front. Also, the mail generated by ReplacePatchSet which gets sent on a rebase, doesn't tell you why you're getting the message but it does have the entire original commit message which doesn't really help. I'd remove the commit message and add the text of the last change. I.E. Foo Bar: Patch Set 8: Patch Set 7 was rebased. That'll at least tell reviewers not to worry about it. Removing (Code Review) from the sender would be good as well. Cherry-picking: I think there are some issues we still need to figure out about the process and the system. Process first... While having all the reviews up at the same time is a good thing, I think we need to always start with the lowest branch the patch is expected to apply to and merge up towards master. I think the wiki says merge down for features and merge up for bugs. It should be up always. You don't want to start a feature in master only to discover that it can't go into 13 because some other feature is only in master. Same for review process. Start in the lowest branch and work up. This will hopefully minimize where different people are starting from different directions. It's hard to follow feedback and ensure you've got all the comments covered like that. Something I've noticed though is that dependencies get messed up when cherry-picking a series of dependent patches. Look at my patches 46, 47, 48 for master. If I cherry-pick 48 to my local repo, I get all 3 which is good because I can't test any without all 3. BUT now look at 43, 44, 45 for 13. When I check out 45, I get only 45 which is an issue. Maybe this is just an artifact of this being the first time with this scenario but it's something to look out for. Other things: There's a rebase button on reviews which need it but if you click it, you lose you're votes. :( We probably need a policy around rebasing reviews. I've noticed a couple of times where doing a 'git review' regenerated the Change ID and I can't figure out why. I've resorted to running 'git review -n' to see the commands and just running the 'git push' part. I'm still investigating. If you move between major releases you'll find various orphaned files and directories that are part of one release but not antother. Use 'git clean -fd' to clean them up. BEWARE: this will remove all files/directories that aren't part of the current branch or in the .gitignore files. If you have local scripts or other stuff you want to keep between checkouts, add them to .git/info/exclude. If you're a Google Apps/Gmail user use the filter list:gerrit.asterisk.org to filter all messages regardless of repo. If you're switching between branches with the same major version a lot, install ccache! You're compiles will go a lot faster. When doing a 'git review' to update a review, you don't have to specify the topic branch again but you DO have to specify the target branch if it's not master. We could still use the links to the specific git/gerrit wiki pages on the gerrit menu. I think that's it for now. -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] A Week with GIT/Gerrit
On Thu, Apr 16, 2015 at 6:00 PM, George Joseph george.jos...@fairview5.com wrote: Something I've noticed though is that dependencies get messed up when cherry-picking a series of dependent patches. Look at my patches 46, 47, 48 for master. If I cherry-pick 48 to my local repo, I get all 3 which is good because I can't test any without all 3. BUT now look at 43, 44, 45 for 13. When I check out 45, I get only 45 which is an issue. Maybe this is just an artifact of this being the first time with this scenario but it's something to look out for. This is likely because for 13, the changes were cherry picked individually as changes against HEAD of 13. They need to be set up as 3 commits in a series on top of 13 and submitted that way. Backporting in that case will need to be done locally to preserve those dependencies instead of clicking the cherry-pick buttons. -- Russell Bryant -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] A Week with GIT/Gerrit
On Thu, Apr 16, 2015 at 4:11 PM, Russell Bryant russ...@russellbryant.net wrote: On Thu, Apr 16, 2015 at 6:00 PM, George Joseph george.jos...@fairview5.com wrote: Something I've noticed though is that dependencies get messed up when cherry-picking a series of dependent patches. Look at my patches 46, 47, 48 for master. If I cherry-pick 48 to my local repo, I get all 3 which is good because I can't test any without all 3. BUT now look at 43, 44, 45 for 13. When I check out 45, I get only 45 which is an issue. Maybe this is just an artifact of this being the first time with this scenario but it's something to look out for. This is likely because for 13, the changes were cherry picked individually as changes against HEAD of 13. They need to be set up as 3 commits in a series on top of 13 and submitted that way. Backporting in that case will need to be done locally to preserve those dependencies instead of clicking the cherry-pick buttons. Ah, that makes sense now. We should probably note that on the wiki. -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
