patchtest: simplify counts aggregation

> 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@gmail.com>
> ---
>  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

>
> > On Wed, Mar 08 2023, Felipe Contreras wrote:
> > > +++ b/t/Makefile
> > > +   '$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'
>
> > > +++ b/t/aggregate-results.sh
> > >
> > > -while read file
> > > +for file in $1
>
> Reading aggregate-results.sh alone looks wrong; but your patch is
> correct overall because of how it's invoked from the Makefile.
> I think that's too subtle...

>
>
> 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@gmail.com>
> > ---
> >  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:

> 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.

> On Wed, Mar 8, 2023 at 3:16 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@gmail.com>
>> > ---
>> >  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.

> > 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.