On Monday 29 November 2010, Ralf Wildenhues wrote: > Hello Stefano, > Hi Ralf. Most of your objections are correct, and I'll apply the patch with just the original nits addressed. Still, I disagree with you on a minor point; see below if you are interested.
> * Stefano Lattarini wrote on Mon, Nov 29, 2010 at 01:55:27PM CET: > > On Monday 29 November 2010, Ralf Wildenhues wrote: > > > * Stefano Lattarini wrote on Thu, Nov 25, 2010 at 02:37:28PM CET: > > > > The attached patch is based off of maint, and intended for master. > > > > OK to apply? > > > > > > With nits addressed. > > > > > --- a/tests/colneq2.test > > > > +++ b/tests/colneq2.test > > > > @@ -20,14 +20,23 @@ > > > > > > > > set -e > > > > > > > > +cat >> configure.in << 'END' > > > > +AC_OUTPUT > > > > +END > > > > + > > > > cat > Makefile.am << 'END' > > > > t = a b c > > > > -EXTRA_DIST = $(t:=.test) > > > > > > Please leave the EXTRA_DIST line in. It is something different if > > > automake skips a normal variable containing this, and a variable that is > > > special to automake. > > > > > In truth, I was planning to add a new test about substitutions and > > indirections in definition of "special" automake variables, as TESTS, > > bin_PROGRAMS and the like; that's why I felt justified in removing the > > line above. But I ended up not completing and submitting that patch > > because the patch queue was (and is) already too long. > > Let's recap what happened: you sent a patch that takes away a (minor) > bit of test coverage; > (as an aside: it's not so minor in my opinion) > I approve the patch but ask you to keep that coverage in, now you update > the patch with an unrelated new change whose applicability depends on > completely different factors (namely deciding whether some behavior is > desirable or not), > Why should its applicatibility depend on such a decision? The testcase just serves to expose the current behaviour explicitly, without telling if it's desirable or not. That could be decided at a later time, and the testcase can be adjusted accordinlgy, if needed. That's not a good reason for not having the testcase from today. On the other hand, as you notice below, such a testcase should have been submitted in a follow-up patch. > plus also a related new test coverage patch. > This procedure is not helpful. Instead of turning N pending patches > into N+2 pending patches, why not turn it into N-1 pending patches. > And THEN, send a new patch doing whatever else you wanted to do. Make > that independent. > > I approved the original submission with nits addressed, so why not > just apply it with those nits addressed, then we can at least forget > about that part of the story and concentrate on the rest only? > OK, you are absolutely right here. Sorry for not having done that right away. > And keep making progress, instead of wasting both patch > production time on your side and review time on my side? > > > Now I have completed at least the testcase for EXTRA_DIST (see > > attachement); so, OK to keep this change if that testcase is added > > in a prior commit? > > No. Sorry. > > > > Uh, oh. Thin ice. This is OK, but we gotta remember that ... > The new testcase would be a good reminder, BTW ;-) > > > ... it won't work reliably if some of these variables are actually > > > automake-set before they are overridden. > > > > automake generally orders all of its > > > variable settings before all of the user ones (so the user ones are > > > preferred). When I override, e.g., libdir here, however, it doesn't > > > get reordered to the user part. I wonder whether that is a bug in > > > automake. > > > > > I've added a new (xfailing) testcase to expose this bug/limitation. > > OK to apply it? > > No. The above paragraph was meant as an incentive to think, dig, and > find out whether this is a bug or not, and if it is, whether it is > possible in general to fix it without harming other legitimate use cases > or not. The test case does not address that question. > Nor it intends to (but your observations here make even clearer that the testcase should have been introduced in a separate patch). > Not everything can be answered with a test case, and no, I don't want > test cases that basically ask me whether the test case is right or > wrong. > I think some tests of this kind could be helpful anyway (but this debate will have to wait for the proper thread -- I'm not doing the same mistake twice in the same day ;-) Regards, Stefano
