Wireshark · Wireshark-dev: Re: [Wireshark-dev] Proposed Gerrit workflow (was: Re: Notes from Sharkfest '13)

[Bálint Réczey <balint@xxxxxxxxxxxxxxx>]

Hi Graham,

Thank you for your detailed and insightful comment.

2013/6/24 Graham Bloice <graham.bloice@xxxxxxxxxxxxx>:
> On 23 June 2013 11:58, Bálint Réczey <balint@xxxxxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> 2013/6/22 Marc Petit-Huguenin <marc@xxxxxxxxxxxxxxxxxx>:
>> > -----BEGIN PGP SIGNED MESSAGE-----
>> > Hash: SHA256
>> >
>> > On 06/22/2013 09:43 AM, Bálint Réczey wrote:
>> >> Hi Marc,
>> >>
>> >> 2013/6/22 Marc Petit-Huguenin <marc@xxxxxxxxxxxxxxxxxx>:
>> >>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
>> >>>
>> >>> On 06/22/2013 03:47 AM, Bálint Réczey wrote:
>> >>>> Hi All,
>> >>>>
>> >>>> 2013/6/21 Marc Petit-Huguenin <marc@xxxxxxxxxxxxxxxxxx>:
>> >>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
>> >>>>>
>> >>>>> On 06/20/2013 04:52 PM, Guy Harris wrote:
>> >>>>>>
>> >>>>>> On Jun 20, 2013, at 2:58 PM, Marc Petit-Huguenin
>> >>>>>> <marc@xxxxxxxxxxxxxxxxxx> wrote:
>> >>>>>>
>> >>>>>>> On 06/20/2013 02:17 PM, Gerald Combs wrote:
>> >>>>>>>
>> >>>>>>>> Advantates: - I'm not sure that an in-house equivalent (e.g.
>> >>>>>>>> Gerrit plus a private repository) would be better than what
>> >>>>>>>> Github offers.
>> >>>>>>>
>> >>>>>>> Yes, Gerrit is better than github:
>> >>>>>>
>> >>>>>> Presumably you mean "Gerrit plus a private repository is better
>> >>>>>> than github", as Gerrit, as far as I can tell, is just software
>> >>>>>> that works with a Git repository.
>> >>>>>
>> >>>>> Yes, although managing repositories being what Gerrit do, Gerrit
>> >>>>> without a least one repository would be a very boring application.
>> >>>> :-)
>> >>>>
>> >>>> I have started describing a Gerrit based workflow which IMO would fit
>> >>>> to the project at http://wiki.wireshark.org/Development/Workflow .
>> >>>> Please check it and share your opinion.
>> >>>>
>> >>>
>> >>> "Code is building and tests are passing on all platforms. (Tests
>> >>> automatically start when at least one Core Developer gives +1 or +2 to
>> >>> prevent overloading or cracking the build servers.)"
>> >>>
>> >>> Why do not build and test all patchsets submitted?  Is that a
>> >>> limitation
>> >>> of the build servers?  Having Jenkins automatically verify your
>> >>> patchset
>> >>> is IMO one of the nice feature of Gerrit, and it will lower the
>> >>> workload
>> >>> of core devs if building and testing are done before they start
>> >>> looking
>> >>> at the patchset.
>> >> Build can be triggered by patchset submissin, too, but it would require
>> >> more build server resources. Usually not the first version of the
>> >> changeset will be accepted especially from new contributors and this
>> >> means
>> >> more builds.
>> >
>> > My view is the opposite: New contributors patchsets will probably be
>> > rejected
>> > anyway (does not build, does not pass test, etc...), so having the
>> > system
>> > doing that lowers the burden on core developers, who they can focus on
>> > more
>> > high level problems.
>> >
>> >> Note that Core Developers would not have to wait since they can give +1
>> >> for
>> >> their own changesets.
>> >>
>> >> The other reason behind requiring a +1 from someone we trust is that
>> >> otherwise it would be easy to prepare a changeset which does
>> >> unspeakable
>> >> things to the build servers which we don't want to happen. Without
>> >> requiring +1 we would have to prepare build systems to cope with
>> >> malicious
>> >> commits.
>> >
>> > That is a good point (basically because of the halting problem).  But
>> > builds
>> > are done in isolation (i.e a git clone is done each time), so apart
>> > using too
>> > much resources or never ending, there is no harm that can be done to the
>> > infrastructure.  And there is a Jenkins plugin to abort a build if it is
>> > stuck.
>> I was concerned about using the buildbots for attacking other systems,
>> too,
>> but all of those threats can be handled and we have time for setting
>> up build bots
>> properly.
>>
>> I have updated the proposal to start tests for every change and allow
>> Code Devs to
>> bypass peer review.
>>
>> Comments are still welcome. :-)
>>
>> Cheers,
>> Balint
>>
>
> Re the possible change to require peer review of all changes.
>
> My experience of this was in my day job, which involves a commercial
> codebase with a long history (20+ years) and 15 or so full time devs.  The
> product is a toolkit which means high flexibility and an inability to test
> (in a practical length of time) all possible scenarios that customers might
> create.  Many of the devs have been with the company for a long time and
> have huge experience of the codebase, and of programming in general.
>
> About 8 years ago we switched from a direct commit to trunk to a branch and
> peer review and merge to trunk model very similar to that proposed for
> Gerrit but using svn and our customised version of a commercial bug tracking
> system.
>
> Code quality and product quality has risen dramatically over time.  One
> factor was that we didn't have a continuous build system so defects
> introduced by changes were either discovered by devs during other work or
> during regression testing before a release or by customers.  We have
> detailed statistics backing up my anecdotes showing the rate at which
> changes cause further defects and amazingly we can now "predict" future
> failure rates based on code churn and current failure rates.
>
> Code inspection by peers improved code quality, prevented "collateral
> damage" where the immediate problem was fixed but the change broke something
> else and we all believe produces better quality solutions to issues and
> features.  Unfortunately devs are human and we still manage to incorporate
> bugs when making changes so it doesn't fix all ills.
>
> Peer review does add a cost but for our commercial product the benefits far
> outweigh the costs.  For Wireshark I'm convinced there will be benefits, but
> the cost is harder to quantify. In a commercial setting devs are tasked for
> peer review but in our ad-hoc Wireshark world this causes me a little
> concern as we would need to get dev traction on getting peer review done.
> IMHO allowing devs to short-circuit per review cuts any benefits gained
> almost to be point of why bother with peer review?
I share your opinion regarding the importance and value of performing
code review
in commercial products.
I'm sorry for summarizing the change to the proposal in my previous
email, it missed
the information that Core Devs should bypass the review only for
trivial changes.
I have reworded the text on the wiki page to emphasize that:
1. At least one Core Developer gave +2. (Core Developers can bypass
peer review for trivial changes by giving +2 for their own commits.)

I believe Core Devs will not abuse this opportunity and we will
collectively set a level for "trivial change" which we can more or
less agree with.

I also believe we can also find some time for performing the code
review, as a start, the time we used for fixing broken master prior to
introducing the new workflow. :-)

>
> tldr;
>
> Peer review has benefits, are they worthwhile for wireshark.
Fully agree.

Cheers,
Balint