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

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

[Denton Liu]

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

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

[Denton Liu]

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(+)

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

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

[Denton Liu]

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

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

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

[Denton Liu]

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

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

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

[Denton Liu]

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 <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
-- 
2.31.0.rc2.261.g7f71774620

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

[Denton Liu]

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

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

[Denton Liu]

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

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

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

[Denton Liu]

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

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

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

[Eric Sunshine]

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/

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

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

[Eric Sunshine]

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;

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.

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

[Denton Liu]

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

> 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

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

[Bagas Sanjaya]

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

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Jeff King]

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.

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

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

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 17 2021, Jeff King wrote:

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

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

[Junio C Hamano]

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.

> 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

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

[Junio C Hamano]

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

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

> 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

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

[Junio C Hamano]

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

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



> 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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

Æ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 ;-)

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

[Junio C Hamano]

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

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

>  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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Eric Sunshine]

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.

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

[Jeff King]

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

> 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