On Thu, May 04, 2023 at 06:34:42PM +0200, Takashi Iwai wrote:
On Thu, 04 May 2023 17:36:11 +0200,
Oswald Buddenhagen wrote:
On Thu, May 04, 2023 at 04:49:58PM +0200, Takashi Iwai wrote:
> Sorry, that doesn't work. The review is done upon the patch, and if
> this patch can't be reviewed easily, it's simply no-go.
>
that's a self-imposed limitation.
And that's how the process works. Over decades.
that doesn't mean that it's the best process. it's the only thing that
was available 30 years ago, but technology has moved on.
All those git-based work flows are based on commits, and commits
consist of patches.
yes
So, reviewing each commit is nothing but reviewing a patch.
that's technically correct, but completely misses the point. with a
proper review tool, looking beyond the patch itself becomes *much*
cheaper, which was the argument here.
in fact, gerrit defaults to side-by-side diff view, so people look at
the new code rather than at the patch. (i actually think that's a stupid
default, but having the option to switch in a second is extremely
valuable.)
IOW, if you do a PR via github, it'll hit the very
same problem; when the commit is not understandable enough for
reviewers, it'll be NAK'ed.
this is true, but with better tooling there are fewer pointless
limitations.
It's not only about contexts. It's just not clear what your patch
does as partial revert and as fix.
the fact that it's a partial revert is a property of the transition
(that is, the patch) and therefore something to note in the history, but
for understanding the correctness of the final code it's utterly
irrelevant.
this is all deeply intertwined code.
Err, I can't follow; in the previous patch and this patch, you added
more comments, use different terms and variable names, and use
different way to calculate the hw_avail value, etc -- which are
basically irrelevant from the behavior fix itself, but they are just
code refactoring. Those could be separated easily.
some bits can be separated, while others can't. you'd just get a bunch
of micro-changes, an insane amount of churn, and maybe two "meaty"
patches which wouldn't be any simpler to actually understand.
so "partially rewrite" is imo exactly the right granularity for
approaching this.
splitting up the patch will
merely give you the *illusion* of better understanding the pieces. but
to actually make sense of it, you need to see the whole, in its end
state, because there are no fully functional intermediate states.
Again, I can't follow your logic. Why splitting into small pieces
can't lead to a better understanding *at all*? Why you must refuse
it? Certainly one needs to take a look at big picture. But it's a
different story.
patches should be atomic. that means each one should contain one
*complete* change. if you split a patch into pieces that aren't complete
in themselves, you're just obfuscating the complexity.
i'm not going to try to prove to you that this is the case here. i just
know that i failed at atomizing this _properly_ the first time around.
the original patch was three patches at first. when i was attempting
to write proper commit messages explaining what fixes what, i found
that it's just impossible to untangle it without lying by omission. so
i squashed them and wrote a cumulative description. and you accepted
it.
The acceptance of your patch was my failure, yeah. I should have
rejected it. So this failure doesn't happen again. You're seeing the
result now.
i think what i'm actually seeing is that you guys got bitten and are
over-compensating.
but the patch was good, it fulfilled the documented api contract, and
was thoroughly tested accordingly. the only problem was that there was
an *undocumented* part of the api contract, and you both failed to
notice it. atomizing the patch further wouldn't have changed anything
about that. shit happens.
regards
