Re: [PATCH] ALSA: pcm: fix snd_pcm_playback_silence() with free-running mode

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

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