<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">My additions to the list:<br>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.<br>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.<br></div><div class="gmail_quote">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.<br></div><div class="gmail_quote">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.<br></div><div class="gmail_quote">5) We should add instructions for squashing commits before review somewhere on the Git or Gerrit usage page.<br></div><div class="gmail_quote"><br><br>On Thu, Apr 16, 2015 at 6:00 PM, George Joseph <span dir="ltr"><<a href="mailto:george.joseph@fairview5.com" target="_blank">george.joseph@fairview5.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div>The Emails:<div><br><div>Overall I think they're too verbose.<br><div>"Change in asterisk[master]: bridge.c: NULL app causes crash during attended transfer"<br></div></div><div>might be better as </div><div>"bridge.c: NULL app causes crash during attended transfer (asterisk[master])"<br></div><div>It's not a lot shorter but it has the most valuable info at the front.</div></div></div></blockquote><div><br>+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.<br><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div><span style="color:rgb(53,53,53);font-family:sans-serif">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.</span></div></div></blockquote><div><br>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.<br><br>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.<br><br>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.<br><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><font face="sans-serif" color="#353535">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.</font></div></div></blockquote><div><br></div><div>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.<br><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><span style="color:rgb(53,53,53);font-family:sans-serif">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.</span><br></div></div></blockquote><div><br></div><div>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.<br><br></div></div></div></div>