threads / patch / 55326

patch, 7 partsSort lists and add static-analysis

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

## tl;dr

24 messages between Mar 16, 2021 and Mar 17, 2021. Diffs are folded; open one to read it.

replies: 23people: 6as markdown or json

Denton Liu· Mar 16, 2021, 00:56 UTC · lore

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

Denton Liu· Mar 16, 2021, 00:56 UTC · re: Denton Liu · lore

[PATCH 3/7] builtin.h: ASCII-sort list of functions

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 <liu.denton@gmail.com>
---
 builtin.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Show changes to builtin.h +11 −11

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

Junio C Hamano· Mar 17, 2021, 17:51 UTC · re: Denton Liu · lore

Re: [PATCH 3/7] builtin.h: ASCII-sort list of functions

Denton Liu <liu.denton@gmail.com> writes:

Show 5 quoted lines

> 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 happens to work only because '(' sorts way earlier than '_'. Otherwise we'd probably be sorting with 'sort -t\( -k1' or something like that.

I do not know if it worth mentioning, because it is unlikely that the sort order between '(' and '_' would ever change ;-).

Show 105 quoted lines

> This patch is best viewed with `--color-moved`.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  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

Denton Liu· Mar 16, 2021, 00:56 UTC · re: Denton Liu · lore

[PATCH 2/7] Makefile: ASCII-sort LIB_OBJS

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 <liu.denton@gmail.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to Makefile +2 −2

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

Denton Liu· Mar 16, 2021, 00:56 UTC · re: Denton Liu · lore

[PATCH 1/7] Makefile: mark 'check-builtins' as a .PHONY target

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 <liu.denton@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

Show changes to Makefile +1 −0

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

Eric Sunshine· Mar 16, 2021, 04:59 UTC · re: Denton Liu · lore

Re: [PATCH 1/7] Makefile: mark 'check-builtins' as a .PHONY target

On Mon, Mar 15, 2021 at 8:57 PM Denton Liu <liu.denton@gmail.com> wrote:

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

s/Then/The/

Show 7 quoted lines

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

Junio C Hamano· Mar 17, 2021, 17:47 UTC · re: Denton Liu · lore

Re: [PATCH 1/7] Makefile: mark 'check-builtins' as a .PHONY target

Denton Liu <liu.denton@gmail.com> writes:

> Then convention in Git's Makefile is to mark targets which do not
> actually generate any precious files (such as static checking targets).

s/Then/The/ has been pointed out, but I am not sure what "precious" means here. I do not think you are referring to the distinction between ordinary targets and .PRECIOUS targets here, but the use of the word makes readers wonder if some interaction with the .PRECIOUS marking was taken into consideration.

> .PHONY enables the target to still run, even if a file is erroneously
> created with the same name as the target.

True. Another convention is to "build" such a target with double-colon rule. The existing code already follows the convention, so there is nothing the patch text needs to do, but if we explain the change in the log message to make the code follow conventions, we probably should mention it as well for completeness.

Thanks.

Show 18 quoted lines

> Mark 'check-builtins' as a .PHONY target.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  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

Denton Liu· Mar 16, 2021, 00:56 UTC · re: Denton Liu · lore

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

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

Show changes to 2 files +56 −0

Makefile, 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

Eric Sunshine· Mar 16, 2021, 06:37 UTC · re: Denton Liu · lore

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

On Mon, Mar 15, 2021 at 8:57 PM Denton Liu <liu.denton@gmail.com> wrote:

Show 26 quoted lines

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

Denton Liu· Mar 17, 2021, 09:50 UTC · re: Eric Sunshine · lore

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

Hi Eric,

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

Show 41 quoted lines

> On Mon, Mar 15, 2021 at 8:57 PM Denton Liu <liu.denton@gmail.com> 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 <liu.denton@gmail.com>
> > ---
> > +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.

Show 5 quoted lines

> 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

Ævar Arnfjörð Bjarmason· Mar 17, 2021, 12:47 UTC · re: Denton Liu · lore

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

On Tue, Mar 16 2021, Denton Liu wrote:

Show 49 quoted lines

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

Jeff King· Mar 17, 2021, 17:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Show 13 quoted lines

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

Show changes to Makefile +11 −1

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

Ævar Arnfjörð Bjarmason· Mar 17, 2021, 17:42 UTC · re: Jeff King · lore

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

On Wed, Mar 17 2021, Jeff King wrote:

Show 17 quoted lines

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

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

Eric Sunshine· Mar 17, 2021, 21:48 UTC · re: Jeff King · lore

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

On Wed, Mar 17, 2021 at 1:34 PM Jeff King <peff@peff.net> 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.

Jeff King· Mar 17, 2021, 22:01 UTC · re: Eric Sunshine · lore

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

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

Show 13 quoted lines

> On Wed, Mar 17, 2021 at 1:34 PM Jeff King <peff@peff.net> 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.

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

Junio C Hamano· Mar 17, 2021, 18:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 17 quoted lines

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

"LANG=C LC_ALL=C sort ..."

I like this 100% better than the original ;-)

Ævar Arnfjörð Bjarmason· Mar 17, 2021, 18:16 UTC · re: Junio C Hamano · lore

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

On Wed, Mar 17 2021, Junio C Hamano wrote:

Show 23 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> +	./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
>>     +'
>
> "LANG=C LC_ALL=C sort ..."
>
> I like this 100% better than the original ;-)

We don't need to use "LANG=C LC_ALL=C sort", the test-lib.sh sets that already, so just "sort" works consistently.

It's also why with GETTEXT_POISON gone we can just "grep" output, instead of worrying that it may be in the user's locale.

Junio C Hamano· Mar 17, 2021, 17:59 UTC · re: Denton Liu · lore

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

Denton Liu <liu.denton@gmail.com> writes:

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

These two are trivial to see.

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

This is too brittle to be acceptable. It FORBIDS us from introducing initialization for another table to the file.

I won't participate in the bikeshedding of how the Perl script would be best written ;-)

Denton Liu· Mar 16, 2021, 00:56 UTC · re: Denton Liu · lore

[PATCH 6/7] ci/run-static-analysis.sh: make check-builtins

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 <liu.denton@gmail.com>
---
 ci/run-static-analysis.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to ci/run-static-analysis.sh +1 −1

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

Denton Liu· Mar 16, 2021, 00:56 UTC · re: Denton Liu · lore

[PATCH 4/7] test-tool.h: ASCII-sort list of functions

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

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

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

Show changes to t/helper/test-tool.h +3 −3

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

Junio C Hamano· Mar 17, 2021, 17:54 UTC · re: Denton Liu · lore

Re: [PATCH 4/7] test-tool.h: ASCII-sort list of functions

Denton Liu <liu.denton@gmail.com> writes:

Show 8 quoted lines

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

Likewise. The rationale 805d9eaf (Makefile: ASCII-sort += lists, 2020-03-21) applies better for Makefile's "X += <name>" as the prefix before the <name> are all common, while in the header files, you have to depend on their return type being the same and '(' sorting before '_'.

Now I am inclined to say that it may be worth mentioning in the log, both for this step and the previous one.

Show 46 quoted lines

> This patch is best viewed with `--color-moved`.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  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

Denton Liu· Mar 16, 2021, 00:56 UTC · re: Denton Liu · lore

[PATCH 7/7] ci/run-static-analysis.sh: make check-sort

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 <liu.denton@gmail.com>
---
 ci/run-static-analysis.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to ci/run-static-analysis.sh +1 −1

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

Bagas Sanjaya· Mar 17, 2021, 11:01 UTC · re: Denton Liu · lore

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

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.

Show 17 quoted lines

> 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

Junio C Hamano· Mar 17, 2021, 18:05 UTC · re: Denton Liu · lore

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

Denton Liu <liu.denton@gmail.com> writes:

Show 13 quoted lines

> 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

I'd expect that the series would be structured better if we

 - move #6 (static-analysis runs check-builtins) earlier, perhaps to
   the front of the series, as there is nothing to fix to make the
   check pass.

 - merge #7 into #5.

I've also left comments on a few individual steps.

Thanks for working on it.

Show 7 quoted lines

>  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