Re: [PATCH 04/10] Refactoring: new $automake_remake_options global variable.

[Stefano Lattarini]

On Monday 03 January 2011, Ralf Wildenhues wrote:
> ....
> [BIG CUT]
> ...

> So no, without an argument why the refactoring is necessary in order to
> fix the bug (rather than just understand it), I'm sorry but I have to
> reject it, on the grounds that it causes a net code quality regression.
>
Well, I never said that the refactoring was necessary in order to fix
the bug, only that it was necessary in order to have an "obvious" and
"natural" fix.

But it turns out I was wrong about this: following your indications,
I've been able to rebase the patch series removing patches 4, 5 and 6,
with only *very* mininal conflicts (all more or less spurious, and
easiliy solved), while fully witholding the bugfix.

I'm going to post this respin in this same thread soon.

> I'm not sure if it obvious, but I often try out code, or change code,
> when working on a bug fix; and then in the end, I see that a lot of the
> changes turn out to not be necessary to actually fix the bug.
>
Well, theoretically this is (or should be) obvious, but in practice, this
time I haven't been able to see by myself that ~half of the changes I did
were indeed useless for the bugfix.  Luckily you saw that on my behalf.

> The right strategy then is to go back and make a smaller, or even
> minimal, fix.
>
The new patch series should do so.

> The final patch may look a lot different from the way to get there.
>
Not really different this time (which IMHO is another good point in
favor of the smaller, less invasive patch).

> ...
> [MEGA CUT]
> ...

> [speaking about "options vs. global-options" distinction]
> Well, you don't answer the question why the current API is not suitable.
> 
Because it has a higer level of complexity than is required by current
automake codebase.  But this is material for another thread at this
point.

> Let's then go for simplicity and simplify the rebuilding rules.
>
Now this too is material for another thread.

> Again, bug fixing and tidying up are different tasks.
>
But not always IMHO (even if it turned out this time they were).

> > > Wow.  I didn't know it was perl-strict-clean to put barewords inside
> > > hash derefs a la $hash{word}.
> > >
> > Yes, for good or for bad, it is. Unfortunately, I cannot remember if
> > there's an easy way to forbid the use of barewords as hash keys
> > (google is not much helpulf on the issue).
> > 
> > > That seems like an undesirable feature
> > > to me, because it is easy to forget a $ as in $hash{$word}.
> > > Can we please not make use of this, it looks like a typo above already
> > > (with $strictness being a valid variable name)?  Thanks.
> > >
> > Fine with me; I amended the patch to do so.
> 
> Thank you, good we agree on this.
>
To be honest, I find this behaviour of perl quite useful and conventient in
situations which involve an heavy use of hashes.  I'd just like that there
was a switch to turn that behaviour off, to accomodate for situations where
its dangers outweight its convenience.

> ...
> [CUT]
> ...

> > > What is this supposed to do, and why?
> > >
> > Check that the automake instances invoked by the automake-generated
> > rebuild rules are passed the expected options (and only those options).
> > 
> > > Why is the same not achieved with a couple of greps in the usage
> > > part of the test below?
> > >
> > Because the rebuild rules are run "silenced" (their recipe is
> > prepended with a `@'), and use calls to "echo" to show what commands
> > they're about to perform [1].  These displayed commands might thus
> > not correspond to the real ones, and we want to check both then.
> 
> Wow.  Your mistrust is honorable, but it might go beyond what I would
> expect from a test suite.
>
But not beyond what *I* would expect ;-)

> Cheers,
> Ralf
> 

Thanks,
  Stefano