Re: [PATCH,HURD][RFC] hurdselect: Step7x, almost complete rewrite finished

[Svante Signell]

On Wed, 2013-02-13 at 00:30 +0100, Richard Braun wrote:
> On Wed, Feb 13, 2013 at 12:12:57AM +0100, Svante Signell wrote:
> > What a pity, I spent a very long time doing this rewrite, and to make
> > the code more readable (at least in my opinion). Back to spaghetti^Wold
> > style code then.
> 
> We warned you several times about making your changes as little
> intrusive as possible for easier review. Since you seem to be unable to
> understand what that implies, it's only natural Samuel no longer wastes
> time on this and simply replies "to long to go through it".

I am able to understand, no problem. And I know that a patch this large
can be difficult to review. Problem is that he did not give any hints on
_how_ to make the rewrite. If introducing the SELECT_ERROR had been
discussed (or allowed to introduce) my solution would have been
different too. But I would still use the helper functions to improve
readability (and maintainability).

> You have to understand that whether the code looks "spaghetti" or not,
> your goal here is not to fix that, it's to fix a POSIX conformance
> issue. If you want to refactor, fine, but please separate it. It's
> really important to make as little change as possible for two reasons.
> The first is the obvious comfort for the reviewers. 

Agreed!

> If it's easier for
> us, it's also faster, and we can more effectively concentrate on
> thinking about the big picture and the details. The second is to reduce
> the likeliness of regressions. Please keep that in mind.

Yes, the risk of regressions increase proportionally to the number of
changes. But sometimes, introducing fixes for every corner case in an
old design can create other bugs and/or side effects. It's all a matter
of hierarchical versus flat design methodology. The second approach
fails completely at some stage when the total complexity increases. Then
starting from scratch is the way to go (perhaps hurdselect.c is not of
the second kind, though).