[PATCH 0/4] appeasing -Wimplicit-fallthrough

[PATCH 0/4] appeasing -Wimplicit-fallthrough

[Jeff King]

I noticed that gcc 7 ships with a new implicit-fallthrough warning that
tries to find accidental switch-case fallthroughs. It's enabled with
-Wextra, or you can turn it on separately. I don't think it finds any
real bugs in the current code, but it does seem like something that's
potentially worth paying attention to.

So here's a series that squelches the false positives. The first patch
is a real bug-fix that I found while poking around, and can be taken
separately.  The middle ones improve code readability, IMHO. And the
final one just annotates the code to shut up the compiler warning.

  [1/4]: cat-file: handle NULL object_context.path
  [2/4]: test-line-buffer: simplify command parsing
  [3/4]: curl_trace(): eliminate switch fallthrough
  [4/4]: consistently use "fallthrough" comments in switches

 apply.c                     |  3 ++-
 builtin/cat-file.c          |  5 +++--
 builtin/checkout.c          |  1 +
 builtin/remote-ext.c        |  2 +-
 builtin/submodule--helper.c |  1 +
 config.c                    |  1 +
 convert.c                   |  3 ++-
 fsck.c                      |  1 +
 http-push.c                 |  1 +
 http.c                      |  7 ++++---
 mailinfo.c                  |  1 +
 quote.c                     |  1 +
 read-cache.c                |  1 +
 send-pack.c                 |  2 +-
 t/helper/test-line-buffer.c | 32 +++++++++++---------------------
 t/t8010-cat-file-filters.sh |  5 +++++
 16 files changed, 37 insertions(+), 30 deletions(-)

[PATCH 1/4] cat-file: handle NULL object_context.path

[Jeff King]

Commit dc944b65f1 (get_sha1_with_context: dynamically
allocate oc->path, 2017-05-19) changed the rules that
callers must follow for seeing if we parsed a path in the
object name. The rules switched from "check if the oc.path
buffer is empty" to "check if the oc.path pointer is NULL".
But that commit forgot to update some sites in
cat_one_file(), meaning we might dereference a NULL pointer.

You can see this by making a path-aware request like
--textconv without specifying --path, and giving an object
name that doesn't have a path in it. Like:

  git cat-file --textconv HEAD

which will reliably segfault.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c          | 4 ++--
 t/t8010-cat-file-filters.sh | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 4ccbfaac31..1ea25331d3 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -97,7 +97,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		return !has_object_file(&oid);
 
 	case 'w':
-		if (!path[0])
+		if (!path)
 			die("git cat-file --filters %s: <object> must be "
 			    "<sha1:path>", obj_name);
 
@@ -107,7 +107,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		break;
 
 	case 'c':
-		if (!path[0])
+		if (!path)
 			die("git cat-file --textconv %s: <object> must be <sha1:path>",
 			    obj_name);
 
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index d8242e467e..0f86c19174 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -51,6 +51,11 @@ test_expect_success '--path=<path> complains without --textconv/--filters' '
 	grep "path.*needs.*filters" err
 '
 
+test_expect_success '--textconv/--filters complain without path' '
+	test_must_fail git cat-file --textconv HEAD &&
+	test_must_fail git cat-file --filters HEAD
+'
+
 test_expect_success 'cat-file --textconv --batch works' '
 	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
 	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
-- 
2.14.1.1051.g8c9143ec35

[PATCH 2/4] test-line-buffer: simplify command parsing

[Jeff King]

The handle_command() function matches an incoming command
string with a sequence of starts_with() checks. But it also
surrounds these with a switch on the first character of the
command, which lets us jump to the right block of
starts_with() without going linearly through the list.

However, each case arm of the switch falls through to the
one below it. This is pointless (we know that a command
starting with 'b' does not need to check any of the commands
in the 'c' block), and it makes gcc's -Wimplicit-fallthrough
complain.

We could solve this by adding a break at the end of each
block. However, this optimization isn't helping anything.
Even if it does make matching faster (which is debatable),
this is code that is run only in the test suite, and each
run receives at most two of these "commands". We should
favor simplicity and readability over micro-optimizing.

Instead, let's drop the switch statement completely and
replace it with an if/else cascade.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-line-buffer.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/t/helper/test-line-buffer.c b/t/helper/test-line-buffer.c
index 81575fe2ab..078dd7e29d 100644
--- a/t/helper/test-line-buffer.c
+++ b/t/helper/test-line-buffer.c
@@ -17,27 +17,17 @@ static uint32_t strtouint32(const char *s)
 
 static void handle_command(const char *command, const char *arg, struct line_buffer *buf)
 {
-	switch (*command) {
-	case 'b':
-		if (starts_with(command, "binary ")) {
-			struct strbuf sb = STRBUF_INIT;
-			strbuf_addch(&sb, '>');
-			buffer_read_binary(buf, &sb, strtouint32(arg));
-			fwrite(sb.buf, 1, sb.len, stdout);
-			strbuf_release(&sb);
-			return;
-		}
-	case 'c':
-		if (starts_with(command, "copy ")) {
-			buffer_copy_bytes(buf, strtouint32(arg));
-			return;
-		}
-	case 's':
-		if (starts_with(command, "skip ")) {
-			buffer_skip_bytes(buf, strtouint32(arg));
-			return;
-		}
-	default:
+	if (starts_with(command, "binary ")) {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addch(&sb, '>');
+		buffer_read_binary(buf, &sb, strtouint32(arg));
+		fwrite(sb.buf, 1, sb.len, stdout);
+		strbuf_release(&sb);
+	} else if (starts_with(command, "copy ")) {
+		buffer_copy_bytes(buf, strtouint32(arg));
+	} else if (starts_with(command, "skip ")) {
+		buffer_skip_bytes(buf, strtouint32(arg));
+	} else {
 		die("unrecognized command: %s", command);
 	}
 }
-- 
2.14.1.1051.g8c9143ec35

[PATCH 3/4] curl_trace(): eliminate switch fallthrough

[Jeff King]

Our trace handler is called by curl with a curl_infotype
variable to interpret its data field. For most types we
print the data and then break out of the switch. But for
CURLINFO_TEXT, we print data and then fall through to the
"default" case, which does the exact same thing (nothing!)
that breaking out of the switch would.

This is probably a leftover from an early iteration of the
patch where the code after the switch _did_ do something
interesting that was unique to the non-text case arms.
But in its current form, this fallthrough is merely
confusing (and causes gcc's -Wimplicit-fallthrough to
complain).

Let's make CURLINFO_TEXT like the other case arms, and push
the default arm to the end where it's more obviously a
catch-all.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 9e40a465fd..713525f38e 100644
--- a/http.c
+++ b/http.c
@@ -638,9 +638,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
 	switch (type) {
 	case CURLINFO_TEXT:
 		trace_printf_key(&trace_curl, "== Info: %s", data);
-	default:		/* we ignore unknown types by default */
-		return 0;
-
+		break;
 	case CURLINFO_HEADER_OUT:
 		text = "=> Send header";
 		curl_dump_header(text, (unsigned char *)data, size, DO_FILTER);
@@ -665,6 +663,9 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
 		text = "<= Recv SSL data";
 		curl_dump_data(text, (unsigned char *)data, size);
 		break;
+
+	default:		/* we ignore unknown types by default */
+		return 0;
 	}
 	return 0;
 }
-- 
2.14.1.1051.g8c9143ec35

[PATCH 4/4] consistently use "fallthrough" comments in switches

[Jeff King]

Gcc 7 adds -Wimplicit-fallthrough, which can warn when a
switch case falls through to the next case. The general idea
is that the compiler can't tell if this was intentional or
not, so you should annotate any intentional fall-throughs as
such, leaving it to complain about any unannotated ones.

There's a GNU __attribute__ which can be used for
annotation, but of course we'd have to #ifdef it away on
non-gcc compilers. Gcc will also recognize
specially-formatted comments, which matches our current
practice. Let's extend that practice to all of the
unannotated sites (which I did look over and verify that
they were behaving as intended).

Ideally in each case we'd actually give some reasons in the
comment about why we're falling through, or what we're
falling through to. And gcc does support that with
-Wimplicit-fallthrough=2, which relaxes the comment pattern
matching to anything that contains "fallthrough" (or a
variety of spelling variants). However, this isn't the
default for -Wimplicit-fallthrough, nor for -Wextra. In the
name of simplicity, it's probably better for us to support
the default level, which requires "fallthrough" to be the
only thing in the comment (modulo some window dressing like
"else" and some punctuation; see the gcc manual for the
complete set of patterns).

This patch suppresses all warnings due to
-Wimplicit-fallthrough. We might eventually want to add that
to the DEVELOPER Makefile knob, but we should probably wait
until gcc 7 is more widely adopted (since earlier versions
will complain about the unknown warning type).

Signed-off-by: Jeff King <peff@peff.net>
---
The cases where I shrunk the comments make me a little sad. Maybe we
should just declare "2" as the right level. Or maybe we should do a
two-comment system, like:

  /* explain why we will fall through */
  /* fallthrough */

But I had trouble finding any meaningful wording for the first comment
in most of the cases.

 apply.c                     | 3 ++-
 builtin/cat-file.c          | 1 +
 builtin/checkout.c          | 1 +
 builtin/remote-ext.c        | 2 +-
 builtin/submodule--helper.c | 1 +
 config.c                    | 1 +
 convert.c                   | 3 ++-
 fsck.c                      | 1 +
 http-push.c                 | 1 +
 mailinfo.c                  | 1 +
 quote.c                     | 1 +
 read-cache.c                | 1 +
 send-pack.c                 | 2 +-
 13 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/apply.c b/apply.c
index 71cbbd141c..c022af53a2 100644
--- a/apply.c
+++ b/apply.c
@@ -2920,6 +2920,7 @@ static int apply_one_fragment(struct apply_state *state,
 			if (plen && (ws_rule & WS_BLANK_AT_EOF) &&
 			    ws_blank_line(patch + 1, plen, ws_rule))
 				is_blank_context = 1;
+			/* fallthrough */
 		case '-':
 			memcpy(old, patch + 1, plen);
 			add_line_info(&preimage, old, plen,
@@ -2927,7 +2928,7 @@ static int apply_one_fragment(struct apply_state *state,
 			old += plen;
 			if (first == '-')
 				break;
-		/* Fall-through for ' ' */
+			/* fallthrough */
 		case '+':
 			/* --no-add does not add new lines */
 			if (first == '+' && state->no_add)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1ea25331d3..f5fa4fd75a 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -113,6 +113,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 
 		if (textconv_object(path, obj_context.mode, &oid, 1, &buf, &size))
 			break;
+		/* else fallthrough */
 
 	case 'p':
 		type = sha1_object_info(oid.hash, NULL);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5c202b7af5..d091f06274 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -436,6 +436,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 		 * update paths in the work tree, and we cannot revert
 		 * them.
 		 */
+		/* fallthrough */
 	case 0:
 		return 0;
 	default:
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index bfb21ba7d2..6a9127a33c 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -57,7 +57,7 @@ static char *strip_escapes(const char *str, const char *service,
 				special = str[rpos];
 				if (rpos == 1)
 					break;
-				/* Fall-through to error. */
+				/* fallthrough */
 			default:
 				die("Bad remote-ext placeholder '%%%c'.",
 					str[rpos]);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 818fe74f0a..f6a5e1af5d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1189,6 +1189,7 @@ static int push_check(int argc, const char **argv, const char *prefix)
 						break;
 					die("HEAD does not match the named branch in the superproject");
 				}
+				/* fallthrough */
 			default:
 				die("src refspec '%s' must name a ref",
 				    rs->src);
diff --git a/config.c b/config.c
index cd5a69e630..08490dfe81 100644
--- a/config.c
+++ b/config.c
@@ -2353,6 +2353,7 @@ static int store_write_pair(int fd, const char *key, const char *value)
 		case '"':
 		case '\\':
 			strbuf_addch(&sb, '\\');
+			/* fallthrough */
 		default:
 			strbuf_addch(&sb, value[i]);
 			break;
diff --git a/convert.c b/convert.c
index a09935cb81..20d7ab67bd 100644
--- a/convert.c
+++ b/convert.c
@@ -1545,8 +1545,9 @@ static int ident_filter_fn(struct stream_filter *filter,
 		switch (ident->state) {
 		default:
 			strbuf_add(&ident->left, head, ident->state);
+			/* fallthrough */
 		case IDENT_SKIPPING:
-			/* fallthru */
+			/* fallthrough */
 		case IDENT_DRAINING:
 			ident_drain(ident, &output, osize_p);
 		}
diff --git a/fsck.c b/fsck.c
index 2d2d2e9432..2ad00fc4d0 100644
--- a/fsck.c
+++ b/fsck.c
@@ -588,6 +588,7 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
 		case S_IFREG | 0664:
 			if (!options->strict)
 				break;
+			/* fallthrough */
 		default:
 			has_bad_modes = 1;
 		}
diff --git a/http-push.c b/http-push.c
index e4c9b065ce..d860c477c6 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1523,6 +1523,7 @@ static int remote_exists(const char *path)
 		break;
 	case HTTP_ERROR:
 		error("unable to access '%s': %s", url, curl_errorstr);
+		/* fallthrough */
 	default:
 		ret = -1;
 	}
diff --git a/mailinfo.c b/mailinfo.c
index f2387a3267..bcdbf98de5 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -822,6 +822,7 @@ static void handle_filter(struct mailinfo *mi, struct strbuf *line)
 		if (!handle_commit_msg(mi, line))
 			break;
 		mi->filter_stage++;
+		/* fallthrough */
 	case 1:
 		handle_patch(mi, line);
 		break;
diff --git a/quote.c b/quote.c
index 53b98a5b84..de2922ddd6 100644
--- a/quote.c
+++ b/quote.c
@@ -431,6 +431,7 @@ void tcl_quote_buf(struct strbuf *sb, const char *src)
 		case '{': case '}':
 		case '$': case '\\': case '"':
 			strbuf_addch(sb, '\\');
+			/* fallthrough */
 		default:
 			strbuf_addch(sb, c);
 			break;
diff --git a/read-cache.c b/read-cache.c
index b211c57af6..a051567610 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -220,6 +220,7 @@ static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st)
 	case S_IFDIR:
 		if (S_ISGITLINK(ce->ce_mode))
 			return ce_compare_gitlink(ce) ? DATA_CHANGED : 0;
+		/* else fallthrough */
 	default:
 		return TYPE_CHANGED;
 	}
diff --git a/send-pack.c b/send-pack.c
index b865f662e4..a8cc6b266e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -497,7 +497,7 @@ int send_pack(struct send_pack_args *args,
 				strbuf_release(&cap_buf);
 				return atomic_push_failure(args, remote_refs, ref);
 			}
-			/* Fallthrough for non atomic case. */
+			/* else fallthrough */
 		default:
 			continue;
 		}
-- 
2.14.1.1051.g8c9143ec35

Re: [PATCH 1/4] cat-file: handle NULL object_context.path

[Jonathan Nieder]

Jeff King wrote:

> Commit dc944b65f1 (get_sha1_with_context: dynamically
> allocate oc->path, 2017-05-19) changed the rules that
> callers must follow for seeing if we parsed a path in the
> object name. The rules switched from "check if the oc.path
> buffer is empty" to "check if the oc.path pointer is NULL".
> But that commit forgot to update some sites in
> cat_one_file(), meaning we might dereference a NULL pointer.
>
> You can see this by making a path-aware request like
> --textconv without specifying --path, and giving an object
> name that doesn't have a path in it. Like:
>
>   git cat-file --textconv HEAD
>
> which will reliably segfault.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/cat-file.c          | 4 ++--
>  t/t8010-cat-file-filters.sh | 5 +++++
>  2 files changed, 7 insertions(+), 2 deletions(-)

Yikes.  Commit dc944b65f1 even touched this function, so we reviewers
have no excuse for not having found it.

Technically this changes the behavior of cat-file --path='', but I
don't think that matters.

Do other GET_SHA1_RECORD_PATH callers need similar treatment?

* builtin/grep.c appears to do the right thing (it stores NULL in
  list, so it passes NULL to grep_object, which calls grep_oid, which
  calls grep_source_init, which stores NULL for the grep machinery
  that is able to cope with a NULL).

* builtin/log.c is correctly updated as part of the patch.

Those are the only other callers.  So we're safe. *phew*

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 2/4] test-line-buffer: simplify command parsing

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> However, each case arm of the switch falls through to the
> one below it. This is pointless (we know that a command
> starting with 'b' does not need to check any of the commands
> in the 'c' block), and it makes gcc's -Wimplicit-fallthrough
> complain.

Wow, this is an embarassment. The code tries to cleverly optimize
it, and then fails miserably.

The updated one reads good.  Thanks.