[asterisk-dev] A Week with GIT/Gerrit

[Matthew Jordan]

On Thu, Apr 16, 2015 at 5: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.

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