[asterisk-dev] A Week with GIT/Gerrit

[Matthew Jordan]

On Sat, Apr 18, 2015 at 8:26 PM, Matthew Jordan <mjordan at digium.com> wrote:
> 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.

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