threads / patch / 54370

patchMakefile: ASCII-sort += lists

Subject: [PATCH] Makefile: ASCII-sort += lists

## tl;dr

9 messages between Oct 8, 2020 and Oct 9, 2020. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Denton Liu· Oct 8, 2020, 07:39 UTC · lore

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. Resort these lists back into ASCII order.

This patch is best viewed with `--color-moved`.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Makefile | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to Makefile +5 −5
diff --git a/Makefile b/Makefile
index 5311b1d2c4..95571ee3fc 100644
--- a/Makefile
+++ b/Makefile
@@ -820,8 +820,8 @@ TEST_SHELL_PATH = $(SHELL_PATH)
 LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 
-GENERATED_H += config-list.h
 GENERATED_H += command-list.h
+GENERATED_H += config-list.h
 
 LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
 	$(FIND) . \
@@ -998,9 +998,9 @@ LIB_OBJS += sigchain.o
 LIB_OBJS += split-index.o
 LIB_OBJS += stable-qsort.o
 LIB_OBJS += strbuf.o
-LIB_OBJS += strvec.o
 LIB_OBJS += streaming.o
 LIB_OBJS += string-list.o
+LIB_OBJS += strvec.o
 LIB_OBJS += sub-process.o
 LIB_OBJS += submodule-config.o
 LIB_OBJS += submodule.o
@@ -1066,15 +1066,15 @@ BUILTIN_OBJS += builtin/checkout-index.o
 BUILTIN_OBJS += builtin/checkout.o
 BUILTIN_OBJS += builtin/clean.o
 BUILTIN_OBJS += builtin/clone.o
-BUILTIN_OBJS += builtin/credential-cache.o
-BUILTIN_OBJS += builtin/credential-cache--daemon.o
-BUILTIN_OBJS += builtin/credential-store.o
 BUILTIN_OBJS += builtin/column.o
 BUILTIN_OBJS += builtin/commit-graph.o
 BUILTIN_OBJS += builtin/commit-tree.o
 BUILTIN_OBJS += builtin/commit.o
 BUILTIN_OBJS += builtin/config.o
 BUILTIN_OBJS += builtin/count-objects.o
+BUILTIN_OBJS += builtin/credential-cache--daemon.o
+BUILTIN_OBJS += builtin/credential-cache.o
+BUILTIN_OBJS += builtin/credential-store.o
 BUILTIN_OBJS += builtin/credential.o
 BUILTIN_OBJS += builtin/describe.o
 BUILTIN_OBJS += builtin/diff-files.o

-- 
2.29.0.rc0.261.g7178c9af9c
Johannes Schindelin· Oct 8, 2020, 10:06 UTC · re: Denton Liu · lore

Re: [PATCH] Makefile: ASCII-sort += lists

Hi Denton,
On Thu, 8 Oct 2020, Denton Liu wrote:
> 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. Resort these lists back into ASCII
> order.

Personally, I would write "Re-sort" or even "Sort again", so that readers such as myself do not stumble over the verb "resort" (as in "We resort to desperate measures").

Also, this strikes me as yet another task that is so automatable that we should really avoid bothering humans with it. I gave it a quick whirl, and this Perl script seems to do the job for me:

	$key = '';
	@to_sort = ();
	sub flush_sorted {
		if ($#to_sort >= 0) {
			print join('', sort @to_sort);
			@to_sort = ();
		}
	}
	while (<>) {
		if (/^(\S+) \+=/) {
			if ($key ne $1) {
				flush_sorted;
				$key = $1;
			}
			push @to_sort, $_;
		} else {
			flush_sorted;
			print $_;
		}
	}
	flush_sorted;

It is not the most elegant Perl script I ever wrote, but it does the job for me. And we could probably adapt and use it for other instances where we want to keep things sorted (think `commands[]` in `git.c` and the `cmd_*()` declarations in `builtin.h`, for example) and hook it up in `ci/run-static-analysis.sh` for added benefit.

My little script also finds this:
-- snip --
Show changes to diff +1 −2
@@ -1231,8 +1231,8 @@ space := $(empty) $(empty)

 ifdef SANITIZE
 SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))
-BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
 BASIC_CFLAGS += -fno-omit-frame-pointer
+BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
 ifneq ($(filter undefined,$(SANITIZERS)),)
 BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
 endif
-- snap --

I am not _so_ sure that we want to order `BASIC_CFLAGS`, but then, it does
not hurt, does it?

Ciao,
Dscho

>
> This patch is best viewed with `--color-moved`.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  Makefile | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 5311b1d2c4..95571ee3fc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -820,8 +820,8 @@ TEST_SHELL_PATH = $(SHELL_PATH)
>  LIB_FILE = libgit.a
>  XDIFF_LIB = xdiff/lib.a
>
> -GENERATED_H += config-list.h
>  GENERATED_H += command-list.h
> +GENERATED_H += config-list.h
>
>  LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
>  	$(FIND) . \
> @@ -998,9 +998,9 @@ LIB_OBJS += sigchain.o
>  LIB_OBJS += split-index.o
>  LIB_OBJS += stable-qsort.o
>  LIB_OBJS += strbuf.o
> -LIB_OBJS += strvec.o
>  LIB_OBJS += streaming.o
>  LIB_OBJS += string-list.o
> +LIB_OBJS += strvec.o
>  LIB_OBJS += sub-process.o
>  LIB_OBJS += submodule-config.o
>  LIB_OBJS += submodule.o
> @@ -1066,15 +1066,15 @@ BUILTIN_OBJS += builtin/checkout-index.o
>  BUILTIN_OBJS += builtin/checkout.o
>  BUILTIN_OBJS += builtin/clean.o
>  BUILTIN_OBJS += builtin/clone.o
> -BUILTIN_OBJS += builtin/credential-cache.o
> -BUILTIN_OBJS += builtin/credential-cache--daemon.o
> -BUILTIN_OBJS += builtin/credential-store.o
>  BUILTIN_OBJS += builtin/column.o
>  BUILTIN_OBJS += builtin/commit-graph.o
>  BUILTIN_OBJS += builtin/commit-tree.o
>  BUILTIN_OBJS += builtin/commit.o
>  BUILTIN_OBJS += builtin/config.o
>  BUILTIN_OBJS += builtin/count-objects.o
> +BUILTIN_OBJS += builtin/credential-cache--daemon.o
> +BUILTIN_OBJS += builtin/credential-cache.o
> +BUILTIN_OBJS += builtin/credential-store.o
>  BUILTIN_OBJS += builtin/credential.o
>  BUILTIN_OBJS += builtin/describe.o
>  BUILTIN_OBJS += builtin/diff-files.o
> --
> 2.29.0.rc0.261.g7178c9af9c
>
>
Jeff King· Oct 8, 2020, 16:08 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Makefile: ASCII-sort += lists

On Thu, Oct 08, 2020 at 12:06:47PM +0200, Johannes Schindelin wrote:
Show 17 quoted lines
> My little script also finds this:
> 
> -- snip --
> @@ -1231,8 +1231,8 @@ space := $(empty) $(empty)
> 
>  ifdef SANITIZE
>  SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))
> -BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>  BASIC_CFLAGS += -fno-omit-frame-pointer
> +BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>  ifneq ($(filter undefined,$(SANITIZERS)),)
>  BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
>  endif
> -- snap --
> 
> I am not _so_ sure that we want to order `BASIC_CFLAGS`, but then, it does
> not hurt, does it?

I agree it would not be wrong to reorder here from the compiler's perspective, but:

  - the current ordering is not arbitrary; the intent was to show that
    we are enabling -fsanitize, and then follow it up with any other
    related options (first any that apply to all sanitizers, of which
    there is only one, and then any sanitizer-specific ones). The patch
    above splits that logic apart.
  - I'd worry that there are cases in which order _does_ matter to the
    compiler. I'm not sure if anything that goes in CFLAGS might
    qualify, but certainly order can matter for other parts of the
    command-line (e.g., static library order).
    So it might be setting us up for confusion later.
-Peff
Johannes Schindelin· Oct 9, 2020, 10:49 UTC · re: Jeff King · lore

Re: [PATCH] Makefile: ASCII-sort += lists

Hi Peff,
On Thu, 8 Oct 2020, Jeff King wrote:
Show 35 quoted lines
> On Thu, Oct 08, 2020 at 12:06:47PM +0200, Johannes Schindelin wrote:
>
> > My little script also finds this:
> >
> > -- snip --
> > @@ -1231,8 +1231,8 @@ space := $(empty) $(empty)
> >
> >  ifdef SANITIZE
> >  SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))
> > -BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
> >  BASIC_CFLAGS += -fno-omit-frame-pointer
> > +BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
> >  ifneq ($(filter undefined,$(SANITIZERS)),)
> >  BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
> >  endif
> > -- snap --
> >
> > I am not _so_ sure that we want to order `BASIC_CFLAGS`, but then, it does
> > not hurt, does it?
>
> I agree it would not be wrong to reorder here from the compiler's
> perspective, but:
>
>   - the current ordering is not arbitrary; the intent was to show that
>     we are enabling -fsanitize, and then follow it up with any other
>     related options (first any that apply to all sanitizers, of which
>     there is only one, and then any sanitizer-specific ones). The patch
>     above splits that logic apart.
>
>   - I'd worry that there are cases in which order _does_ matter to the
>     compiler. I'm not sure if anything that goes in CFLAGS might
>     qualify, but certainly order can matter for other parts of the
>     command-line (e.g., static library order).
>
>     So it might be setting us up for confusion later.
Fair enough. It's easy to exclude `.*_CFLAGS` via a negative look-behind:
	$key = '';
	@to_sort = ();
	while (<>) {
		if ($#to_sort >= 0) {
			if (/^$key \+=/) {
				push @to_sort, $_;
				next;
			}
			print join('', sort @to_sort);
			@to_sort = ();
		}
		if (/^(\S+(?<!_CFLAGS)) \+=/) {
			$key = $1;
			push @to_sort, $_;
		} else {
			print $_;
		}
	}
	if ($#to_sort >= 0) {
		print join('', sort @to_sort);
	}

Ciao, Dscho

Denton Liu· Oct 9, 2020, 01:45 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Makefile: ASCII-sort += lists

Hi Dscho,
On Thu, Oct 08, 2020 at 12:06:47PM +0200, Johannes Schindelin wrote:
> Also, this strikes me as yet another task that is so automatable that we
> should really avoid bothering humans with it.

Yep, I found these changes via a similar-looking Python script. I like the Perl version, though, since it gives a path for inclusion so that we can automate this task.

Show 47 quoted lines
> I gave it a quick whirl, and
> this Perl script seems to do the job for me:
> 
> 	$key = '';
> 	@to_sort = ();
> 
> 	sub flush_sorted {
> 		if ($#to_sort >= 0) {
> 			print join('', sort @to_sort);
> 			@to_sort = ();
> 		}
> 	}
> 
> 	while (<>) {
> 		if (/^(\S+) \+=/) {
> 			if ($key ne $1) {
> 				flush_sorted;
> 				$key = $1;
> 			}
> 			push @to_sort, $_;
> 		} else {
> 			flush_sorted;
> 			print $_;
> 		}
> 	}
> 	flush_sorted;
> 
> It is not the most elegant Perl script I ever wrote, but it does the job
> for me. And we could probably adapt and use it for other instances where
> we want to keep things sorted (think `commands[]` in `git.c` and the
> `cmd_*()` declarations in `builtin.h`, for example) and hook it up in
> `ci/run-static-analysis.sh` for added benefit.
> 
> My little script also finds this:
> 
> -- snip --
> @@ -1231,8 +1231,8 @@ space := $(empty) $(empty)
> 
>  ifdef SANITIZE
>  SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))
> -BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>  BASIC_CFLAGS += -fno-omit-frame-pointer
> +BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>  ifneq ($(filter undefined,$(SANITIZERS)),)
>  BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
>  endif
> -- snap --

I opted to exclude this hunk because it didn't seem like a list that should be sorted. Perhaps if we include this in the static-analysis script, we could define a whitelist of lists that we want to keep sorted?

Thanks, Denton

Johannes Schindelin· Oct 9, 2020, 10:51 UTC · re: Denton Liu · lore

Re: [PATCH] Makefile: ASCII-sort += lists

Hi Denton,
On Thu, 8 Oct 2020, Denton Liu wrote:
Show 7 quoted lines
> On Thu, Oct 08, 2020 at 12:06:47PM +0200, Johannes Schindelin wrote:
> > Also, this strikes me as yet another task that is so automatable that we
> > should really avoid bothering humans with it.
>
> Yep, I found these changes via a similar-looking Python script. I like
> the Perl version, though, since it gives a path for inclusion so that we
> can automate this task.
Heh, when it comes to string processing, I always reach for Perl.
Show 52 quoted lines
> > I gave it a quick whirl, and
> > this Perl script seems to do the job for me:
> >
> > 	$key = '';
> > 	@to_sort = ();
> >
> > 	sub flush_sorted {
> > 		if ($#to_sort >= 0) {
> > 			print join('', sort @to_sort);
> > 			@to_sort = ();
> > 		}
> > 	}
> >
> > 	while (<>) {
> > 		if (/^(\S+) \+=/) {
> > 			if ($key ne $1) {
> > 				flush_sorted;
> > 				$key = $1;
> > 			}
> > 			push @to_sort, $_;
> > 		} else {
> > 			flush_sorted;
> > 			print $_;
> > 		}
> > 	}
> > 	flush_sorted;
> >
> > It is not the most elegant Perl script I ever wrote, but it does the job
> > for me. And we could probably adapt and use it for other instances where
> > we want to keep things sorted (think `commands[]` in `git.c` and the
> > `cmd_*()` declarations in `builtin.h`, for example) and hook it up in
> > `ci/run-static-analysis.sh` for added benefit.
> >
> > My little script also finds this:
> >
> > -- snip --
> > @@ -1231,8 +1231,8 @@ space := $(empty) $(empty)
> >
> >  ifdef SANITIZE
> >  SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))
> > -BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
> >  BASIC_CFLAGS += -fno-omit-frame-pointer
> > +BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
> >  ifneq ($(filter undefined,$(SANITIZERS)),)
> >  BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
> >  endif
> > -- snap --
>
> I opted to exclude this hunk because it didn't seem like a list that
> should be sorted. Perhaps if we include this in the static-analysis
> script, we could define a whitelist of lists that we want to keep
> sorted?
I agree, modulo s/whitelist/allow list/.

Thanks, Dscho

Junio C Hamano· Oct 9, 2020, 16:12 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Makefile: ASCII-sort += lists

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 14 quoted lines
>> > -BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>> >  BASIC_CFLAGS += -fno-omit-frame-pointer
>> > +BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>> >  ifneq ($(filter undefined,$(SANITIZERS)),)
>> >  BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
>> >  endif
>> > -- snap --
>>
>> I opted to exclude this hunk because it didn't seem like a list that
>> should be sorted. Perhaps if we include this in the static-analysis
>> script, we could define a whitelist of lists that we want to keep
>> sorted?
>
> I agree, modulo s/whitelist/allow list/.

If we were to do this, I agree that explicitly enumerating "lists whose elements must be sorted" would be a much better approach than declaring that our lists by default must be sorted and have a list of "lists whose elements are sorted in an order that has meaning, not just by codepoints".

But I somehow find the use of allow-list (as a concept [*1*]) awkward in this context. Technically, a list of things whose sortedness we care about may be "allowed to be automatically modified", and the remainder would be "forbidden from getting touched". But both are quite awkward way to think about them.

It would become even more awkward if the list is going to be used in a make target whose name has "check" in it, in which case the target would only point out problem so that the user can fix, and at that point, the said list would become a list of things that are "allowed to be checked".

[Footnote]
*1* ... not the phrase---I do not care in what color the allowed
    things are painted.
Jeff King· Oct 8, 2020, 16:03 UTC · re: Denton Liu · lore

Re: [PATCH] Makefile: ASCII-sort += lists

On Thu, Oct 08, 2020 at 12:39:26AM -0700, Denton Liu wrote:
Show 32 quoted lines
>  LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
>  	$(FIND) . \
> @@ -998,9 +998,9 @@ LIB_OBJS += sigchain.o
>  LIB_OBJS += split-index.o
>  LIB_OBJS += stable-qsort.o
>  LIB_OBJS += strbuf.o
> -LIB_OBJS += strvec.o
>  LIB_OBJS += streaming.o
>  LIB_OBJS += string-list.o
> +LIB_OBJS += strvec.o
>  LIB_OBJS += sub-process.o
>  LIB_OBJS += submodule-config.o
>  LIB_OBJS += submodule.o
> @@ -1066,15 +1066,15 @@ BUILTIN_OBJS += builtin/checkout-index.o
>  BUILTIN_OBJS += builtin/checkout.o
>  BUILTIN_OBJS += builtin/clean.o
>  BUILTIN_OBJS += builtin/clone.o
> -BUILTIN_OBJS += builtin/credential-cache.o
> -BUILTIN_OBJS += builtin/credential-cache--daemon.o
> -BUILTIN_OBJS += builtin/credential-store.o
>  BUILTIN_OBJS += builtin/column.o
>  BUILTIN_OBJS += builtin/commit-graph.o
>  BUILTIN_OBJS += builtin/commit-tree.o
>  BUILTIN_OBJS += builtin/commit.o
>  BUILTIN_OBJS += builtin/config.o
>  BUILTIN_OBJS += builtin/count-objects.o
> +BUILTIN_OBJS += builtin/credential-cache--daemon.o
> +BUILTIN_OBJS += builtin/credential-cache.o
> +BUILTIN_OBJS += builtin/credential-store.o
>  BUILTIN_OBJS += builtin/credential.o
>  BUILTIN_OBJS += builtin/describe.o
>  BUILTIN_OBJS += builtin/diff-files.o

Wow. Both of these hunks are from me, and I clearly _tried_ to put them in the right place. I think...maybe I'm just bad at alphabetizing?

Curiously, the only subtle part (ascii "-" versus ".") was wrong in the original spot I moved it from, so I won't blame myself for that. :)

Anyway, the whole patch looks good to me.
-Peff
Junio C Hamano· Oct 8, 2020, 17:38 UTC · re: Denton Liu · lore

Re: [PATCH] Makefile: ASCII-sort += lists

Denton Liu <liu.denton@gmail.com> writes:
> 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. Resort these lists back into ASCII
> order.

OK. I tend to agree with the comment on Resort (by Dscho?? Sorry I forgot), so I'll locally do s/Resort/Sort/ while applying.

By the way, I have a mixed feelings about a patch like this very close to prerelease freeze. It is a good timing because not much _ought_ to be changing, and a small mechanical code churn like this one whose correctness is so obvious is easy to accept without risk of breaking things. But at the same time, it is distracting when we would rather see contributors' and reviewers' time spent on double checking that the changes made during this cycle did not introduce regressions.

Will queue.  Thanks.

← back to recent threads