From: Fabian Stelzer <fs@gigacodes.de>
Date: Mon, 13 Dec 2021 16:14:19 GMT
Subject: Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test
Message-ID: <20211213161419.cwamlunrgz3adrhh@fs>
In-Reply-To: <CAPig+cSXHBMgOUycL0cXuVCb_PJ2=x2w4wUkc7eQQueyk=0Uzw@mail.gmail.com>

On 13.12.2021 11:02, Eric Sunshine wrote:
>On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> Personally i find the initial for loop variant to be the most readable.
>> Ævars makefile targets could be very nice too, but especially:
>>
>> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
>> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
>> +       $(CHAINLINT) <$< | \
>> +        sed -e '/^# LINT: /d' >$@ && \
>> +       diff -u $(basename $<).expect $@
>>
>> i find very hard to grasp :/
>> I have no idea what is going on here: `<$< |` ?
>
>Ya, that line-noise is an unfortunate combination of shell and
>Makefile gobbledygook. The `$<` is effectively the source file (the
>file being passed into chainlint.sed), and the rest of it is just
>normal shell. `<` is redirection (using the source file `$<` as
>stdin), and `|` is the pipe operator (sending the output of
>chainlint.sed to another `sed`), and capturing it all via shell `>`
>redirection in `$@` which is the Makefile variable for the target
>file.
>

Thanks, that explains it nicely. I'm not familiar enough with makefile 
syntax.

>Anyhow, although the commit message tries to sell this change as some
>sort of optimization, it's really in preparation for the new chainlint
>which wants to check all tests in all files with a single invocation
>rather than being invoked over and over and over. The self-test files
>also require more preprocessing to work with the new chainlint, so the
>implementation of `check-chainlint` gets rather more complex once the
>end state is reached. I'll think about it a bit, but at the moment,
>I'm still leaning toward this intermediate step as being beneficial to
>reaching the end state. However, my opinion could change since the way
>this is done here was probably influenced by an earlier iteration of
>the new chainlint, but now that the implementation of the new
>chainlint is concrete, it may not be especially important to do it
>this way.

I don't mind much either way and i tend to favor the efficient variant as 
well. On the other hand i can also see some bug mixing up the contents of 
these files producing a huge diff with no good indication for the developer 
of what has happened.