diff mbox series

Makefile: ASCII-sort += lists

Message ID f0f1ef1f677133eabd1bce00c6cdbbcc6477f00b.1602142738.git.liu.denton@gmail.com (mailing list archive)
State Accepted
Commit 8474f2658156157476e84e8b0ad2b8dcd6354f39
Headers show
Series Makefile: ASCII-sort += lists | expand

Commit Message

Denton Liu Oct. 8, 2020, 7:39 a.m. UTC 
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(-)

Comments

Johannes Schindelin Oct. 8, 2020, 10:06 a.m. UTC | #1 
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 --
@@ -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, 4:03 p.m. UTC | #2 
On Thu, Oct 08, 2020 at 12:39:26AM -0700, Denton Liu wrote:

>  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
Jeff King Oct. 8, 2020, 4:08 p.m. UTC | #3 
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.

-Peff
Junio C Hamano Oct. 8, 2020, 5:38 p.m. UTC | #4 
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.
Denton Liu Oct. 9, 2020, 1:45 a.m. UTC | #5 
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.

> 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:49 a.m. UTC | #6 
Hi Peff,

On Thu, 8 Oct 2020, Jeff King wrote:

> 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
Johannes Schindelin Oct. 9, 2020, 10:51 a.m. UTC | #7 
Hi Denton,

On Thu, 8 Oct 2020, Denton Liu wrote:

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

> > 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, 4:12 p.m. UTC | #8 
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > -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.
diff mbox series

Patch

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