Changes to our Git infrastructure

[Martin Gräßlin]

On Tuesday 06 January 2015 12:48:41 Jan Kundrát wrote:
> On Tuesday, 6 January 2015 07:40:01 CEST, Ian Wadham wrote:
> >    a) "I do not know anything about Dr K, but I will try and
> > 
> > find someone who does."
> > 
> >    b) "Unfortunately there is nobody available any more who
> > 
> > knows anything about
> > 
> >         Dr K, but I (or another suggested guy) will try to
> > 
> > help.  How about we take this
> > 
> >         offline via email or IRC and then you can walk us
> > 
> > through the problem you are
> > 
> >         trying to fix, its significance and impact and how you
> > 
> > are going about fixing it…"
> 
> This has a risk of splitting the discussion about a patch into multiple
> independent streams where people will have hard time keeping track of all
> the results, IMHO.
> 
> > The polishing (fixing nitpicks, etc.) should come *after* the stone is
> > cut.
> 
> That's a good suggestion.

I have to disagree here. I'm not able to do a good functional review if the 
change is full of unintended "nitpick" things. Each changed line triggers a 
"what is that change for?" and if it's just a change like
- if (foo) {
+ if (foo)
+ {

then I have a hard time as a reviewer. For me as a reviewer it's important 
that the change is free of those "nitpick" things before I start looking at 
the functional things. It's also something I expect from a contributor to 
spend the time to run astyle on the change and configure kate to remove 
trailing white spaces before requiring my time.

For me it's difficult to recognize the "true" change of a patch if it's full 
of unintended changes. It just makes everything more difficult. If I have to 
review a change with such nitpick items I normally do a nitpick round first, 
then wait for the next patch version to do the proper review. This is 
optimizing my time as a reviewer. (just to get some feeling on my review 
costs: since yesterday I received 20 new mails in my reviewboard mail folder).

Also I do not think that reviewboard makes nitpicking to easy - if it wouldn't 
highlight the trailing whitespaces I would still point them out as I consider 
these things as important.

> 
> > Going straight to that mode is inappropriate because it conveys the
> > message, "The problem you are trying to fix is unimportant to us".
> 
> Would it work for you if there was a bot which pointed out these issues to
> the patch author? That way it would be obvious which part of the review is
> random nitpicking which is indeed not important when one considers the
> general direction of the patch, and in addition it would come from a
> machine so there won't be any bad feelings about people not appreciating
> the contributor's work.

I would certainly appreciate it. I don't like doing things a computer could 
do. It might also make my life as a reviewer easier as I can see that the 
computer already handled it.

Qt does that and if I hit such a problem the first thing I do is fixing those 
items to upload a clean patch.

Cheers
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20150107/41e1db65/attachment.sig>