[Insight-developers] gerrit: Ouch...my mouse clicking finger is sore ; )

Thu Oct 21 10:05:36 EDT 2010

This is where Brad and I have been discussing adding a topic level
view to Gerrit. Agreed, until that is there (or some other way to
collapse a topic down) Gerrit's interface is poorly adapted to such
large topics. I think it should also be noted that any change where
the number of changed lines numbers in the thousands is tough to do
code review on - the reviewers do not scale well...

I have tried to make it clear from the start (as has Brad King) that
the current Gerrit needs some new features before it deals with topic
branches well. This is something we want to improve, and there is a
general need in the community to improve this. The previous Gerrit
workflow has been very commit centric.

Marcus

2010/10/21 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>:
>
> I guess we all have to agree that this is not right way to use gerrit.
> In order to avoid overloading the gerrit interface and giving enough
> visibility to the other entries, I'd like to push those changes in ITK, if
> nobody disagree.
>
> At some point, we'll have to define a limit so the developer can choose what
> should go in gerrit and what shouldn't.
>
> Gaëtan
>
>
> Le 19 oct. 10 à 21:45, Gaëtan Lehmann a écrit :
>
>>
>> Le 19 oct. 10 à 19:54, Marcus D. Hanwell a écrit :
>>
>>> On Tue, Oct 19, 2010 at 1:25 PM, Bill Lorensen <bill.lorensen at gmail.com>
>>> wrote:
>>>>
>>>> What is the magic patch that will include all of the patches?
>>>
>>> http://review.source.kitware.com/#change,209
>>>>
>>>> On Tue, Oct 19, 2010 at 1:19 PM, Hans Johnson <hans-johnson at uiowa.edu>
>>>> wrote:
>>>>>
>>>>> So here’s my experience with gerrit on the 33 patches as part of the
>>>>> labelmap topic branch.
>>>>>
>>>>> I like the separate patches, and think that from a development point of
>>>>> view
>>>>> this is definitely the best approach.
>>>
>>> Personally, I would not have made a single commit for each
>>> class...Gerrit or no Gerrit. I would have also squashed a few of the
>>> commits such as,
>>>
>>> http://review.source.kitware.com/#change,200
>>>
>>> This is something that I would edit in my local history before publish.
>>
>> The classes touched by this patch were already in the ITK repository.
>> I've squashed as much changes as I can to avoid the bug fixing patches of
>> my previous non yet integrated patches and make the number of patches not to
>> big.
>>
>>>
>>>>> It was annoying to click so many times to do a review that was so
>>>>> closely
>>>>> related for every item.
>>>>> Gerrit was mind-numbingly PAINFUL to use in to approve these.  It took
>>>>> almost 10 minutes to click through each of the review stages pushing
>>>>> the
>>>>> same buttons, and pasting the same comment.
>>>
>>> Like I said, such large topic are likely to be painful either way. The
>>> web interface is certainly not optimized for topic branches either.
>>> Would the command line Gerrit tools have been useful?
>>>
>>> http://gerrit.googlecode.com/svn/documentation/2.1.5/cmd-review.html
>>>
>>> $ ssh review.source.kitware.com gerrit review --project ITK --message
>>> \"This comment entered from the command line.\" --verified 1
>>> --code-review 1 eb675d9870bc94ebe8bd8d87968ee36cfe5e7ff9
>>>>>
>>>>> I know I’m complaining a lot here, but it is because I really want
>>>>> gerrit to
>>>>> succeed.  It took me more than an hour to review this topic branch,
>>>>> verify
>>>>> that it works, and record my approval of the patches.
>>>>>
>>> I think it all depends on whether the commits ever needed to be
>>> reviewed independently. If they did not then I am not sure they needed
>>> their own commits, but that is up for debate. If it is just the number
>>> of clicks you had to make in order to approve a topic branch, then I
>>> wonder if an 'Approve topic branch' solution would be most
>>> appropriate?
>>>
>>> The problem is if you ask someone to squash all of their commits into
>>> one for review, ask for a change to be made before approval do they
>>> then need to go back to their topic, edit its history, re-squash it,
>>> then add back in the Change-Id and when it is finally merged have what
>>> is merged bear no resemblance to what Gerrit say/approved?
>>>
>>> Gerrit has a fairly well developed command line interface if you would
>>> like a functionality to approve a topic branch. I guess when the patch
>>> set is so big it is unlikely people will have time to fully review the
>>> changes. The merge is certainly a simple operation, merging in the tip
>>> of the topic will merge all other commits.
>>>
>>> I would tend to bring something this big in as a full topic, using git
>>> to view the diff while it is compiling. So perhaps a way of approving
>>> a topic in its entirety, and this is something we probably want to
>>> work towards in the web interface too.
>>
>>
>> Maybe this tool is not the right one for this kind of work: everything
>> here goes in Review and should have been reviewed by the IJ reviewers first.
>> It may have been better to commit most of my changes directly in ITK, and
>> to have put only the patches not strictly from the IJ contribution in
>> gerrit. That would have reduced the list to
>>
>>  http://review.source.kitware.com/#change,181
>>  http://review.source.kitware.com/#change,186
>>  http://review.source.kitware.com/#change,197
>>  http://review.source.kitware.com/#change,199
>>  http://review.source.kitware.com/#change,198
>>  http://review.source.kitware.com/#change,208
>>  http://review.source.kitware.com/#change,207
>>  http://review.source.kitware.com/#change,206
>>
>> Gaëtan
>>
>> --
>> Gaëtan Lehmann
>> Biologie du Développement et de la Reproduction
>> INRA de Jouy-en-Josas (France)
>> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
>> http://voxel.jouy.inra.fr  http://www.itk.org
>> http://www.mandriva.org  http://www.bepo.fr
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Kitware offers ITK Training Courses, for more information visit:
>> http://kitware.com/products/protraining.html
>>
>> Please keep messages on-topic and check the ITK FAQ at:
>> http://www.itk.org/Wiki/ITK_FAQ
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.itk.org/mailman/listinfo/insight-developers
>
> --
> Gaëtan Lehmann
> Biologie du Développement et de la Reproduction
> INRA de Jouy-en-Josas (France)
> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
> http://voxel.jouy.inra.fr  http://www.itk.org
> http://www.mandriva.org  http://www.bepo.fr
>
>