On Sat, 10 Oct 2015 20:37:39 +0200 hasufell <hasuf...@gentoo.org> wrote: [...] > >> This is just a concept of peer-reviewing, which was very difficult > >> in CVS times. > > > > I fail to see how post-commit reviews are made easier with git. > > > > Quite offtopic, but we could discuss this off-list if you want.
You could just have said why. I don't see a difference between reviewing gentoo-commits from cvs than reviewing gentoo-commits from git. Maybe there's something you prefer, that I don't use, and I can't debate your preferences. > > [...] > >>> Also, you should probably focus on what's really important: > >>> reviews like "this is weird, care to explain?" or stylistic > >>> nitpicks are just a waste of every one time, meaning more > >>> important stuff does not get done. > >>> > >> > >> 'has_version' (which you are probably referring to) as a > >> conditional for sedding headers is more than just "weird" and > >> indicates a serious build system bug that needs to be addressed > >> properly. > > > > It indicates a conditional fix. Just as the code says. > > It's a hack, not a fix. And as such, it is worth discussing. "It's a hack", "indicates a serious build system bug that needs to be addressed", etc. : such comments are critics, a bit aggressive, and call for more aggressiveness. In order to help, and have it welcomed, you should probably try a bit more of courtesy :) Not everyone has an "hasufell-translator" reading "hack" as "build fix" :) > > Before throwing > > an email to -dev ml, I would have expected a reviewer to do his > > homework and try to understand what the condition is, when it will > > be satisfied, and why this was conditional. There is absolutely > > nothing wrong about not knowing the answer, but using -dev ml for > > it is a bit spammy IMHO. > > > > I did have a look. I was checking both packages (dev-libs/libcdio and > dev-libs/libcdio-paranoia) for the header and made up my own mind > about this. I was still wondering if the maintainer even knows what > this is about. I don't think you can make a proper opinion on this without the history of the split. The code in question is actually correct, it could just benefit a bit of cleanup for legacy stuff. But again, nobody expects you to know that. Reviewing is about pointing out improvements, so in that case that would be: "Hi, the code you're sedding is actually only compiled when USE=cdda is set, which actually always pulls dev-libs/libcdio-paranoia, so it seems to me the conditional could be dropped for clarity." This is what makes the difference between helpful reviews and critics or even testing "if the maintainer even knows what this is about" :) [...] Alexis.
