From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Wed, 17 Mar 2021 12:47:15 GMT
Subject: Re: [PATCH 5/7] Makefile: add 'check-sort' target
Message-ID: <87mtv2dk18.fsf@evledraar.gmail.com>
In-Reply-To: <5088e93d76e44de9d079b7b2296b8c810828a2f5.1615856156.git.liu.denton@gmail.com>


On Tue, Mar 16 2021, Denton Liu wrote:

> In the previous few commits, we sorted many lists into ASCII-order. In
> order to ensure that they remain that way, add the 'check-sort' target.
>
> The check-sort.perl program ensures that consecutive lines that match
> the same regex are sorted in ASCII-order. The 'check-sort' target runs
> the check-sort.perl program on some files which are known to contain
> sorted lists.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>
> Notes:
>     Full disclaimer: this is the first time I've written anything in Perl.
>     Please let me know if I'm doing anything unconventional :)
>
>  Makefile        | 25 +++++++++++++++++++++++++
>  check-sort.perl | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>  create mode 100755 check-sort.perl
>
> diff --git a/Makefile b/Makefile
> index 5832aa33da..b23dff384d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3240,6 +3240,31 @@ check-docs::
>  check-builtins::
>  	./check-builtins.sh
>  
> +.PHONY: check-sort
> +check-sort::
> +	./check-sort.perl \
> +		'ALL_COMMANDS \+=' \
> +		'ALL_COMMANDS_TO_INSTALL \+=' \
> +		'BINDIR_PROGRAMS_NEED_X \+=' \
> +		'BINDIR_PROGRAMS_NO_X \+=' \
> +		'BUILTIN_OBJS \+=' \
> +		'BUILT_INS \+=' \
> +		'FUZZ_OBJS \+=' \
> +		'GENERATED_H \+=' \
> +		'LIB_OBJS \+=' \
> +		'SCRIPT_LIB \+=' \
> +		'SCRIPT_PERL \+=' \
> +		'SCRIPT_PYTHON \+=' \
> +		'SCRIPT_SH \+=' \
> +		'TEST_BUILTINS_OBJS \+=' \
> +		'TEST_PROGRAMS_NEED_X \+=' \
> +		'THIRD_PARTY_SOURCES \+=' \
> +		'XDIFF_OBJS \+=' \
> +		<Makefile

Why does this part need to be a Perl script at all? We can check this in
the makefile itself. Make has a sort function and string comparisons,
e.g.:

LIB_OBJS_SORTED =
LIB_OBJS_SORTED += $(sort $(LIB_OBJS))
ifneq ("$(LIB_OBJS)", "$(LIB_OBJS_SORTED)")
$(error "please sort and de-duplicate LIB_OBJS!")
endif

This will fail/pass before/after your patches. Note that make's sort
isn't just a sort, it also de-deplicates (not that we're likely to have
that issue).

> [...]
> +	./check-sort.perl '\t\{ "[^"]*",' <git.c

This last one you can IMO be done better as (or if we want to be more
anal, we could make git die on startup if it's not true):
    
    diff --git a/t/t0012-help.sh b/t/t0012-help.sh
    index 5679e29c62..5bd2ebceca 100755
    --- a/t/t0012-help.sh
    +++ b/t/t0012-help.sh
    @@ -77,6 +77,11 @@ test_expect_success 'generate builtin list' '
            git --list-cmds=builtins >builtins
     '
     
    +test_expect_success 'list of builtins in git.c should be sorted' '
    +       sort builtins >sorted &&
    +       test_cmp sorted builtins
    +'
    +
     while read builtin
     do
            test_expect_success "$builtin can handle -h" '

Which just leaves:

> +	./check-sort.perl 'int cmd_[^(]*\(' <builtin.h
> +	./check-sort.perl 'int cmd__[^(]*\(' <t/helper/test-tool.h

As something that can't be done in the Makefile itself or that we're
already extracting from the tests.

Both of those IMO would be better handled down the line by making the
relevant part of these files generated from the data in the Makefile, at
which point we'd have no need for the external perl script.