Re: [PATCH 5/7] Makefile: add 'check-sort' target

From

Jeff King <peff@peff.net>

Date

Mar 17, 2021, 17:32 UTC

Message-ID

<YFI9QzKMKLMXYoyz@coredump.intra.peff.net>

In-Reply-To

<87mtv2dk18.fsf@evledraar.gmail.com>

On Wed, Mar 17, 2021 at 01:47:15PM +0100, Ævar Arnfjörð Bjarmason wrote:

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

diff --git a/Makefile b/Makefile
index dfb0f1000f..3fa7893c8b 100644
--- a/Makefile
+++ b/Makefile
@@ -596,6 +596,16 @@ THIRD_PARTY_SOURCES =
 # interactive shell sessions without exporting it.
 unexport CDPATH
 
+# usage: $(call check-sort,VAR)
+# If this complains, then make sure the contents of VAR are ASCII-sorted.
+define check-sort-template
+SORTED_$1 = $$(sort $$($1))
+ifneq ($$($1),$$(SORTED_$1))
+$$(error "please sort and de-duplicate $1!")
+endif
+endef
+check-sort = $(eval $(call check-sort-template,$1))
+
 SCRIPT_SH += git-bisect.sh
 SCRIPT_SH += git-difftool--helper.sh
 SCRIPT_SH += git-filter-branch.sh
@@ -1037,6 +1047,7 @@ LIB_OBJS += ws.o
 LIB_OBJS += wt-status.o
 LIB_OBJS += xdiff-interface.o
 LIB_OBJS += zlib.o
+$(call check-sort,LIB_OBJS)
 
 BUILTIN_OBJS += builtin/add.o
 BUILTIN_OBJS += builtin/am.o

And then it's just a single-liner for each block that should be checked.
We haven't used $(call) or $(eval) yet in our Makefile, but past
discussions have reached the conclusion that they should be safe
(they're both in GNU make 3.80, which is the oldest version worth caring
about).

TBH, I'm a little on the fence on whether automatically checking this is
even worth the hassle. Doing the make function above was a fun
diversion, but already I think this discussion has taken more time than
people actually spend resolving conflicts on unsorted Makefile lists.

> > [...]
> > +	./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
>     +'

Again, much nicer, because it just happens as part of the test suite
(which I hope everyone is running after making changes). If we care
about the order in the source code, then this check implies that we are
not sorting the list internally before outputting it, but that is
probably reasonable.

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

Agreed. Automating away a tedious task is always better than
automatically checking that somebody did it right. :)

-Peff