On Wed, Mar 08 2023, Felipe Contreras wrote:
> On Wed, Mar 8, 2023 at 3:16 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>>
>>
>> On Wed, Mar 08 2023, Felipe Contreras wrote:
>>
>> > When the list of files as input was implemented in 6508eedf67
>> > (t/aggregate-results: accomodate systems with small max argument list
>> > length, 2010-06-01), a much simpler solution wasn't considered.
>> >
>> > Let's just pass the pattern as an argument.
>> >
>> > Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
>> > ---
>> > t/Makefile | 4 +---
>> > t/aggregate-results.sh | 2 +-
>> > 2 files changed, 2 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/t/Makefile b/t/Makefile
>> > index 2c2b252240..6bc878558f 100644
>> > --- a/t/Makefile
>> > +++ b/t/Makefile
>> > @@ -140,9 +140,7 @@ aggregate-results-and-cleanup: $(T)
>> > $(MAKE) clean
>> >
>> > aggregate-results:
>> > - for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \
>> > - echo "$$f"; \
>> > - done | '$(SHELL_PATH_SQ)' ./aggregate-results.sh
>> > + '$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'
>> >
>> > valgrind:
>> > $(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
>> > diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh
>> > index 7f2b83bdc8..2efc2c37cd 100755
>> > --- a/t/aggregate-results.sh
>> > +++ b/t/aggregate-results.sh
>> > @@ -8,7 +8,7 @@ broken=0
>> > total=0
>> > missing_prereq=
>> >
>> > -while read file
>> > +for file in $1
>> > do
>> > while read type value
>> > do
>>
>> This leaves this code in contrib presumably broken:
>
> Right, I didn't know the "contrib" code called that.
>
> Easy fix.
>
>> But overall I like this direction, if we can just change that contrib
>> Makefile as well to use the new mode the script excepts.
>>
>> I think we can go even further here, and just pass the
>> $(TEST_RESULTS_DIRECTORY_SQ) as an argument to the script, then have it
>> do something like (untested):
>>
>> results_dir=$1
>> for file in "$results_dir"/t*-*.counts
>>
>> Which I think is a bit more obvious, and since the only task of the
>> script is to do exactly this, there's no reason not to have it do that
>> search by itself.
>
> Or just:
>
> for file in "${TEST_OUTPUT_DIRECTORY-.}"/test-results/t*-*.counts
>
> And don't pass anything.
Yeah, I think that would work, but at least on an ad-hoc basis I've
sometimes saved away the "test-results" directory
(e.g. "test-results.prev").
I think it would be useful if the script part of our tooling was happy
to accept any name for such a directory, and then examined its contents.
But I don't feel strongly about it, and I don't use aggregate-results.sh
in particular (I always use "prove").
