travis-ci: enable more warnings on travis linux-gcc job

We have DEVELOPER config to enable more warnings, but since we can't set a fixed gcc version to all devs, that has to be a bit more conservative. On travis, we know almost exact version to be used (bumped to 6.x for more warnings), we could be more aggressive.

---
 .travis.yml     |  3 +++
 ci/run-build.sh | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

index 4684b3f4f3..273b1d508a 100644
@@ -16,10 +16,13 @@ compiler:
 
 addons:
   apt:
+    sources:
+    - ubuntu-toolchain-r-test
     packages:
     - language-pack-is
     - git-svn
     - apache2
+    - gcc-6
 
 matrix:
   include:

index 4f940d1032..04e163359c 100755
@@ -5,4 +5,19 @@
 
 . ${0%/*}/lib-travisci.sh
 
+if [ "$jobname" = linux-gcc ]; then
+	gcc-6 --version
+	cat >config.mak <<-EOF
+	CC=gcc-6
+	CFLAGS = -g -O2 -Wall
+	CFLAGS += -Wextra
+	CFLAGS += -Wmissing-prototypes
+	CFLAGS += -Wno-empty-body
+	CFLAGS += -Wno-maybe-uninitialized
+	CFLAGS += -Wno-missing-field-initializers
+	CFLAGS += -Wno-sign-compare
+	CFLAGS += -Wno-unused-function
+	CFLAGS += -Wno-unused-parameter
+	EOF
+fi
 make --jobs=2

2.16.1.435.g8f24da2e1a

We have DEVELOPER config to enable more warnings, but since we can't set a fixed gcc version to all devs, that has to be a bit more conservative. On travis, we know almost exact version to be used (bumped to 6.x for more warnings), we could be more aggressive.

---
I still think it's a good thing to do. So here's the rebased version (v1 collides badly with another series, which has landed)

 .travis.yml               |  3 +++
 ci/run-build-and-tests.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

index 5f5ee4f3bd..0b3c50f5e7 100644
@@ -16,10 +16,13 @@ compiler:
 
 addons:
   apt:
+    sources:
+    - ubuntu-toolchain-r-test
     packages:
     - language-pack-is
     - git-svn
     - apache2
+    - gcc-6
 
 matrix:
   include:

index 3735ce413f..f6f346c468 100755
@@ -7,6 +7,22 @@
 
 ln -s "$cache_dir/.prove" t/.prove
 
+if [ "$jobname" = linux-gcc ]; then
+	gcc-6 --version
+	cat >config.mak <<-EOF
+	CC=gcc-6
+	CFLAGS = -g -O2 -Wall
+	CFLAGS += -Wextra
+	CFLAGS += -Wmissing-prototypes
+	CFLAGS += -Wno-empty-body
+	CFLAGS += -Wno-maybe-uninitialized
+	CFLAGS += -Wno-missing-field-initializers
+	CFLAGS += -Wno-sign-compare
+	CFLAGS += -Wno-unused-function
+	CFLAGS += -Wno-unused-parameter
+	EOF
+fi
+
 make --jobs=2
 make --quiet test
 if test "$jobname" = "linux-gcc"

2.16.2.903.gd04caf5039

Certainly it makes sense to dial up any static checks we can for the Travis environment, but...

DEVELOPER=1 is always set even before this patch. It's set and exported in lib-travisci.sh.

Duy

I interpreted Peff's comment like this:

If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, then we could set your additional flags in the makefile.

This way every developer with a new compiler would run these flags locally (if DEVELOPER=1 is set).

- Lars

Aha. Something like this? I split developer cflags out to a separate file because I imagine people may start to add gcc7, clang....

-- 8< --

index 5f5ee4f3bd..0b3c50f5e7 100644
@@ -16,10 +16,13 @@ compiler:
 
 addons:
   apt:
+    sources:
+    - ubuntu-toolchain-r-test
     packages:
     - language-pack-is
     - git-svn
     - apache2
+    - gcc-6
 
 matrix:
   include:

index de4b8f0c02..e8321e44d6 100644
@@ -436,15 +436,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-	-Wdeclaration-after-statement \
-	-Wno-format-zero-length \
-	-Wold-style-definition \
-	-Woverflow \
-	-Wpointer-arith \
-	-Wstrict-prototypes \
-	-Wunused \
-	-Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)

@@ -1045,7 +1036,8 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
+CFLAGS += $(DEV_CFLAGS)
 endif
 
 comma := ,

index e69de29bb2..644d0d581f 100644
@@ -0,0 +1,23 @@
+DEV_CFLAGS =
+DEV_CFLAGS += -Werror
+DEV_CFLAGS += -Wdeclaration-after-statement
+DEV_CFLAGS += -Wno-format-zero-length
+DEV_CFLAGS += -Wold-style-definition
+DEV_CFLAGS += -Woverflow
+DEV_CFLAGS += -Wpointer-arith
+DEV_CFLAGS += -Wstrict-prototypes
+DEV_CFLAGS += -Wunused
+DEV_CFLAGS += -Wvla
+
+GCC_VER := $(shell sh -c '$(CC) --version | grep ^gcc >/dev/null && $(CC) -dumpversion | cut -f 1 -d .')
+
+ifeq ($(GCC_VER),6)
+DEV_CFLAGS += -Wextra
+DEV_CFLAGS += -Wmissing-prototypes
+DEV_CFLAGS += -Wno-empty-body
+DEV_CFLAGS += -Wno-maybe-uninitialized
+DEV_CFLAGS += -Wno-missing-field-initializers
+DEV_CFLAGS += -Wno-sign-compare
+DEV_CFLAGS += -Wno-unused-function
+DEV_CFLAGS += -Wno-unused-parameter
+endif

-- 8< --

Yeah, gcc 7 is already standard on debian unstable, so...

index a1d8775adb..9dfd152a1e 100644
@@ -442,15 +442,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-	-Wdeclaration-after-statement \
-	-Wno-format-zero-length \
-	-Wold-style-definition \
-	-Woverflow \
-	-Wpointer-arith \
-	-Wstrict-prototypes \
-	-Wunused \
-	-Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)

@@ -1051,7 +1042,7 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
 endif
 
 comma := ,

new file mode 100644
index 0000000000..59aef342c4
@@ -0,0 +1,28 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+CFLAGS += -Wmissing-prototypes
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-maybe-uninitialized
+endif
+endif

new file mode 100755
index 0000000000..043367828c
@@ -0,0 +1,32 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+print_flags() {
+	family=$1
+	version=$($CC -dumpversion | cut -f 1 -d .)
+
+	# Print a feature flag not only for the current version, but also
+	# for any prior versions we encompass. This avoids needing to do
+	# numeric comparisons in make, which are awkward.
+	while test "$version" -gt 0
+	do
+		echo $family$version
+		version=$((version - 1))
+	done
+}
+
+case "$($CC --version 2>&1)" in
+gcc*)
+	print_flags gcc
+	;;
+clang*)
+	print_flags clang
+	;;
+*)
+	: unknown compiler family
+	;;
+esac

2.17.0.rc0.402.ged0b3fd1ee

I see this discussion is going in the right direction, and like the approach taken by this "how about this" patch.

Thanks.

The set of extra warnings we enable when DEVELOPER has to be conservative because we can't assume any compiler version the developer may use. Detect the compiler version so we know when it's safe to enable -Wextra and maybe more.

---
-dumpversion does not work correctly for clang. So I use "-v" instead which seems to produce the same output for both gcc and clang (with a minor difference in freebsd where it says "FreeBSD clang" instead of just "clang"). Not sure if it helps your "cc on debian" case though

Tested with clang-5.0.1 and gcc-6.4.0 (too lazy to test on freebsd clang 3.4.1 but I don't expect surprises there)

I will still need to pull more modern gcc/clang on travis, but that can wait until this patch settles.

 Makefile        | 11 +----------
 config.mak.dev  | 28 +++++++++++++++++++++++++++
 detect-compiler | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

index a1d8775adb..9dfd152a1e 100644
@@ -442,15 +442,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-	-Wdeclaration-after-statement \
-	-Wno-format-zero-length \
-	-Wold-style-definition \
-	-Woverflow \
-	-Wpointer-arith \
-	-Wstrict-prototypes \
-	-Wunused \
-	-Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)

@@ -1051,7 +1042,7 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
 endif
 
 comma := ,

new file mode 100644
index 0000000000..59aef342c4
@@ -0,0 +1,28 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+CFLAGS += -Wmissing-prototypes
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-maybe-uninitialized
+endif
+endif

2.17.0.rc0.347.gf9cf61673a

Does it still build ok for you with your changes squashed in? I think the check for clang4/gcc6 and setting -Wextra in config.mak.dev may backfire because clang9 probably has a lot more warnings enabled and some of them we don't want and cause compile error...

Duy

I don't think we can predict that going forward, though. We'd have to wait for people to use the new compiler and then send a patch disabling whatever causes a problem (or better yet, fixing the code if appropriate).

This framework can only help us write those patches; I don't think it can save us from having to do them in the first place.

-Peff

The project does still compile cleanly for me. (And I used "make V=1" to verify that the new flags are used.)

Is that really way ahead of the clang releases (which are at 7.0, AFAIK), or do they use a totally different number scheme?

If the latter, I guess we'd have to treat it separately from clang and have all of the conditionals treat it independently? Yuck.

-Peff

I have no idea, but from past experience with Apple, I'd guess that the version number is an Apple invention.

{...goes and researches a bit...}

Indeed, there doesn't seem to be any correlation between what Apple reports and the actual clang version number[1].

[1]: https://stackoverflow.com/questions/33603027/get-apple-clang-version-and-corresponding-upstream-llvm-version

Hrm, I was planning to expand my patch into a series. In particular, in my experiments some of those warnings do not need to be disabled under -Wextra (I tested going back to gcc 5 and clang 4.0).

I think it's worth splitting up the boilerplate changes from the decisions on individual warnings, because I suspect we'll need to revise the latter going forward.

I also noticed that all of those options seem to go back at least to gcc-5. It makes me wonder how much we even need this version-detecting framework. There are one or two "clang vs gcc" things I've found, but I haven't found a case where old versions of gcc don't support a particular option.

IOW, could we get away with just adding these to the DEVELOPER knob and assuming that people who use it have a reasonably modern gcc or clang setup? That's more or less what the existing knobs do.

But I didn't follow whether there was any earlier discussion on specific problems.

Heh. At first I was confused, as it seems to work for me:

  $ clang-4.0 -dumpversion
  4.2.1

But then I tried this:

  $ clang-5.0 -dumpversion
  4.2.1

Whoops. It returns the same value up through clang 7.

-Peff

I started this so I think people expect me to move it forward. But I'll gladly hand it to you. It looks like you have more compilers to play with anyway (I'm on gentoo and installing a new compiler, especially llvm-based, takes ages).

Duy

I've been doing a lot of compiling recently, using 10 'different' versions of clang and gcc ('different' if you count 64-bit and 32-bit compilers with the same version number as different!)

However, I can tell you that clang version 3.4 and 3.8.0 don't support -Og, but clang version 5.0.1 does.

ATB,
Ramsay Jones

Yeah. I checked clang git mirror and this -Og is in 4.x release branch (couldn't nail down exact release) so 5.x should be a safe bet.

Duy

I don't know, that feels kind of weird to me. I thought DEVELOPER was supposed to mean "ratchet up the linting, I want to know if I've broken something".

But tweaking -O is about "I am in an edit-compile-debug cycle". Sometimes you are and sometimes you aren't, but you'd presumably want to have extra warnings regardless (especially because some warnings only trigger under particular optimization settings).

Personally, I default to -O0, but then crank up to -O2 for performance testing or for my daily-driver builds. But I _always_ have as many warnings enabled as possible[1].

-Peff

[1] I do have some pretty horrific magic to turn on -Werror when I'm visiting a "historical" commit, such as during a bisect.

The set of extra warnings we enable when DEVELOPER has to be conservative because we can't assume any compiler version the developer may use. Detect the compiler version so we know when it's safe to enable -Wextra and maybe more.

These warning settings are mostly from my custom config.mak a long time ago when I tried to enable as many warnings as possible that can still build without showing warnings. Some of them those warnings are probably worth fixing instead of just suppressing in future.

---
v2 improves a bit over v1:

- apple clang support (though I suspect we may want to make it a separate compiler family since apple clang has different versioning, but I can't and won't work on that front)

- support CC='ccache gcc' (yes it breaks now if you have spaces in your cc path)

- allow to skip detect-compiler by setting COMPILER_FEATURES in config.mak

I notice Ramsay is working on clean -Wmaybe-uninitialized, if his series is merged first I'll stop disabling it here.

Interdiff

    diff --git a/Makefile b/Makefile
    index 9dfd152a1e..04b2a39bab 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -434,6 +434,10 @@ all::
     #
     # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
     # which the built Git will run (for instance "x86_64").
    +#
    +# Define DEVELOPER to enable more compiler warnings. Compiler version
    +# and faimily are auto detected, but could be overridden by defining
    +# COMPILER_FEATURES (see config.mak.dev)
     
     GIT-VERSION-FILE: FORCE
     	@$(SHELL_PATH) ./GIT-VERSION-GEN
    diff --git a/config.mak.dev b/config.mak.dev
    index 59aef342c4..d8beaf9347 100644
    --- a/config.mak.dev
    +++ b/config.mak.dev
    @@ -8,7 +8,9 @@ CFLAGS += -Wstrict-prototypes
     CFLAGS += -Wunused
     CFLAGS += -Wvla
     
    +ifndef COMPILER_FEATURES
     COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
    +endif
     
     ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
     CFLAGS += -Wtautological-constant-out-of-range-compare
    diff --git a/detect-compiler b/detect-compiler
    index bc2ea39ef5..70b754481c 100755
    --- a/detect-compiler
    +++ b/detect-compiler
    @@ -9,7 +9,7 @@ CC="$*"
     #
     # FreeBSD clang version 3.4.1 (tags/RELEASE...)
     get_version_line() {
    -	"$CC" -v 2>&1 | grep ' version '
    +	$CC -v 2>&1 | grep ' version '
     }
     
     get_family() {
    @@ -38,12 +38,15 @@ case "$(get_family)" in
     gcc)
     	print_flags gcc
     	;;
    -*clang)
    +clang)
     	print_flags clang
     	;;
     "FreeBSD clang")
     	print_flags clang
     	;;
    +"Apple LLVM")
    +	print_flags clang
    +	;;
     *)
     	: unknown compiler family
     	;;

 Makefile        | 15 +++++---------
 config.mak.dev  | 30 ++++++++++++++++++++++++++++
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

index a1d8775adb..04b2a39bab 100644
@@ -434,6 +434,10 @@ all::
 #
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
+#
+# Define DEVELOPER to enable more compiler warnings. Compiler version
+# and faimily are auto detected, but could be overridden by defining
+# COMPILER_FEATURES (see config.mak.dev)
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -442,15 +446,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-	-Wdeclaration-after-statement \
-	-Wno-format-zero-length \
-	-Wold-style-definition \
-	-Woverflow \
-	-Wpointer-arith \
-	-Wstrict-prototypes \
-	-Wunused \
-	-Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)

@@ -1051,7 +1046,7 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
 endif
 
 comma := ,

new file mode 100644
index 0000000000..d8beaf9347
@@ -0,0 +1,30 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+ifndef COMPILER_FEATURES
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+endif
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+CFLAGS += -Wmissing-prototypes
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-maybe-uninitialized
+endif
+endif

2.17.0.rc0.348.gd5a49e0b6f

s/them those/those/

This is a good idea in general, but we are not quite ready without some fixups.

Here is a quick summary (not exhaustive) from my trial merge to 'pu' (which will be reverted before today's integration is pushed out).

- json-writer.c triggers -Werror=old-style-decl

- t/helper/test-json-writer.c triggers Werror=missing-prototypes quite a few times.

- connect.c -Werror=implicit-fallthough around die_initial_contact().

I expected these (and it was a good reason to push this patch forward). I think people also reported style problems with this series.

This I did not expect. But I just looked again and I had this option explicitly turned off in config.mak! gcc-6.4 and gcc-4.9 do not complain about this. gcc-7.3 does. What's your compiler/version?

Duy

Duy Nguyen <[email protected]> writes:

gcc (Debian 7.3.0-5) 7.3.0

v3 fixes the fallthru warnings in connect.c, and with json-writer series rerolled, 'pu' should build cleanly now. -Wno-maybe-uninitialized is removed, thanks to Ramsay.

v3 also adds an experimental patch that adds EAGER_DEVELOPER=1. These developers will have all warnings enabled (nothing is suppressed) but they are not fatal. They could go through them and perhaps clean up the code base a bit more.

Interdiff

index e6680a8977..e4f04ce1cb 100644
@@ -434,7 +434,9 @@ all::
 #
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and faimily are auto detected, but could be overridden by defining
-# COMPILER_FEATURES (see config.mak.dev)
+# COMPILER_FEATURES (see config.mak.dev).
+# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning
+# class is suppressed anymore.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -1041,6 +1043,9 @@ include config.mak.uname
 -include config.mak.autogen
 -include config.mak
 
+ifdef EAGER_DEVELOPER
+DEVELOPER = Yes
+endif
 ifdef DEVELOPER
 include config.mak.dev
 endif

index d8beaf9347..13883410b3 100644
@@ -1,4 +1,6 @@
+ifndef EAGER_DEVELOPER
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition

@@ -18,13 +20,23 @@ endif
 
 ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
 CFLAGS += -Wextra
+# if a function is public, there should be a prototype and the right
+# header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifndef EAGER_DEVELOPER
+# These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
 CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
-ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
-CFLAGS += -Wno-maybe-uninitialized
+endif
+endif
+
+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
+# not worth fixing since newer compilers correctly stop complaining
+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-uninitialized
 endif
 endif

index c3a014c5ba..49eca46462 100644
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
 	if (unexpected)
 		die(_("The remote end hung up upon initial contact"));

Nguyễn Thái Ngọc Duy (3):
connect.c: mark die_initial_contact() NORETURN
Makefile: detect compiler and enable more warnings in DEVELOPER=1
Makefile: add EAGER_DEVELOPER mode

 Makefile        | 20 +++++++++----------
 config.mak.dev  | 42 +++++++++++++++++++++++++++++++++++++++
 connect.c       |  2 +-
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 11 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

2.17.0.rc1.439.gca064e2955

There is a series running in parallel with this one that adds code like this

    switch (...) {
    case ...:
        die_initial_contact();
    case ...:

There is nothing wrong with this. There is no actual falling through. But since gcc is not that smart and gcc 7.x introduces -Wimplicit-fallthrough, it raises a false alarm in this case.

This class of warnings may be useful elsewhere, so instead of suppressing the whole class, let's try to fix just this code. gcc is smart enough to realize that no execution can continue after a NORETURN function call and no longer raises the warning.

---
 connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c3a014c5ba..49eca46462 100644
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
 	if (unexpected)
 		die(_("The remote end hung up upon initial contact"));

2.17.0.rc1.439.gca064e2955

The set of extra warnings we enable when DEVELOPER has to be conservative because we can't assume any compiler version the developer may use. Detect the compiler version so we know when it's safe to enable -Wextra and maybe more.

These warning settings are mostly from my custom config.mak a long time ago when I tried to enable as many warnings as possible that can still build without showing warnings. Some of those warnings are probably worth fixing instead of just suppressing in future.

---
 Makefile        | 15 +++++---------
 config.mak.dev  | 38 +++++++++++++++++++++++++++++++++++
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

index 7f40f76739..e6680a8977 100644
@@ -431,6 +431,10 @@ all::
 #
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
+#
+# Define DEVELOPER to enable more compiler warnings. Compiler version
+# and faimily are auto detected, but could be overridden by defining
+# COMPILER_FEATURES (see config.mak.dev)
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -439,15 +443,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-	-Wdeclaration-after-statement \
-	-Wno-format-zero-length \
-	-Wold-style-definition \
-	-Woverflow \
-	-Wpointer-arith \
-	-Wstrict-prototypes \
-	-Wunused \
-	-Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)

@@ -1047,7 +1042,7 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
 endif
 
 comma := ,

new file mode 100644
index 0000000000..716a14ecc7
@@ -0,0 +1,38 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+ifndef COMPILER_FEATURES
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+endif
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+# if a function is public, there should be a prototype and the right
+# header file should be included. If not, it should be static.
+CFLAGS += -Wmissing-prototypes
+# These are disabled because we have these all over the place.
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+endif
+
+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
+# not worth fixing since newer compilers correctly stop complaining
+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-uninitialized
+endif
+endif

2.17.0.rc1.439.gca064e2955

This mode is for developers who want to keep the code base clean. There are warning classes that are currently suppressed because we have too many of them in the code base. This mode will stop the suppression, let the developer see and decide how to fix them.

---
 Makefile       | 7 ++++++-
 config.mak.dev | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

index e6680a8977..e4f04ce1cb 100644
@@ -434,7 +434,9 @@ all::
 #
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and faimily are auto detected, but could be overridden by defining
-# COMPILER_FEATURES (see config.mak.dev)
+# COMPILER_FEATURES (see config.mak.dev).
+# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning
+# class is suppressed anymore.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -1041,6 +1043,9 @@ include config.mak.uname
 -include config.mak.autogen
 -include config.mak
 
+ifdef EAGER_DEVELOPER
+DEVELOPER = Yes
+endif
 ifdef DEVELOPER
 include config.mak.dev
 endif

index 716a14ecc7..13883410b3 100644
@@ -1,4 +1,6 @@
+ifndef EAGER_DEVELOPER
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition

@@ -21,6 +23,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifndef EAGER_DEVELOPER
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers

@@ -28,6 +31,7 @@ CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
 endif
+endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining

2.17.0.rc1.439.gca064e2955

Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag which (approximately) enables -Wextra so that any combination of them and -Werror can be set.

I've long wanted to use DEVELOPER=1 in my production builds, but on some old systems I still get warnings, and thus the build would fail. However if the build/tests fail for some other reason, it would still be useful to scroll up and see what the relevant code is warning about.

This change allows for that. Now setting DEVELOPER will set -Werror as before, but if DEVELOPER_NONFATAL is set you'll get the same warnings, but without -Werror.

I've renamed the newly added EAGER_DEVELOPER flag to DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra, and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than inventing some new name of our own (VERY_EAGER_DEVELOPER?).

The DEVELOPER_EXTRA flag implicitly means DEVELOPER_NONFATAL, but just so that this change doesn't introduce yet another arbitrary limitation it's possible to combine it with DEVELOPER_FATAL, which will turn on -Werror for it as well.

---

I really like where this is going, but as noted I think this on top would be even better.

 Makefile       | 17 +++++++++++------
 config.mak.dev | 10 ++++++++--
 2 files changed, 19 insertions(+), 8 deletions(-)

index e4f04ce1cb..641461a569 100644
@@ -432,11 +432,16 @@ all::
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
 #
-# Define DEVELOPER to enable more compiler warnings. Compiler version
-# and faimily are auto detected, but could be overridden by defining
-# COMPILER_FEATURES (see config.mak.dev).
-# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning
-# class is suppressed anymore.
+# Define DEVELOPER to enable more compiler warnings. We'll also enable
+# -Werror unless DEVELOPER_NONFATAL is defined. To enable even more
+# pedantic warnings that'll flag some potential existing issues in the
+# codebase turn on DEVELOPER_EXTRA, which implicitly sets DEVELOPER as
+# well, This is -Wextra with a whitelist of disabled warnings. Unless
+# DEVELOPER_NONFATAL is set DEVELOPER_EXTRA will turn it on
+# implicitly, so if you for some reason want both DEVELOPER and
+# DEVELOPER_EXTRA with fatal warnings, you need to set
+# DEVELOPER_FATAL=1 to force -Werror. See config.mak.dev for how this
+# all works.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -1043,7 +1048,7 @@ include config.mak.uname
 -include config.mak.autogen
 -include config.mak
 
-ifdef EAGER_DEVELOPER
+ifdef DEVELOPER_EXTRA
 DEVELOPER = Yes
 endif
 ifdef DEVELOPER

index 13883410b3..40f3365729 100644
@@ -1,6 +1,12 @@
-ifndef EAGER_DEVELOPER
+ifndef DEVELOPER_NONFATAL
+ifndef DEVELOPER_EXTRA
 CFLAGS += -Werror
 endif
+endif
+ifdef DEVELOPER_FATAL
+CFLAGS += -Werror
+endif
+
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition

@@ -23,7 +29,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
-ifndef EAGER_DEVELOPER
+ifndef DEVELOPER_EXTRA
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers

2.17.0.rc1.321.gba9d0f2565

Before we go with zillions of *DEVELOPER* maybe we can have something like DEVOPTS where you can give multiple keywords to a single variable to influence config.mak.dev. This is similar to COMPILER_FEATURES we already have in there, but now it's driven by the dev instead of the compiler. So you can have keywords like "gentle" (no -Werror) "extra" (-Wextra with no suppression) and something else.

Duy

We could do that, but I don't think it's that bad. This patch is one extra option on top of yours, and it's not going to result in some combinatorial explosion of options, i.e. if we add DEVELOPER_PEDANTIC we'll just add one extra flag.

But sure, we could make this some string we'd need to parse out similar to COMPILER_FEATURES, it just seems more complex to me for this task.

And that eager this was marked experimental because I was not sure if it was the right way :)

It's not that complex. With the EAGER_DEVELOPER patch removed, we can have something like this where eager devs just need to put

DEVOPTS = gentle no-suppression

and you put

DEVOPTS = gentle

(bad naming, I didn't spend time thinking about names)

-- 8< --

index e6680a8977..7b4e038e1d 100644
@@ -435,6 +435,11 @@ all::
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and faimily are auto detected, but could be overridden by defining
 # COMPILER_FEATURES (see config.mak.dev)
+#
+# When DEVELOPER is set, DEVOPTS can be used to control compiler options.
+# This variable contains keywords separated by whitespace. Two keywords
+# are recognized: "gentle" removes -Werror and "no-suppression"
+# removes all "-Wno-" options.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

index 716a14ecc7..1d7aba6a6a 100644
@@ -1,4 +1,6 @@
+ifeq ($(filter gentle,$(DEVOPTS)),)
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition

@@ -21,6 +23,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifeq ($(filter no-suppression,$(DEVOPTS)),)
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers

@@ -28,6 +31,7 @@ CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
 endif
+endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining

-- 8< --

It seems to me like we're losing the point of DEVELOPER here. I thought the idea was to have a turn-key flag you could set to get extra linting on your commits. But now we're tweaking all kinds of individual options. At some point are we better off just letting you put "-Wno-foo" in your CFLAGS yourself?

I don't mind the version-based checks because they're automatic, so the feature remains turn-key. But this kind of DEVOPTS seems like a step in the wrong direction.

-Peff

It's because what AEvar wanted is no longer a dev thing :)

Duy

It's still a dev thing, but just for a slightly different use-case. I hack my patches up with DEVELOPER=1, and then eventually deploy a patched git version with those (and others) on top of the latest release tag.

At that point I want the same compiler assertions, but might be building on much older compilers, so I just want the warning output without -Werror.

Anyway, I see you've pushed a new version with DEVOPTS. I'll submit mine on top of that once your new version lands (unless you want to try to integrate it yourself).

Actually I think I'll just drop both EAGER_DEVELOPER and DEVOTPS.

Duy

OK. I'll submit something on top to optionally get rid of -Werror, since that's the only bit I actually care about & have a use-case for.

This is a v4 and replacement of gitster/nd/warn-more-for-devs. I'm sending this with Duy's blessing.

The first two patches are the same, except for one trivial s/faimily/family/ typo fix.

The third patch in gitster/nd/warn-more-for-devs ("Makefile: add EAGER_DEVELOPER mode") is gone, instead there's now a DEVOPTS option. The 3/4 and 4/4 add a way to turn off -Werror & the -Wextra suppression, respectively.

Duy was right in [1] that this is a much better and extensible way of doing this than my "Makefile: untangle DEVELOPER and -Werror" patch.

Most of 3/4 & 4/4 are just tweaked from [email protected]:pclouds/git.git pclouds/more-warnings and combined with my previous 4/3 patch[2].

I changed the "no-suppression" name in Duy's WIP patch to "extra-all", and "gentle" to "no-error". I think those are cleare,r and leave things more open to future expansion, e.g. if we'd like pedantic-all.

1. https://public-inbox.org/git/CACsJy8CyB0igY365NMkswSgAi9_rf+XBOMQyJ7XW6iQxQiCEyQ@mail.gmail.com/
2. https://public-inbox.org/git/[email protected]/

Nguyễn Thái Ngọc Duy (2):
connect.c: mark die_initial_contact() NORETURN
Makefile: detect compiler and enable more warnings in DEVELOPER=1

Ævar Arnfjörð Bjarmason (2):
Makefile: add a DEVOPTS to suppress -Werror under DEVELOPER
Makefile: add a DEVOPTS to get all of -Wextra

 Makefile        | 31 +++++++++++++++++++----------
 config.mak.dev  | 42 +++++++++++++++++++++++++++++++++++++++
 connect.c       |  2 +-
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 11 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

2.17.0.290.gded63e768a

From: Nguyễn Thái Ngọc Duy <[email protected]>

There is a series running in parallel with this one that adds code like this

    switch (...) {
    case ...:
        die_initial_contact();
    case ...:

There is nothing wrong with this. There is no actual falling through. But since gcc is not that smart and gcc 7.x introduces -Wimplicit-fallthrough, it raises a false alarm in this case.

This class of warnings may be useful elsewhere, so instead of suppressing the whole class, let's try to fix just this code. gcc is smart enough to realize that no execution can continue after a NORETURN function call and no longer raises the warning.

---
 connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c3a014c5ba..49eca46462 100644
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
 	if (unexpected)
 		die(_("The remote end hung up upon initial contact"));

2.17.0.290.gded63e768a

From: Nguyễn Thái Ngọc Duy <[email protected]>

The set of extra warnings we enable when DEVELOPER has to be conservative because we can't assume any compiler version the developer may use. Detect the compiler version so we know when it's safe to enable -Wextra and maybe more.

These warning settings are mostly from my custom config.mak a long time ago when I tried to enable as many warnings as possible that can still build without showing warnings. Some of those warnings are probably worth fixing instead of just suppressing in future.

---
 Makefile        | 15 +++++---------
 config.mak.dev  | 38 +++++++++++++++++++++++++++++++++++
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

index f181687250..3038c78325 100644
@@ -441,6 +441,10 @@ all::
 #
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
+#
+# Define DEVELOPER to enable more compiler warnings. Compiler version
+# and family are auto detected, but could be overridden by defining
+# COMPILER_FEATURES (see config.mak.dev)
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -449,15 +453,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-	-Wdeclaration-after-statement \
-	-Wno-format-zero-length \
-	-Wold-style-definition \
-	-Woverflow \
-	-Wpointer-arith \
-	-Wstrict-prototypes \
-	-Wunused \
-	-Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)

@@ -1062,7 +1057,7 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
 endif
 
 comma := ,

new file mode 100644
index 0000000000..716a14ecc7
@@ -0,0 +1,38 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+ifndef COMPILER_FEATURES
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+endif
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+# if a function is public, there should be a prototype and the right
+# header file should be included. If not, it should be static.
+CFLAGS += -Wmissing-prototypes
+# These are disabled because we have these all over the place.
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+endif
+
+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
+# not worth fixing since newer compilers correctly stop complaining
+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-uninitialized
+endif
+endif

2.17.0.290.gded63e768a

Add a DEVOPTS variable that'll be used to tweak the behavior of DEVELOPER.

I've long wanted to use DEVELOPER=1 in my production builds, but on some old systems I still get warnings, and thus the build would fail. However if the build/tests fail for some other reason, it would still be useful to scroll up and see what the relevant code is warning about.

This change allows for that. Now setting DEVELOPER will set -Werror as before, but if DEVOPTS=no-error is provided is set you'll get the same warnings, but without -Werror.

---
 Makefile       | 10 ++++++++++
 config.mak.dev |  2 ++
 2 files changed, 12 insertions(+)

index 3038c78325..9a57495fae 100644
@@ -445,6 +445,16 @@ all::
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and family are auto detected, but could be overridden by defining
 # COMPILER_FEATURES (see config.mak.dev)
+#
+# When DEVELOPER is set, DEVOPTS can be used to control compiler
+# options.  This variable contains keywords separated by
+# whitespace. The following keywords are are recognized:
+#
+#    no-error:
+#
+#        suppresses the -Werror that implicitly comes with
+#        DEVELOPER=1. Useful for getting the full set of errors
+#        without immediately dying, or for logging them.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

index 716a14ecc7..7a54426d78 100644
@@ -1,4 +1,6 @@
+ifeq ($(filter no-error,$(DEVOPTS)),)
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition

2.17.0.290.gded63e768a

Change DEVOPTS to understand a "extra-all" option. When the DEVELOPER flag is enabled we turn on -Wextra, but manually switch some of the warnings it turns on off.

This is because we have many existing occurrences of them in the code base. This mode will stop the suppression, let the developer see and decide whether to fix them.

This change is a slight alteration of Nguyễn Thái Ngọc Duy EAGER_DEVELOPER mode patch[1]

1. "[PATCH v3 3/3] Makefile: add EAGER_DEVELOPER

    mode" (<[email protected]>;
    https://public-inbox.org/git/[email protected]/)

---
 Makefile       | 6 ++++++
 config.mak.dev | 2 ++
 2 files changed, 8 insertions(+)

index 9a57495fae..47ddc85aae 100644
@@ -455,6 +455,12 @@ all::
 #        suppresses the -Werror that implicitly comes with
 #        DEVELOPER=1. Useful for getting the full set of errors
 #        without immediately dying, or for logging them.
+#
+#    extra-all:
+#
+#        The DEVELOPER mode enables -Wextra with a few exceptions. By
+#        setting this flag the exceptions are removed, and all of
+#        -Wextra is used.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

index 7a54426d78..2d244ca470 100644
@@ -23,6 +23,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifeq ($(filter extra-all,$(DEVOPTS)),)
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers

@@ -30,6 +31,7 @@ CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
 endif
+endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining

2.17.0.290.gded63e768a

Looks sensible, mostly. Will queue. Thanks.

Actually, I was mostly just confused, and didn't realize that these were going above and beyond what's in DEVELOPER.

But that said, now that I understand, I agree completely with your suggestion. :)

-Peff