[asterisk-dev] A Week with GIT/Gerrit

[Corey Farrell]

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.joseph at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150417/6a730ebb/attachment.html>