On Thu, 04 May 2023 15:54:15 +0200, Oswald Buddenhagen wrote: > > On Thu, May 04, 2023 at 03:33:01PM +0200, Jaroslav Kysela wrote: > > On 04. 05. 23 15:28, Takashi Iwai wrote: > >> Honestly speaking, this is really hard to review. > > >> As most of changes here are the revert of the previous commit, > >> > no they aren't. Erm, that is a big part of problems. We don't see clearly what part is the revert to the old logic and what is the actual fix. You mixed things, and it's really hard to follow. > >> I'd rather like to get it > >> reverted whole once, and re-apply the proper fix gradually. > > > > I fully agree here. Takashi, please, revert the broken patch right now. > > > actually reverting would include reverting the comments, which would > be just plain stupid. A whole revert sounds too much, but it makes the changes more straightforward afterwards. This is the biggest win. We want to see the fix applied in each commit in the right way. Not in a mixture. > > I think that the review and improving the code may take some days. > > > i'm not going to deliver anything more on that matter just to satisfy > some arbitrary process. It's not "some arbitrary process". The patch review is _the_ most important process, and if a patch makes it difficult, it implies that it's already fundamentally bad. > i think the patch does the right thing, with > the right granularity, and i'm not going to spend time breaking my > head on producing broken intermediate states which i cannot properly > reason about due to their internal inconsistency. > > you can "rebase" my patch by checking it out on top of a partial > revert, but you need to come up with your own commit message then. and > i think that it would be utterly counterproductive. viewing the diff > for review purposes may make sense, though. 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. Again, the problem is the mixture; it partially reverts to the original code while it modifies some part in some other way. For example, as already pointed, only from this patch, it's not clear whether the handling of silence_start in the patched code is OK or not. That's no part of revert, and I can't judge whether it's the correct and intended change or an oversight. By reverting the whole and reapplying fixes -- although it'll need more steps -- we can see more clearly what change fixes which part. The patch granularity, the patch description, rules and whatever, all of these are just for reviewing the patches properly; it results in better understanding and in the good code in the end. Takashi
