On Sat, Apr 18, 2015 at 8:26 PM, Matthew Jordan <[email protected]> wrote: > On Thu, Apr 16, 2015 at 5:00 PM, George Joseph > <[email protected]> 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 under the Asterisk menu. -- 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
