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

Thu Oct 21 08:44:57 EDT 2010

Gaetan,

I made a few comments where the standard ITK copyright comments were missing
in files.  Could you please add those before you do the commit?

Once the copyright comment is added, I'm OK with you pushing this.  It will
be nice to have this included before before the software licensing changes
are made (early this weekend).

Thanks,
Hans

On 10/21/10 4:43 AM, "Gaëtan Lehmann" <gaetan.lehmann at jouy.inra.fr> wrote:

> 
> 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