[PATCH 0/7] Sort lists and add static-analysis

As a follow-up to [0], sort some file lists and create a static-analysis
check to ensure that those lists don't ever become un-sorted.

[0]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2010081156350.50@tvgsbejvaqbjf.bet/

Denton Liu (7):
  Makefile: mark 'check-builtins' as a .PHONY target
  Makefile: ASCII-sort LIB_OBJS
  builtin.h: ASCII-sort list of functions
  test-tool.h: ASCII-sort list of functions
  Makefile: add 'check-sort' target
  ci/run-static-analysis.sh: make check-builtins
  ci/run-static-analysis.sh: make check-sort

 Makefile                  | 30 ++++++++++++++++++++++++++++--
 builtin.h                 | 22 +++++++++++-----------
 check-sort.perl           | 31 +++++++++++++++++++++++++++++++
 ci/run-static-analysis.sh |  2 +-
 t/helper/test-tool.h      |  6 +++---
 5 files changed, 74 insertions(+), 17 deletions(-)
 create mode 100755 check-sort.perl

-- 
2.31.0.rc2.261.g7f71774620

The list of builtin functions have, over time, gotten slightly out of
ASCII order. Sort this list to bring them back into order.

ASCII sorting was chosen over strict alphabetical order for the same
reason as 805d9eaf5e (Makefile: ASCII-sort += lists, 2020-03-21): the
purpose of maintaining the sorted list is to ensure line insertions are
deterministic. By using ASCII ordering, it is more easily mechanically
reproducible in the future, such as by using :sort in Vim.

This patch is best viewed with `--color-moved`.

Signed-off-by: Denton Liu <redacted>
---
 builtin.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin.h b/builtin.h
index b6ce981b73..dd6beff6b2 100644
--- a/builtin.h
+++ b/builtin.h

@@ -122,16 +122,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix);
 int cmd_bugreport(int argc, const char **argv, const char *prefix);
 int cmd_bundle(int argc, const char **argv, const char *prefix);
 int cmd_cat_file(int argc, const char **argv, const char *prefix);
-int cmd_checkout(int argc, const char **argv, const char *prefix);
-int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 int cmd_check_attr(int argc, const char **argv, const char *prefix);
 int cmd_check_ignore(int argc, const char **argv, const char *prefix);
 int cmd_check_mailmap(int argc, const char **argv, const char *prefix);
 int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
+int cmd_checkout(int argc, const char **argv, const char *prefix);
+int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 int cmd_cherry(int argc, const char **argv, const char *prefix);
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
-int cmd_clone(int argc, const char **argv, const char *prefix);
 int cmd_clean(int argc, const char **argv, const char *prefix);
+int cmd_clone(int argc, const char **argv, const char *prefix);
 int cmd_column(int argc, const char **argv, const char *prefix);
 int cmd_commit(int argc, const char **argv, const char *prefix);
 int cmd_commit_graph(int argc, const char **argv, const char *prefix);

@@ -143,9 +143,9 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix);
 int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix);
 int cmd_credential_store(int argc, const char **argv, const char *prefix);
 int cmd_describe(int argc, const char **argv, const char *prefix);
+int cmd_diff(int argc, const char **argv, const char *prefix);
 int cmd_diff_files(int argc, const char **argv, const char *prefix);
 int cmd_diff_index(int argc, const char **argv, const char *prefix);
-int cmd_diff(int argc, const char **argv, const char *prefix);
 int cmd_diff_tree(int argc, const char **argv, const char *prefix);
 int cmd_difftool(int argc, const char **argv, const char *prefix);
 int cmd_env__helper(int argc, const char **argv, const char *prefix);

@@ -169,16 +169,16 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);
 int cmd_log(int argc, const char **argv, const char *prefix);
 int cmd_log_reflog(int argc, const char **argv, const char *prefix);
 int cmd_ls_files(int argc, const char **argv, const char *prefix);
-int cmd_ls_tree(int argc, const char **argv, const char *prefix);
 int cmd_ls_remote(int argc, const char **argv, const char *prefix);
+int cmd_ls_tree(int argc, const char **argv, const char *prefix);
 int cmd_mailinfo(int argc, const char **argv, const char *prefix);
 int cmd_mailsplit(int argc, const char **argv, const char *prefix);
 int cmd_maintenance(int argc, const char **argv, const char *prefix);
 int cmd_merge(int argc, const char **argv, const char *prefix);
 int cmd_merge_base(int argc, const char **argv, const char *prefix);
+int cmd_merge_file(int argc, const char **argv, const char *prefix);
 int cmd_merge_index(int argc, const char **argv, const char *prefix);
 int cmd_merge_ours(int argc, const char **argv, const char *prefix);
-int cmd_merge_file(int argc, const char **argv, const char *prefix);
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
 int cmd_merge_tree(int argc, const char **argv, const char *prefix);
 int cmd_mktag(int argc, const char **argv, const char *prefix);

@@ -189,6 +189,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix);
 int cmd_notes(int argc, const char **argv, const char *prefix);
 int cmd_pack_objects(int argc, const char **argv, const char *prefix);
 int cmd_pack_redundant(int argc, const char **argv, const char *prefix);
+int cmd_pack_refs(int argc, const char **argv, const char *prefix);
 int cmd_patch_id(int argc, const char **argv, const char *prefix);
 int cmd_prune(int argc, const char **argv, const char *prefix);
 int cmd_prune_packed(int argc, const char **argv, const char *prefix);

@@ -204,6 +205,7 @@ int cmd_remote(int argc, const char **argv, const char *prefix);
 int cmd_remote_ext(int argc, const char **argv, const char *prefix);
 int cmd_remote_fd(int argc, const char **argv, const char *prefix);
 int cmd_repack(int argc, const char **argv, const char *prefix);
+int cmd_replace(int argc, const char **argv, const char *prefix);
 int cmd_rerere(int argc, const char **argv, const char *prefix);
 int cmd_reset(int argc, const char **argv, const char *prefix);
 int cmd_restore(int argc, const char **argv, const char *prefix);

@@ -216,9 +218,10 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix);
 int cmd_show(int argc, const char **argv, const char *prefix);
 int cmd_show_branch(int argc, const char **argv, const char *prefix);
 int cmd_show_index(int argc, const char **argv, const char *prefix);
+int cmd_show_ref(int argc, const char **argv, const char *prefix);
 int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);
-int cmd_status(int argc, const char **argv, const char *prefix);
 int cmd_stash(int argc, const char **argv, const char *prefix);
+int cmd_status(int argc, const char **argv, const char *prefix);
 int cmd_stripspace(int argc, const char **argv, const char *prefix);
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
 int cmd_switch(int argc, const char **argv, const char *prefix);

@@ -235,14 +238,11 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
 int cmd_upload_pack(int argc, const char **argv, const char *prefix);
 int cmd_var(int argc, const char **argv, const char *prefix);
 int cmd_verify_commit(int argc, const char **argv, const char *prefix);
+int cmd_verify_pack(int argc, const char **argv, const char *prefix);
 int cmd_verify_tag(int argc, const char **argv, const char *prefix);
 int cmd_version(int argc, const char **argv, const char *prefix);
 int cmd_whatchanged(int argc, const char **argv, const char *prefix);
 int cmd_worktree(int argc, const char **argv, const char *prefix);
 int cmd_write_tree(int argc, const char **argv, const char *prefix);
-int cmd_verify_pack(int argc, const char **argv, const char *prefix);
-int cmd_show_ref(int argc, const char **argv, const char *prefix);
-int cmd_pack_refs(int argc, const char **argv, const char *prefix);
-int cmd_replace(int argc, const char **argv, const char *prefix);
 
 #endif

-- 
2.31.0.rc2.261.g7f71774620

In 805d9eaf5e (Makefile: ASCII-sort += lists, 2020-03-21), the += lists
in the Makefile were sorted into ASCII order. Since then, more out of
order elements have been introduced. Sort these lists back into ASCII
order.

This patch is best viewed with `--color-moved`.

Signed-off-by: Denton Liu <redacted>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 6dbecdb606..5832aa33da 100644
--- a/Makefile
+++ b/Makefile

@@ -858,8 +858,8 @@ LIB_OBJS += date.o
 LIB_OBJS += decorate.o
 LIB_OBJS += delta-islands.o
 LIB_OBJS += diff-delta.o
-LIB_OBJS += diff-merges.o
 LIB_OBJS += diff-lib.o
+LIB_OBJS += diff-merges.o
 LIB_OBJS += diff-no-index.o
 LIB_OBJS += diff.o
 LIB_OBJS += diffcore-break.o

@@ -910,8 +910,8 @@ LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
 LIB_OBJS += mem-pool.o
 LIB_OBJS += merge-blobs.o
-LIB_OBJS += merge-ort.o
 LIB_OBJS += merge-ort-wrappers.o
+LIB_OBJS += merge-ort.o
 LIB_OBJS += merge-recursive.o
 LIB_OBJS += merge.o
 LIB_OBJS += mergesort.o

-- 
2.31.0.rc2.261.g7f71774620

Then convention in Git's Makefile is to mark targets which do not
actually generate any precious files (such as static checking targets).
.PHONY enables the target to still run, even if a file is erroneously
created with the same name as the target.

Mark 'check-builtins' as a .PHONY target.

Signed-off-by: Denton Liu <redacted>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index dfb0f1000f..6dbecdb606 100644
--- a/Makefile
+++ b/Makefile

@@ -3236,6 +3236,7 @@ check-docs::
 
 ### Make sure built-ins do not have dups and listed in git.c
 #
+.PHONY: check-builtins
 check-builtins::
 	./check-builtins.sh
 

-- 
2.31.0.rc2.261.g7f71774620

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

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
+	./check-sort.perl 'int cmd_[^(]*\(' <builtin.h
+	./check-sort.perl 'int cmd__[^(]*\(' <t/helper/test-tool.h
+	./check-sort.perl '\t\{ "[^"]*",' <git.c
+
 ### Test suite coverage testing
 #
 .PHONY: coverage coverage-clean coverage-compile coverage-test coverage-report

diff --git a/check-sort.perl b/check-sort.perl
new file mode 100755
index 0000000000..cd723db14d
--- /dev/null
+++ b/check-sort.perl

@@ -0,0 +1,31 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+my @regexes = map { qr/^$_/ } @ARGV;
+my $last_regex = 0;
+my $last_line = '';
+
+while (<STDIN>) {
+	my $matched = 0;
+	chomp;
+
+	for my $regex (@regexes) {
+		next unless $_ =~ $regex;
+
+		if ($last_regex == $regex) {
+			die "duplicate lines: '$_'\n" unless $last_line ne $_;
+			die "unsorted lines: '$last_line' before '$_'\n" unless $last_line lt $_;
+		}
+
+		$matched = 1;
+		$last_regex = $regex;
+		$last_line = $_;
+	}
+
+	unless ($matched) {
+		$last_regex = 0;
+		$last_line = '';
+	}
+}

-- 
2.31.0.rc2.261.g7f71774620

To ensure that any newly introduced builtins are in git.c command list
and that no duplicate script files exist, run the 'check-builtins'
target as part of static-analysis.

Signed-off-by: Denton Liu <redacted>
---
 ci/run-static-analysis.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
index 65bcebda41..1ae122fc70 100755
--- a/ci/run-static-analysis.sh
+++ b/ci/run-static-analysis.sh

@@ -26,7 +26,7 @@ then
 	exit 1
 fi
 
-make hdr-check ||
+make hdr-check check-builtins ||
 exit 1
 
 save_good_tree

-- 
2.31.0.rc2.261.g7f71774620

The list of test-tool functions have, over time, gotten slightly out of
ASCII order. Sort this list to bring them back into order.

ASCII sorting was chosen over strict alphabetical order for the same
reason as 805d9eaf5e (Makefile: ASCII-sort += lists, 2020-03-21): the
purpose of maintaining the sorted list is to ensure line insertions are
deterministic. By using ASCII ordering, it is more easily mechanically
reproducible in the future, such as by using :sort in Vim.

This patch is best viewed with `--color-moved`.

Signed-off-by: Denton Liu <redacted>
---
 t/helper/test-tool.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 28072c0ad5..9856e84149 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h

@@ -22,14 +22,15 @@ int cmd__example_decorate(int argc, const char **argv);
 int cmd__fast_rebase(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);
 int cmd__genzeros(int argc, const char **argv);
-int cmd__hashmap(int argc, const char **argv);
 int cmd__hash_speed(int argc, const char **argv);
+int cmd__hashmap(int argc, const char **argv);
 int cmd__index_version(int argc, const char **argv);
 int cmd__json_writer(int argc, const char **argv);
 int cmd__lazy_init_name_hash(int argc, const char **argv);
 int cmd__match_trees(int argc, const char **argv);
 int cmd__mergesort(int argc, const char **argv);
 int cmd__mktemp(int argc, const char **argv);
+int cmd__oid_array(int argc, const char **argv);
 int cmd__oidmap(int argc, const char **argv);
 int cmd__online_cpus(int argc, const char **argv);
 int cmd__parse_options(int argc, const char **argv);

@@ -52,7 +53,6 @@ int cmd__run_command(int argc, const char **argv);
 int cmd__scrap_cache_tree(int argc, const char **argv);
 int cmd__serve_v2(int argc, const char **argv);
 int cmd__sha1(int argc, const char **argv);
-int cmd__oid_array(int argc, const char **argv);
 int cmd__sha256(int argc, const char **argv);
 int cmd__sigchain(int argc, const char **argv);
 int cmd__strcmp_offset(int argc, const char **argv);

@@ -62,8 +62,8 @@ int cmd__submodule_nested_repo_config(int argc, const char **argv);
 int cmd__subprocess(int argc, const char **argv);
 int cmd__trace2(int argc, const char **argv);
 int cmd__urlmatch_normalization(int argc, const char **argv);
-int cmd__xml_encode(int argc, const char **argv);
 int cmd__wildmatch(int argc, const char **argv);
+int cmd__xml_encode(int argc, const char **argv);
 #ifdef GIT_WINDOWS_NATIVE
 int cmd__windows_named_pipe(int argc, const char **argv);
 #endif

-- 
2.31.0.rc2.261.g7f71774620

To ensure that lists in some files remain in sorted order, run the
'check-sort' target as part of static-analysis.

Signed-off-by: Denton Liu <redacted>
---
 ci/run-static-analysis.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
index 1ae122fc70..92437c09df 100755
--- a/ci/run-static-analysis.sh
+++ b/ci/run-static-analysis.sh

@@ -26,7 +26,7 @@ then
 	exit 1
 fi
 
-make hdr-check check-builtins ||
+make hdr-check check-builtins check-sort ||
 exit 1
 
 save_good_tree

-- 
2.31.0.rc2.261.g7f71774620

On Mon, Mar 15, 2021 at 8:57 PM Denton Liu [off-list ref] wrote:

Then convention in Git's Makefile is to mark targets which do not

s/Then/The/

actually generate any precious files (such as static checking targets).
.PHONY enables the target to still run, even if a file is erroneously
created with the same name as the target.

Mark 'check-builtins' as a .PHONY target.

Signed-off-by: Denton Liu <redacted>

On Mon, Mar 15, 2021 at 8:57 PM Denton Liu [off-list ref] 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.
[...]
Signed-off-by: Denton Liu <redacted>
---
+my @regexes = map { qr/^$_/ } @ARGV;
+my $last_regex = 0;
+my $last_line = '';
+while (<STDIN>) {
+       my $matched = 0;
+       chomp;
+       for my $regex (@regexes) {
+               next unless $_ =~ $regex;
+               if ($last_regex == $regex) {
+                       die "duplicate lines: '$_'\n" unless $last_line ne $_;
+                       die "unsorted lines: '$last_line' before '$_'\n" unless $last_line lt $_;
+               }
+               $matched = 1;
+               $last_regex = $regex;
+               $last_line = $_;
+       }
+       unless ($matched) {
+               $last_regex = 0;
+               $last_line = '';
+       }
+}

This is, of course, endlessly bikesheddable. Here is a shorter -- and,
at least for me, easier to understand -- way to do it:

    my $rc = 0;
    chomp(my @all = <STDIN>);
    foreach my $needle (@ARGV) {
        my @lines = grep(/^$needle/, @all);
        if (join("\n", @lines) ne join("\n", sort @lines)) {
            print "'$needle' lines not sorted\n";
            $rc = 1;
        }
    }
    exit $rc;

By the way, it might be a good idea to also print the filename in
which the problem occurred. Such context can be important for the
person trying to track down the complaint. To do so, you'd probably
want to pass the filename as an argument, and open and read the file
rather than sending it only as standard-input.

Hi Eric,

On Tue, Mar 16, 2021 at 02:37:16AM -0400, Eric Sunshine wrote:

On Mon, Mar 15, 2021 at 8:57 PM Denton Liu [off-list ref] wrote:

quoted

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.
[...]
Signed-off-by: Denton Liu <redacted>
---
+my @regexes = map { qr/^$_/ } @ARGV;
+my $last_regex = 0;
+my $last_line = '';
+while (<STDIN>) {
+       my $matched = 0;
+       chomp;
+       for my $regex (@regexes) {
+               next unless $_ =~ $regex;
+               if ($last_regex == $regex) {
+                       die "duplicate lines: '$_'\n" unless $last_line ne $_;
+                       die "unsorted lines: '$last_line' before '$_'\n" unless $last_line lt $_;
+               }
+               $matched = 1;
+               $last_regex = $regex;
+               $last_line = $_;
+       }
+       unless ($matched) {
+               $last_regex = 0;
+               $last_line = '';
+       }
+}

This is, of course, endlessly bikesheddable. Here is a shorter -- and,
at least for me, easier to understand -- way to do it:

    my $rc = 0;
    chomp(my @all = <STDIN>);
    foreach my $needle (@ARGV) {
        my @lines = grep(/^$needle/, @all);
        if (join("\n", @lines) ne join("\n", sort @lines)) {
            print "'$needle' lines not sorted\n";
            $rc = 1;
        }
    }
    exit $rc;

That's pretty clever, thanks for showing me how it's done :)

However, the reason I wrote it out the way that I did is because my code
ensures that consecutive lines matching the regex are sorted but if
there are any breaks between matching regex lines, it will consider them
separate blocks. Just taking all the lines fails in the case of
`LIB_OBJS \+=` in Makefile since we have

	LIB_OBJS += zlib.o

	[... many intervening lines ...]

	LIB_OBJS += $(COMPAT_OBJS)

and that is technically unsorted. That being said, I don't really like
my current approach that much.

I think I have two better options:

	1. Tighten up the regexes so that it excludes the
	   $(COMPAT_OBJS). I don't want to be too strict, though,
	   because if we end up not matching a line it might end up
	   unsorted.

	2. Consider blank lines to be block separators and only consider
	   it to be sorted if the text matching regexes within a block
	   are sorted.

Now that I've written that all out, I think I like option 1 more,
although I could definitely be convinced to go either way.

By the way, it might be a good idea to also print the filename in
which the problem occurred. Such context can be important for the
person trying to track down the complaint. To do so, you'd probably
want to pass the filename as an argument, and open and read the file
rather than sending it only as standard-input.

Agreed.

Thanks,
Denton

On 16/03/21 07.56, Denton Liu wrote:

As a follow-up to [0], sort some file lists and create a static-analysis
check to ensure that those lists don't ever become un-sorted.

[0]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2010081156350.50@tvgsbejvaqbjf.bet/

Remark for future patches that touch these file lists: please try to keep the ASCII
sort order when adding something.

But anyways, grazie Denton.

Denton Liu (7):
   Makefile: mark 'check-builtins' as a .PHONY target
   Makefile: ASCII-sort LIB_OBJS
   builtin.h: ASCII-sort list of functions
   test-tool.h: ASCII-sort list of functions
   Makefile: add 'check-sort' target
   ci/run-static-analysis.sh: make check-builtins
   ci/run-static-analysis.sh: make check-sort

  Makefile                  | 30 ++++++++++++++++++++++++++++--
  builtin.h                 | 22 +++++++++++-----------
  check-sort.perl           | 31 +++++++++++++++++++++++++++++++
  ci/run-static-analysis.sh |  2 +-
  t/helper/test-tool.h      |  6 +++---
  5 files changed, 74 insertions(+), 17 deletions(-)
  create mode 100755 check-sort.perl

-- 
An old man doll... just what I always wanted! - Clara

On Tue, Mar 16 2021, Denton Liu 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).

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

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

I like that much better, if only because it runs quickly and
automatically (as opposed to much later as part of some CI process).
Reducing the length of the feedback loop makes automated checks much
less annoying.

It doesn't indicate which lines are out of order or duplicated. We could
probably add some $(shell) magic to dump both sides to diff inside the
ifneq. That's getting unwieldy to use in each site, but I wonder if
something like this solves that:

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.

quoted

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

quoted

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

On Wed, Mar 17 2021, Jeff King wrote:

...also this sort of thing can be guarded by "ifdef DEVELOPER" or
something, which AFAICT (from trying to introduce syntax errors etc.)
will get parsed first, before "make" even tries to parse what's within
the ifdef.

So if we have bells & whistles in that sort of setup it can be less
portable than the Makefile in general..

On Wed, Mar 17, 2021 at 1:34 PM Jeff King [off-list ref] wrote:

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.

I had the same reaction. Like you, I jumped in for the fun diversion.
It allowed me to flex my Perl muscle a bit which has atrophied, but an
out-of-order item here and there is such a minor concern, especially
since they don't impact correctness, that I worry that such a CI job
would be more hassle than it's worth. Making the feedback loop
tighter, as discussed elsewhere in this thread, makes the idea of the
automated check a bit more palatable.

On Wed, Mar 17, 2021 at 05:48:18PM -0400, Eric Sunshine wrote:

On Wed, Mar 17, 2021 at 1:34 PM Jeff King [off-list ref] wrote:

quoted

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.

I had the same reaction. Like you, I jumped in for the fun diversion.
It allowed me to flex my Perl muscle a bit which has atrophied, but an
out-of-order item here and there is such a minor concern, especially
since they don't impact correctness, that I worry that such a CI job
would be more hassle than it's worth. Making the feedback loop
tighter, as discussed elsewhere in this thread, makes the idea of the
automated check a bit more palatable.

There's an implication in what both of us said that I think is worth
calling out explicitly: I would not feel the same about a problem that
impacts the correctness of the resulting code. E.g., if the list of
builtins were used with a binary search, then an unsorted list would
produce the wrong result. And that would be worth testing.

It just seems that the stakes here are much lower.

-Peff