Attention is currently required from: Arthur Heymans, Jonathan Zhang, Johnny Lin, David Hendricks, Stefan Reinauer, Christian Walter, Deomid "rojer" Ryabkov, Tim Chu.
1 comment:
Patchset:
David, I wrote my response below first, than had another look at it. It
seems to me that you simply underestimate everything, especially the work
that is put on reviewers. If I had to derive a rule about -2 from this
it would be like this:
Make sure that you see the full picture and carefully weigh
alternatives before giving a -2 score.
But I don't think we should have rules like this. I think -2 privileges
should be limited to few people that can learn to make their own rules
and act appropriately. That includes you David, and I hope you learn from
this incident (eventually; I'm not saying that my answer concludes
anything).
While aggravating, a -2 is not a personal attack, and in this case I am pretty sure that it was an attempt to spark a conversation.
Indeed, and this is the crux of the matter. Nico's reaction when he saw something broken was to revert, which is understandable, while mine was to fix the code so it works as intended (which obviously did not include breaking other things). In this situation one of the possible fixes was literally 1 line of code (2 if you count the comment) in CB:57581. Other fixes proposed were a few lines longer. Simple enough, right?
Simple to write. But you ignore the review it seems. CB:57581 does two things
and makes a lot of assumptions. One needs to find two SPI Guides to review it.
I guess which alone might take multiple hours. It also lives from the assump-
tion that the obvious regression is the only thing that is wrong with the ori-
ginal commit. I'd estimate about 4 to 6 hours of review to clear the whole
matter up and breaking the circle of having to revert things. This review
would have been forced by you with your -2. And that with time pressure.
Because you didn't allow the master branch to be fixed before the whole
matter is settled.
My rationale for wanting to push those changes instead was because they're so tiny and got both chipsets working again so we could all move on to solve other problems. To me that seems a lot simpler, faster, and easier to follow in the git log than reverting CB:54965 with this patch, fixing the bad assumption introduced in CB:55651 using CB:57633, and then reverting the revert (this patch) to add Emittsburg back in. We could have had everything working within a day taking timezone differences into account.
I know you had somewhat good intentions, but if any of that is true, why are
you not done yet? You could have followed the flow, instead you stopped the
whole current with your -2. I'd estimate the same "within a day" if you hadn't
given your -2. We can never tell if that had worked out, because we didn't
take that path. You took the -2 path and, given that it's not Monday anymore,
proved that the one-day assumption is wrong for your path.
And as per the gerrit guidelines, when giving the -2 I provided a concrete recommendation for how to fix things.
Let's see, here is what you wrote, IMHO not less cryptic than my later
comments:
>> Ibex Peak isn't "detected",
This is true for the current master state and supports the revert not
your -2. But there are these quotes so you probably meant something
else.
>> it's the default when `content->ISL != 16`.
Is that supposed to explain the quotes? Let's assume yes, but why
care about the ISL != 16 case? The working code detected Ibex Peak
if ISL == 16 and warned when not.
>> The way that if-statement works also assumes no FD will have content->ISL > 16, unless `FLMAP2 == 0` (which isn't true in all cases).
The only if statement in this patch: `if (content->ISL <= 80)`. Who
can follow?
>> We need to fix Ibex Peak detection
Again this is what the revert achieves.
>> or come up with a better handle these ISLs,
Absolutely an option. But if you have to force other people to help
you find a solution, isn't there something wrong?
Also, I hope this is a boolean `or`. Otherwise you would have
suggested that not fixing Ibex Peak detection is an option.
>> not prevent other platforms from getting added.
There is nothing that prevents adding other platforms. The easiest
way is to ask for it on the mailing list; people in this community
are incredibly helpful. In this one instance it's not much work
actually, as the existing code seems to support most aspects of
the new platform already. One just needs to compare the existing
code to the datasheet to find the right path, I guess.
Alas, for the commit in question, it seems instead of looking into
the datasheet, assumptions were made. These assumptions were wrong
and ignored during review, and on top something broke due to a program-
ming error.
That was the original -2 comment so far, you continued then with this:
>> See comments in CB:54965
This is what made me really angry because you forced me to look
into another review (not a fix) which I had no time for. I do not
think that forcing people to review can justify a -2. It's some-
thing people really shouldn't do to each other.
My recommendation actually solved the problem for both sides. I didn't just throw a tantrum and spew unhelpful and condescending remarks at the other side.
See above, how was that cryptic -2 justification helpful? Neither
do I see any clear recommendation nor is anything solved, not even
by now.
Reviewing a <5 line patch that actually fixes things would have taken far less time, too.
Please prove that. If a patch makes additional assumptions and is
based on assumptions made by a patch that is broken, how can that
be easy to review? The number of lines really doesn't matter, you
can break any program with a single line.
And when Edward stepped in to clarify that "we should revert and re-land with it fixed correctly" I took my -2 off. Perhaps that should be in the flashrom development guidelines to automatically resolve these sorts of questions: https://www.flashrom.org/Development_Guidelines
And what exactly do you suggest to add there? My biggest concern
about your -2 was that it would force people to review (and fix
because you wrote them rather hastily) your patches. Isn't that
implicit that we don't use -2 to force other people like that?
To view, visit change 57589. To unsubscribe, or for help writing mail filters, visit settings.