[gentoo-dev] "Trivial" commit reviews

[gentoo-dev] "Trivial" commit reviews

[Donnie Berkholz]

Mike Doty (KingTaco) just told me I could stop sending reviews to -dev 
that are just about adding quotes or other trivial issues that come up 
over and over. I'm going to tell you why it's still a good thing.

First, where one problem lurks, others often do too. In code with such 
simple problems, it's likely that more complex problems also exist. 
Getting more eyes on problematic code of any sort can help find them.

Second, as we've already seen, no one developer is familiar with all the 
code. Both Mike Frysinger and Daniel Drake have responded to some of my 
reviews, pointing out further problems with the same code.

Third, by continuing to post these reviews, it should become obvious to 
_all_ developers that they should be checking for them _before_ 
committing instead of waiting for a review.

Over time, the number of these simple reviews should go dramatically 
down so it no longer bothers anyone to see them. If it doesn't, that 
means some of our developers aren't learning or paying attention, and we 
should take a closer look at whether they should remain developers.

Thanks,
Donnie
-- 
gentoo-dev@gentoo.org mailing list

Re: [gentoo-dev] "Trivial" commit reviews

[Mike Doty]

Donnie Berkholz wrote:
> Mike Doty (KingTaco) just told me I could stop sending reviews to -dev 
> that are just about adding quotes or other trivial issues that come up 
> over and over. I'm going to tell you why it's still a good thing.
> 
> First, where one problem lurks, others often do too. In code with such 
> simple problems, it's likely that more complex problems also exist. 
> Getting more eyes on problematic code of any sort can help find them.
> 
> Second, as we've already seen, no one developer is familiar with all the 
> code. Both Mike Frysinger and Daniel Drake have responded to some of my 
> reviews, pointing out further problems with the same code.
> 
> Third, by continuing to post these reviews, it should become obvious to 
> _all_ developers that they should be checking for them _before_ 
> committing instead of waiting for a review.
> 
> Over time, the number of these simple reviews should go dramatically 
> down so it no longer bothers anyone to see them. If it doesn't, that 
> means some of our developers aren't learning or paying attention, and we 
> should take a closer look at whether they should remain developers.
> 
> Thanks,
> Donnie
My concern is that if we flood -dev with "trivial" commit problems then 
more people will stop watching -dev and/or resort to killfiles or other 
filtering.  While I do agree with Donnies assessment, my concern is that 
over a longer time period, it might have a negative effect.

-- 
=======================================================
Mike Doty                      kingtaco -at- gentoo.org
Gentoo Infrastructure
Gentoo/AMD64 Strategic Lead
GPG: E1A5 1C9C 93FE F430 C1D6  F2AF 806B A2E4 19F4 AE05
=======================================================
-- 
gentoo-dev@gentoo.org mailing list

[gentoo-dev] Re: "Trivial" commit reviews

[Ryan Hill]

Mike Doty wrote:
> Donnie Berkholz wrote:
>> Mike Doty (KingTaco) just told me I could stop sending reviews to -dev
>> that are just about adding quotes or other trivial issues that come up
>> over and over. I'm going to tell you why it's still a good thing.
>>
>> First, where one problem lurks, others often do too. In code with such
>> simple problems, it's likely that more complex problems also exist.
>> Getting more eyes on problematic code of any sort can help find them.
>>
>> Second, as we've already seen, no one developer is familiar with all
>> the code. Both Mike Frysinger and Daniel Drake have responded to some
>> of my reviews, pointing out further problems with the same code.
>>
>> Third, by continuing to post these reviews, it should become obvious
>> to _all_ developers that they should be checking for them _before_
>> committing instead of waiting for a review.
>>
>> Over time, the number of these simple reviews should go dramatically
>> down so it no longer bothers anyone to see them. If it doesn't, that
>> means some of our developers aren't learning or paying attention, and
>> we should take a closer look at whether they should remain developers.
>>
>> Thanks,
>> Donnie
> My concern is that if we flood -dev with "trivial" commit problems then
> more people will stop watching -dev and/or resort to killfiles or other
> filtering.  While I do agree with Donnies assessment, my concern is that
> over a longer time period, it might have a negative effect.
 
I think this is exactly what -dev is for.  And the number of trivial
mistakes people are making (not discounting myself) shows this kind of code
review is needed.  There is a huge amount of basic stuff, like quoting or 
when to use ${ROOT}, that we just don't have documented anywhere, and expect
people to somehow just pick up.

If we flood -dev with "trivial" commit problems then more people will stop
commiting them.  If people want to stop reading or filter then that's their
prerogative.  -dev isn't a required ML.

-- 
                  fonts / wxWindows / gcc-porting / treecleaners
  9B81 6C9F E791 83BB 3AB3  5B2D E625 A073 8379 37E8 (0x837937E8)

-- 
gentoo-dev@gentoo.org mailing list

Re: [gentoo-dev] "Trivial" commit reviews

[Thilo Bangert]

[-- Attachment #1: Type: text/plain, Size: 317 bytes --]

i am all for the 'trivial' review. as i am not on the commit list, 
however, i can't tell whether this acutally helps. 

do people fix the stuff that is pointed out to them?

also, perhaps the more common ones should additionally be converted to 
repoman tests, if that is feasable.

kind regards
Thilo



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [gentoo-dev] "Trivial" commit reviews

[Donnie Berkholz]

On 06:22 Mon 24 Sep     , Thilo Bangert wrote:
> do people fix the stuff that is pointed out to them?

Yep, I've seen a lot of fixes for reviews.

> also, perhaps the more common ones should additionally be converted to 
> repoman tests, if that is feasable.

That might be reasonable for some cases, but it won't be perfect, and 
won't even be possible for many.

So far, the only one I've seen that might work well for is quoting 
around specific variables. You could do something like a grep for words 
containing '${+D[^[:alnum:]-_]' (haven't tested that, just beginnings of 
an idea) and the same for S and WORKDIR.

Thanks,
Donnie
-- 
gentoo-dev@gentoo.org mailing list

Re: [gentoo-dev] "Trivial" commit reviews

[Matti Bickel]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

Mike Doty <kingtaco@gentoo.org> wrote:
> Donnie Berkholz wrote:
>> Over time, the number of these simple reviews should go dramatically down 
>> so it no longer bothers anyone to see them. If it doesn't, that means some 
>> of our developers aren't learning or paying attention, and we should take 
>> a closer look at whether they should remain developers.

> My concern is that if we flood -dev with "trivial" commit problems then 
> more people will stop watching -dev and/or resort to killfiles or other 
> filtering.  While I do agree with Donnies assessment, my concern is that 
> over a longer time period, it might have a negative effect.

I totally agree with Donnie here. Please keep up the work, everybody
should be encouraged to fix these (trivial) problems. I sincerly hope
that these message will not have to continue for long. But as long as
they do, they serve as a big reminder in your inbox of what is wrong.

Just my 0.02$
-- 
Regards, Matti Bickel
Signed/Encrypted email preferred (key 4849EC6C)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[gentoo-dev] Ebuild QA (was Re: "Trivial" commit reviews)

[Steve Long]

Donnie Berkholz wrote:
> On 06:22 Mon 24 Sep     , Thilo Bangert wrote:
>> also, perhaps the more common ones should additionally be converted to
>> repoman tests, if that is feasable.
> 
> That might be reasonable for some cases, but it won't be perfect, and
> won't even be possible for many.
>
Which are the cases you (and others ofc) think it would be reasonable for?
 
> So far, the only one I've seen that might work well for is quoting
> around specific variables. You could do something like a grep for words
> containing '${+D[^[:alnum:]-_]' (haven't tested that, just beginnings of
> an idea) and the same for S and WORKDIR.
> 
marienz mentioned something similar a few months ago, wrt allowing unquoted
expansions for specific vars that are used in eg for loops (like A or
SRC_URI) and not for any others.

ed(1) really is the tool to use for that -- g/RegEx/p does exactly what grep
does, and there's no compatibility issue for FreeBSD vs Linux. (It's also a
lot more capable than sed, and there really is no need for awk in this
instance.)

1) http://bash-hackers.org/wiki/doku.php?id=howto:edit-ed


-- 
gentoo-dev@gentoo.org mailing list

Re: [gentoo-dev] Ebuild QA (was Re: "Trivial" commit reviews)

[Donnie Berkholz]

On 09:55 Mon 24 Sep     , Steve Long wrote:
> Donnie Berkholz wrote:
> > On 06:22 Mon 24 Sep     , Thilo Bangert wrote:
> >> also, perhaps the more common ones should additionally be converted to
> >> repoman tests, if that is feasable.
> > 
> > That might be reasonable for some cases, but it won't be perfect, and
> > won't even be possible for many.
> >
> Which are the cases you (and others ofc) think it would be reasonable for?

I don't think we've yet discovered what all the common cases are. As the 
current top issues drop because people check for them, these reviews 
will likely begin to reveal new top issues, as the quoting is now.

> ed(1) really is the tool to use for that -- g/RegEx/p does exactly what grep
> does, and there's no compatibility issue for FreeBSD vs Linux. (It's also a
> lot more capable than sed, and there really is no need for awk in this
> instance.)

The problem with using ed in Gentoo is that nobody (figuratively) knows 
how to use it. This means nobody can spot problems with others' use of 
it. Our standard set of ebuild tools is pretty much:

bash
grep
sed
awk

Even if it's not the most elegant solution technically, it's the one 
that works best when you consider the bigger picture of our developers' 
skills.

Thanks,
Donnie
-- 
gentoo-dev@gentoo.org mailing list

Re: [gentoo-dev] "Trivial" commit reviews

[Rémi Cardona]

Matti Bickel wrote:
> I totally agree with Donnie here. Please keep up the work, everybody
> should be encouraged to fix these (trivial) problems. I sincerly hope
> that these message will not have to continue for long. But as long as
> they do, they serve as a big reminder in your inbox of what is wrong.

+1, I've already learned or re-learned some very useful stuff.

/me will keep reading those ebuild reviews.

Rémi
-- 
gentoo-dev@gentoo.org mailing list