{"thread":{"id":"46796","subject":"[PATCH 0/4] appeasing -Wimplicit-fallthrough","startedAt":"2017-09-21T06:20:50Z","lastAt":"2017-09-22T03:47:42Z","messageCount":7,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"328547","messageId":"20170921062043.la2f3pjsnojirzyw@sigill.intra.peff.net","threadId":"46796","inReplyTo":null,"subject":"[PATCH 0/4] appeasing -Wimplicit-fallthrough","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T06:20:43Z","receivedAt":"2017-09-21T06:20:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed that gcc 7 ships with a new implicit-fallthrough warning that\ntries to find accidental switch-case fallthroughs. It's enabled with\n-Wextra, or you can turn it on separately. I don't think it finds any\nreal bugs in the current code, but it does seem like something that's\npotentially worth paying attention to.\n\nSo here's a series that squelches the false positives. The first patch\nis a real bug-fix that I found while poking around, and can be taken\nseparately.  The middle ones improve code readability, IMHO. And the\nfinal one just annotates the code to shut up the compiler warning.\n\n  [1/4]: cat-file: handle NULL object_context.path\n  [2/4]: test-line-buffer: simplify command parsing\n  [3/4]: curl_trace(): eliminate switch fallthrough\n  [4/4]: consistently use \"fallthrough\" comments in switches\n\n apply.c                     |  3 ++-\n builtin/cat-file.c          |  5 +++--\n builtin/checkout.c          |  1 +\n builtin/remote-ext.c        |  2 +-\n builtin/submodule--helper.c |  1 +\n config.c                    |  1 +\n convert.c                   |  3 ++-\n fsck.c                      |  1 +\n http-push.c                 |  1 +\n http.c                      |  7 ++++---\n mailinfo.c                  |  1 +\n quote.c                     |  1 +\n read-cache.c                |  1 +\n send-pack.c                 |  2 +-\n t/helper/test-line-buffer.c | 32 +++++++++++---------------------\n t/t8010-cat-file-filters.sh |  5 +++++\n 16 files changed, 37 insertions(+), 30 deletions(-)\n\n"},{"id":"328548","messageId":"20170921062140.cyfjv7rfuppqpblh@sigill.intra.peff.net","threadId":"46796","inReplyTo":"20170921062043.la2f3pjsnojirzyw@sigill.intra.peff.net","subject":"[PATCH 1/4] cat-file: handle NULL object_context.path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T06:21:40Z","receivedAt":"2017-09-21T06:21:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit dc944b65f1 (get_sha1_with_context: dynamically\nallocate oc->path, 2017-05-19) changed the rules that\ncallers must follow for seeing if we parsed a path in the\nobject name. The rules switched from \"check if the oc.path\nbuffer is empty\" to \"check if the oc.path pointer is NULL\".\nBut that commit forgot to update some sites in\ncat_one_file(), meaning we might dereference a NULL pointer.\n\nYou can see this by making a path-aware request like\n--textconv without specifying --path, and giving an object\nname that doesn't have a path in it. Like:\n\n  git cat-file --textconv HEAD\n\nwhich will reliably segfault.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c          | 4 ++--\n t/t8010-cat-file-filters.sh | 5 +++++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 4ccbfaac31..1ea25331d3 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -97,7 +97,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\treturn !has_object_file(&oid);\n \n \tcase 'w':\n-\t\tif (!path[0])\n+\t\tif (!path)\n \t\t\tdie(\"git cat-file --filters %s: <object> must be \"\n \t\t\t    \"<sha1:path>\", obj_name);\n \n@@ -107,7 +107,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\tbreak;\n \n \tcase 'c':\n-\t\tif (!path[0])\n+\t\tif (!path)\n \t\t\tdie(\"git cat-file --textconv %s: <object> must be <sha1:path>\",\n \t\t\t    obj_name);\n \ndiff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh\nindex d8242e467e..0f86c19174 100755\n--- a/t/t8010-cat-file-filters.sh\n+++ b/t/t8010-cat-file-filters.sh\n@@ -51,6 +51,11 @@ test_expect_success '--path=<path> complains without --textconv/--filters' '\n \tgrep \"path.*needs.*filters\" err\n '\n \n+test_expect_success '--textconv/--filters complain without path' '\n+\ttest_must_fail git cat-file --textconv HEAD &&\n+\ttest_must_fail git cat-file --filters HEAD\n+'\n+\n test_expect_success 'cat-file --textconv --batch works' '\n \tsha1=$(git rev-parse -q --verify HEAD:world.txt) &&\n \ttest_config diff.txt.textconv \"tr A-Za-z N-ZA-Mn-za-m <\" &&\n-- \n2.14.1.1051.g8c9143ec35\n\n"},{"id":"328549","messageId":"20170921062242.t63u2gvpysrrmijs@sigill.intra.peff.net","threadId":"46796","inReplyTo":"20170921062043.la2f3pjsnojirzyw@sigill.intra.peff.net","subject":"[PATCH 2/4] test-line-buffer: simplify command parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T06:22:43Z","receivedAt":"2017-09-21T06:22:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The handle_command() function matches an incoming command\nstring with a sequence of starts_with() checks. But it also\nsurrounds these with a switch on the first character of the\ncommand, which lets us jump to the right block of\nstarts_with() without going linearly through the list.\n\nHowever, each case arm of the switch falls through to the\none below it. This is pointless (we know that a command\nstarting with 'b' does not need to check any of the commands\nin the 'c' block), and it makes gcc's -Wimplicit-fallthrough\ncomplain.\n\nWe could solve this by adding a break at the end of each\nblock. However, this optimization isn't helping anything.\nEven if it does make matching faster (which is debatable),\nthis is code that is run only in the test suite, and each\nrun receives at most two of these \"commands\". We should\nfavor simplicity and readability over micro-optimizing.\n\nInstead, let's drop the switch statement completely and\nreplace it with an if/else cascade.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-line-buffer.c | 32 +++++++++++---------------------\n 1 file changed, 11 insertions(+), 21 deletions(-)\n\ndiff --git a/t/helper/test-line-buffer.c b/t/helper/test-line-buffer.c\nindex 81575fe2ab..078dd7e29d 100644\n--- a/t/helper/test-line-buffer.c\n+++ b/t/helper/test-line-buffer.c\n@@ -17,27 +17,17 @@ static uint32_t strtouint32(const char *s)\n \n static void handle_command(const char *command, const char *arg, struct line_buffer *buf)\n {\n-\tswitch (*command) {\n-\tcase 'b':\n-\t\tif (starts_with(command, \"binary \")) {\n-\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tstrbuf_addch(&sb, '>');\n-\t\t\tbuffer_read_binary(buf, &sb, strtouint32(arg));\n-\t\t\tfwrite(sb.buf, 1, sb.len, stdout);\n-\t\t\tstrbuf_release(&sb);\n-\t\t\treturn;\n-\t\t}\n-\tcase 'c':\n-\t\tif (starts_with(command, \"copy \")) {\n-\t\t\tbuffer_copy_bytes(buf, strtouint32(arg));\n-\t\t\treturn;\n-\t\t}\n-\tcase 's':\n-\t\tif (starts_with(command, \"skip \")) {\n-\t\t\tbuffer_skip_bytes(buf, strtouint32(arg));\n-\t\t\treturn;\n-\t\t}\n-\tdefault:\n+\tif (starts_with(command, \"binary \")) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_addch(&sb, '>');\n+\t\tbuffer_read_binary(buf, &sb, strtouint32(arg));\n+\t\tfwrite(sb.buf, 1, sb.len, stdout);\n+\t\tstrbuf_release(&sb);\n+\t} else if (starts_with(command, \"copy \")) {\n+\t\tbuffer_copy_bytes(buf, strtouint32(arg));\n+\t} else if (starts_with(command, \"skip \")) {\n+\t\tbuffer_skip_bytes(buf, strtouint32(arg));\n+\t} else {\n \t\tdie(\"unrecognized command: %s\", command);\n \t}\n }\n-- \n2.14.1.1051.g8c9143ec35\n\n"},{"id":"328550","messageId":"20170921062324.ehlfvyng7e62eoti@sigill.intra.peff.net","threadId":"46796","inReplyTo":"20170921062043.la2f3pjsnojirzyw@sigill.intra.peff.net","subject":"[PATCH 3/4] curl_trace(): eliminate switch fallthrough","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T06:23:24Z","receivedAt":"2017-09-21T06:23:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our trace handler is called by curl with a curl_infotype\nvariable to interpret its data field. For most types we\nprint the data and then break out of the switch. But for\nCURLINFO_TEXT, we print data and then fall through to the\n\"default\" case, which does the exact same thing (nothing!)\nthat breaking out of the switch would.\n\nThis is probably a leftover from an early iteration of the\npatch where the code after the switch _did_ do something\ninteresting that was unique to the non-text case arms.\nBut in its current form, this fallthrough is merely\nconfusing (and causes gcc's -Wimplicit-fallthrough to\ncomplain).\n\nLet's make CURLINFO_TEXT like the other case arms, and push\nthe default arm to the end where it's more obviously a\ncatch-all.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 9e40a465fd..713525f38e 100644\n--- a/http.c\n+++ b/http.c\n@@ -638,9 +638,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n \tswitch (type) {\n \tcase CURLINFO_TEXT:\n \t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n-\tdefault:\t\t/* we ignore unknown types by default */\n-\t\treturn 0;\n-\n+\t\tbreak;\n \tcase CURLINFO_HEADER_OUT:\n \t\ttext = \"=> Send header\";\n \t\tcurl_dump_header(text, (unsigned char *)data, size, DO_FILTER);\n@@ -665,6 +663,9 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,\n \t\ttext = \"<= Recv SSL data\";\n \t\tcurl_dump_data(text, (unsigned char *)data, size);\n \t\tbreak;\n+\n+\tdefault:\t\t/* we ignore unknown types by default */\n+\t\treturn 0;\n \t}\n \treturn 0;\n }\n-- \n2.14.1.1051.g8c9143ec35\n\n"},{"id":"328551","messageId":"20170921062541.ew67gyvrmb2ot4sf@sigill.intra.peff.net","threadId":"46796","inReplyTo":"20170921062043.la2f3pjsnojirzyw@sigill.intra.peff.net","subject":"[PATCH 4/4] consistently use \"fallthrough\" comments in switches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T06:25:41Z","receivedAt":"2017-09-21T06:25:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Gcc 7 adds -Wimplicit-fallthrough, which can warn when a\nswitch case falls through to the next case. The general idea\nis that the compiler can't tell if this was intentional or\nnot, so you should annotate any intentional fall-throughs as\nsuch, leaving it to complain about any unannotated ones.\n\nThere's a GNU __attribute__ which can be used for\nannotation, but of course we'd have to #ifdef it away on\nnon-gcc compilers. Gcc will also recognize\nspecially-formatted comments, which matches our current\npractice. Let's extend that practice to all of the\nunannotated sites (which I did look over and verify that\nthey were behaving as intended).\n\nIdeally in each case we'd actually give some reasons in the\ncomment about why we're falling through, or what we're\nfalling through to. And gcc does support that with\n-Wimplicit-fallthrough=2, which relaxes the comment pattern\nmatching to anything that contains \"fallthrough\" (or a\nvariety of spelling variants). However, this isn't the\ndefault for -Wimplicit-fallthrough, nor for -Wextra. In the\nname of simplicity, it's probably better for us to support\nthe default level, which requires \"fallthrough\" to be the\nonly thing in the comment (modulo some window dressing like\n\"else\" and some punctuation; see the gcc manual for the\ncomplete set of patterns).\n\nThis patch suppresses all warnings due to\n-Wimplicit-fallthrough. We might eventually want to add that\nto the DEVELOPER Makefile knob, but we should probably wait\nuntil gcc 7 is more widely adopted (since earlier versions\nwill complain about the unknown warning type).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe cases where I shrunk the comments make me a little sad. Maybe we\nshould just declare \"2\" as the right level. Or maybe we should do a\ntwo-comment system, like:\n\n  /* explain why we will fall through */\n  /* fallthrough */\n\nBut I had trouble finding any meaningful wording for the first comment\nin most of the cases.\n\n apply.c                     | 3 ++-\n builtin/cat-file.c          | 1 +\n builtin/checkout.c          | 1 +\n builtin/remote-ext.c        | 2 +-\n builtin/submodule--helper.c | 1 +\n config.c                    | 1 +\n convert.c                   | 3 ++-\n fsck.c                      | 1 +\n http-push.c                 | 1 +\n mailinfo.c                  | 1 +\n quote.c                     | 1 +\n read-cache.c                | 1 +\n send-pack.c                 | 2 +-\n 13 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 71cbbd141c..c022af53a2 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2920,6 +2920,7 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\tif (plen && (ws_rule & WS_BLANK_AT_EOF) &&\n \t\t\t    ws_blank_line(patch + 1, plen, ws_rule))\n \t\t\t\tis_blank_context = 1;\n+\t\t\t/* fallthrough */\n \t\tcase '-':\n \t\t\tmemcpy(old, patch + 1, plen);\n \t\t\tadd_line_info(&preimage, old, plen,\n@@ -2927,7 +2928,7 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\told += plen;\n \t\t\tif (first == '-')\n \t\t\t\tbreak;\n-\t\t/* Fall-through for ' ' */\n+\t\t\t/* fallthrough */\n \t\tcase '+':\n \t\t\t/* --no-add does not add new lines */\n \t\t\tif (first == '+' && state->no_add)\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 1ea25331d3..f5fa4fd75a 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -113,6 +113,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \n \t\tif (textconv_object(path, obj_context.mode, &oid, 1, &buf, &size))\n \t\t\tbreak;\n+\t\t/* else fallthrough */\n \n \tcase 'p':\n \t\ttype = sha1_object_info(oid.hash, NULL);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5c202b7af5..d091f06274 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -436,6 +436,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \t\t * update paths in the work tree, and we cannot revert\n \t\t * them.\n \t\t */\n+\t\t/* fallthrough */\n \tcase 0:\n \t\treturn 0;\n \tdefault:\ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex bfb21ba7d2..6a9127a33c 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -57,7 +57,7 @@ static char *strip_escapes(const char *str, const char *service,\n \t\t\t\tspecial = str[rpos];\n \t\t\t\tif (rpos == 1)\n \t\t\t\t\tbreak;\n-\t\t\t\t/* Fall-through to error. */\n+\t\t\t\t/* fallthrough */\n \t\t\tdefault:\n \t\t\t\tdie(\"Bad remote-ext placeholder '%%%c'.\",\n \t\t\t\t\tstr[rpos]);\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 818fe74f0a..f6a5e1af5d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1189,6 +1189,7 @@ static int push_check(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\tbreak;\n \t\t\t\t\tdie(\"HEAD does not match the named branch in the superproject\");\n \t\t\t\t}\n+\t\t\t\t/* fallthrough */\n \t\t\tdefault:\n \t\t\t\tdie(\"src refspec '%s' must name a ref\",\n \t\t\t\t    rs->src);\ndiff --git a/config.c b/config.c\nindex cd5a69e630..08490dfe81 100644\n--- a/config.c\n+++ b/config.c\n@@ -2353,6 +2353,7 @@ static int store_write_pair(int fd, const char *key, const char *value)\n \t\tcase '\"':\n \t\tcase '\\\\':\n \t\t\tstrbuf_addch(&sb, '\\\\');\n+\t\t\t/* fallthrough */\n \t\tdefault:\n \t\t\tstrbuf_addch(&sb, value[i]);\n \t\t\tbreak;\ndiff --git a/convert.c b/convert.c\nindex a09935cb81..20d7ab67bd 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1545,8 +1545,9 @@ static int ident_filter_fn(struct stream_filter *filter,\n \t\tswitch (ident->state) {\n \t\tdefault:\n \t\t\tstrbuf_add(&ident->left, head, ident->state);\n+\t\t\t/* fallthrough */\n \t\tcase IDENT_SKIPPING:\n-\t\t\t/* fallthru */\n+\t\t\t/* fallthrough */\n \t\tcase IDENT_DRAINING:\n \t\t\tident_drain(ident, &output, osize_p);\n \t\t}\ndiff --git a/fsck.c b/fsck.c\nindex 2d2d2e9432..2ad00fc4d0 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -588,6 +588,7 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \t\tcase S_IFREG | 0664:\n \t\t\tif (!options->strict)\n \t\t\t\tbreak;\n+\t\t\t/* fallthrough */\n \t\tdefault:\n \t\t\thas_bad_modes = 1;\n \t\t}\ndiff --git a/http-push.c b/http-push.c\nindex e4c9b065ce..d860c477c6 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1523,6 +1523,7 @@ static int remote_exists(const char *path)\n \t\tbreak;\n \tcase HTTP_ERROR:\n \t\terror(\"unable to access '%s': %s\", url, curl_errorstr);\n+\t\t/* fallthrough */\n \tdefault:\n \t\tret = -1;\n \t}\ndiff --git a/mailinfo.c b/mailinfo.c\nindex f2387a3267..bcdbf98de5 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -822,6 +822,7 @@ static void handle_filter(struct mailinfo *mi, struct strbuf *line)\n \t\tif (!handle_commit_msg(mi, line))\n \t\t\tbreak;\n \t\tmi->filter_stage++;\n+\t\t/* fallthrough */\n \tcase 1:\n \t\thandle_patch(mi, line);\n \t\tbreak;\ndiff --git a/quote.c b/quote.c\nindex 53b98a5b84..de2922ddd6 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -431,6 +431,7 @@ void tcl_quote_buf(struct strbuf *sb, const char *src)\n \t\tcase '{': case '}':\n \t\tcase '$': case '\\\\': case '\"':\n \t\t\tstrbuf_addch(sb, '\\\\');\n+\t\t\t/* fallthrough */\n \t\tdefault:\n \t\t\tstrbuf_addch(sb, c);\n \t\t\tbreak;\ndiff --git a/read-cache.c b/read-cache.c\nindex b211c57af6..a051567610 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -220,6 +220,7 @@ static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st)\n \tcase S_IFDIR:\n \t\tif (S_ISGITLINK(ce->ce_mode))\n \t\t\treturn ce_compare_gitlink(ce) ? DATA_CHANGED : 0;\n+\t\t/* else fallthrough */\n \tdefault:\n \t\treturn TYPE_CHANGED;\n \t}\ndiff --git a/send-pack.c b/send-pack.c\nindex b865f662e4..a8cc6b266e 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -497,7 +497,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\tstrbuf_release(&cap_buf);\n \t\t\t\treturn atomic_push_failure(args, remote_refs, ref);\n \t\t\t}\n-\t\t\t/* Fallthrough for non atomic case. */\n+\t\t\t/* else fallthrough */\n \t\tdefault:\n \t\t\tcontinue;\n \t\t}\n-- \n2.14.1.1051.g8c9143ec35\n"},{"id":"328602","messageId":"20170922005857.GK27425@aiede.mtv.corp.google.com","threadId":"46796","inReplyTo":"20170921062140.cyfjv7rfuppqpblh@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] cat-file: handle NULL object_context.path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-22T00:58:57Z","receivedAt":"2017-09-22T01:00:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Commit dc944b65f1 (get_sha1_with_context: dynamically\n> allocate oc->path, 2017-05-19) changed the rules that\n> callers must follow for seeing if we parsed a path in the\n> object name. The rules switched from \"check if the oc.path\n> buffer is empty\" to \"check if the oc.path pointer is NULL\".\n> But that commit forgot to update some sites in\n> cat_one_file(), meaning we might dereference a NULL pointer.\n>\n> You can see this by making a path-aware request like\n> --textconv without specifying --path, and giving an object\n> name that doesn't have a path in it. Like:\n>\n>   git cat-file --textconv HEAD\n>\n> which will reliably segfault.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/cat-file.c          | 4 ++--\n>  t/t8010-cat-file-filters.sh | 5 +++++\n>  2 files changed, 7 insertions(+), 2 deletions(-)\n\nYikes.  Commit dc944b65f1 even touched this function, so we reviewers\nhave no excuse for not having found it.\n\nTechnically this changes the behavior of cat-file --path='', but I\ndon't think that matters.\n\nDo other GET_SHA1_RECORD_PATH callers need similar treatment?\n\n* builtin/grep.c appears to do the right thing (it stores NULL in\n  list, so it passes NULL to grep_object, which calls grep_oid, which\n  calls grep_source_init, which stores NULL for the grep machinery\n  that is able to cope with a NULL).\n\n* builtin/log.c is correctly updated as part of the patch.\n\nThose are the only other callers.  So we're safe. *phew*\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"328612","messageId":"xmqqh8vvl7hk.fsf@gitster.mtv.corp.google.com","threadId":"46796","inReplyTo":"20170921062242.t63u2gvpysrrmijs@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] test-line-buffer: simplify command parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-22T03:47:35Z","receivedAt":"2017-09-22T03:47:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> However, each case arm of the switch falls through to the\n> one below it. This is pointless (we know that a command\n> starting with 'b' does not need to check any of the commands\n> in the 'c' block), and it makes gcc's -Wimplicit-fallthrough\n> complain.\n\nWow, this is an embarassment. The code tries to cleverly optimize\nit, and then fails miserably.\n\nThe updated one reads good.  Thanks.\n"}]}