{"thread":{"id":"58003","subject":"Git Test Coverage Report for v2.37.0-rc0","startedAt":"2022-06-14T20:19:40Z","lastAt":"2022-06-15T13:21:53Z","messageCount":6,"participants":["Derrick Stolee","Taylor Blau","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"457217","messageId":"00a57a1d-0566-8f54-26b2-0f3558bde88d@github.com","threadId":"58003","inReplyTo":null,"subject":"Git Test Coverage Report for v2.37.0-rc0","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T20:18:16Z","receivedAt":"2022-06-14T20:19:40Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Hello,\n\nIt's been a long time, but I decided it might be a good idea to revive my\ngit-test-coverage project and re-run the coverage checks for v2.37.0-rc0\nversus v2.36.1.\n\nFor those who haven't seen this before: I run 'make coverage-test' in the\nGit codebase and then compare the uncovered lines to the changes in each\nfile since some recent version. This gives us a view of the lines that were\nmodified but aren't covered by tests.\n\nKeep in mind that this doesn't mean we should always add tests, but that\nit is worth looking closely at the lines to be sure we did not add a\nregression during this release cycle.\n\nI'll reply to this email with my own assessment of these lines and whether\nor not they are critical to cover or not.\n\nThanks,\n-Stolee\n\n---\n\nUncovered code in 'v2.37.0-rc0' not in 'v2.36.1'\n--------------------------------------------------------\n\nCommits introducing uncovered code:\nÆvar Arnfjörð Bjarmason\t5e480176 stash: always have the owner of \"stash_info\" free it\nbuiltin/stash.c\n5e480176 200) break;\n5e480176 909) goto usage;\n5e480176 942) usage:\n5e480176 943) do_usage = 1;\n5e480176 944) goto cleanup;\n\nÆvar Arnfjörð Bjarmason\t86f4e312 connect.c: refactor sending of agent & object-format\nconnect.c\n86f4e312 490) reader->hash_algo = &hash_algos[GIT_HASH_SHA1];\n\nÆvar Arnfjörð Bjarmason\t2108fe4a revisions API users: add straightforward release_revisions()\nmerge-recursive.c\n2108fe4a 1163) release_revisions(&revs);\n\nÆvar Arnfjörð Bjarmason\t6d40f0ad cache-tree.c: use bug() and BUG_if_bug()\ncache-tree.c\n6d40f0ad 725) bug(\"there are unmerged index entries:\");\n6d40f0ad 729) bug(\"%d %.*s\", ce_stage(ce),\n\nÆvar Arnfjörð Bjarmason\t53ca5694 parse-options.c: use new bug() API for optbug()\nparse-options.c\n53ca5694 17) static void optbug(const struct option *opt, const char *reason)\n53ca5694 19) if (opt->long_name && opt->short_name)\n53ca5694 20) bug(\"switch '%c' (--%s) %s\", opt->short_name,\n53ca5694 22) else if (opt->long_name)\n53ca5694 23) bug(\"option '%s' %s\", opt->long_name, reason);\n53ca5694 25) bug(\"switch '%c' %s\", opt->short_name, reason);\n53ca5694 450) optbug(opts, \"uses incompatible flags \"\n53ca5694 454) optbug(opts, \"invalid short name\");\n53ca5694 456) optbug(opts, \"short name already used\");\n53ca5694 463) optbug(opts, \"uses feature \"\n53ca5694 473) optbug(opts, \"should not accept an argument\");\n53ca5694 497) optbug(opts, \"multi-word argh should use dash to separate words\");\n\nÆvar Arnfjörð Bjarmason\t5b2f5d92 parse-options.c: use optbug() instead of BUG() \"opts\" check\nparse-options.c\n5b2f5d92 477) optbug(opts, \"OPTION_CALLBACK needs one callback\");\n5b2f5d92 479) optbug(opts, \"OPTION_CALLBACK can't have two callbacks\");\n5b2f5d92 483) optbug(opts, \"OPTION_LOWLEVEL_CALLBACK needs a callback\");\n5b2f5d92 485) optbug(opts, \"OPTION_LOWLEVEL_CALLBACK needs no high level callback\");\n5b2f5d92 488) optbug(opts, \"OPT_ALIAS() should not remain at this point. \"\n5b2f5d92 491) break;\n\nÆvar Arnfjörð Bjarmason\t338959da remote.c: remove braces from one-statement \"for\"-loops\nremote.c\n338959da 149) for (i = 0; i < remote->url_nr; i++)\n338959da 153) for (i = 0; i < remote->pushurl_nr; i++)\n\nÆvar Arnfjörð Bjarmason\t323822c7 remote.c: don't dereference NULL in freeing loop\nremote.c\n323822c7 151) FREE_AND_NULL(remote->url);\n\nÆvar Arnfjörð Bjarmason\t29fda24d run-command API: rename \"env_array\" to \"env\"\nbuiltin/receive-pack.c\n29fda24d 783) strvec_pushf(&proc->env,\n29fda24d 825) strvec_pushf(&proc.env, \"GIT_PUSH_OPTION_COUNT\");\n\ndaemon.c\n29fda24d 497) strvec_pushv(&cld.env, env->v);\n29fda24d 907) strvec_pushf(&cld.env, \"REMOTE_ADDR=%s\", buf);\n29fda24d 908) strvec_pushf(&cld.env, \"REMOTE_PORT=%d\",\n29fda24d 915) strvec_pushf(&cld.env, \"REMOTE_ADDR=[%s]\", buf);\n29fda24d 916) strvec_pushf(&cld.env, \"REMOTE_PORT=%d\",\n\nhttp-backend.c\n29fda24d 479) strvec_pushf(&cld.env, \"GIT_COMMITTER_NAME=%s\", user);\n29fda24d 481) strvec_pushf(&cld.env,\n\npager.c\n29fda24d 102) setup_pager_env(&pager_process->env);\n29fda24d 132) strvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n\nÆvar Arnfjörð Bjarmason\t07b1d8f1 receive-pack: use bug() and BUG_if_bug()\nbuiltin/receive-pack.c\n07b1d8f1 1820) bug(\"connectivity check has not been run on ref %s\",\n\nÆvar Arnfjörð Bjarmason\tae1b383d revisions API: have release_revisions() release \"topo_walk_info\"\nrevision.c\nae1b383d 3494) static void reset_topo_walk(struct rev_info *revs)\nae1b383d 3496) release_revisions_topo_walk_info(revs->topo_walk_info);\nae1b383d 3497) revs->topo_walk_info = NULL;\n\nÆvar Arnfjörð Bjarmason\tfd3aaf53 run-command: add an \"ungroup\" option to run_process_parallel()\nrun-command.c\nfd3aaf53 1645) code = pp->start_failure(pp->ungroup ? NULL :\nfd3aaf53 1646)  &pp->children[i].err,\nfd3aaf53 1649) if (!pp->ungroup) {\nfd3aaf53 1650) strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\nfd3aaf53 1651) strbuf_reset(&pp->children[i].err);\n\nÆvar Arnfjörð Bjarmason\t1f6cf450 fetch-pack: move --keep=* option filling to a function\nfetch-pack.c\n1f6cf450 855) xsnprintf(hostname, sizeof(hostname), \"localhost\");\n\nÆvar Arnfjörð Bjarmason\t0139c58a revisions API users: add \"goto cleanup\" for release_revisions()\nbuiltin/diff-files.c\n0139c58a 80) result = -1;\n0139c58a 81) goto cleanup;\n\nbuiltin/submodule--helper.c\n0139c58a 1144) ret = -1;\n0139c58a 1145) goto cleanup;\n0139c58a 1149) ret = -1;\n0139c58a 1150) goto cleanup;\n\nsequencer.c\n0139c58a 5382) goto cleanup;\n0139c58a 5387) goto cleanup;\n\nt/helper/test-fast-rebase.c\n0139c58a 143) goto cleanup;\n0139c58a 150) goto cleanup;\n\nÆvar Arnfjörð Bjarmason\tf196c1e9 revisions API users: use release_revisions() needing REV_INFO_INIT\nbisect.c\nf196c1e9 1063) res = BISECT_FAILED;\nf196c1e9 1064) goto cleanup;\n\nÆvar Arnfjörð Bjarmason\t0cc05b04 usage.c: add a non-fatal bug() function to go with BUG()\nt/helper/test-trace2.c\n0cc05b04 214) return 0;\n\nÆvar Arnfjörð Bjarmason\tb78ce337 revisions API users: use release_revisions() in http-push.c\nhttp-push.c\nb78ce337 1944) release_revisions(&revs);\n\nChris Down\t0cf1defa bisect: output state before we are ready to compute bisection\nbuiltin/bisect--helper.c\n0cf1defa 427) return;\n\nChristian Couder\t511cfd3b http: add custom hostname to IP address resolutions\nhttp.c\n511cfd3b 398) if (!strcmp(\"http.curloptresolve\", var)) {\n511cfd3b 399) if (!value) {\n511cfd3b 400) return config_error_nonbool(var);\n511cfd3b 401) } else if (!*value) {\n511cfd3b 402) curl_slist_free_all(host_resolutions);\n511cfd3b 403) host_resolutions = NULL;\n511cfd3b 405) host_resolutions = curl_slist_append(host_resolutions, value);\n511cfd3b 407) return 0;\n511cfd3b 1148) curl_slist_free_all(host_resolutions);\n511cfd3b 1149) host_resolutions = NULL;\n\nDerrick Stolee\tb56166ca multi-pack-index: use --object-dir real path\nbuiltin/multi-pack-index.c\nb56166ca 61) opts.object_dir = xstrdup(get_object_directory());\nb56166ca 277) goto usage;\n\nDerrick Stolee\tac8acb4f sparse-index: complete partial expansion\nsparse-index.c\nac8acb4f 327) pl = NULL;\n\nDerrick Stolee\t080ab56a cache-tree: implement cache_tree_find_path()\ncache-tree.c\n080ab56a 104) struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path)\n080ab56a 108) struct cache_tree_sub it_sub = {\n080ab56a 111) struct cache_tree_sub *down = &it_sub;\n080ab56a 113) while (down) {\n080ab56a 114) slash = strchrnul(path, '/');\n080ab56a 115) namelen = slash - path;\n080ab56a 116) down->cache_tree->entry_count = -1;\n080ab56a 117) if (!*slash) {\n080ab56a 119) pos = cache_tree_subtree_pos(down->cache_tree, path, namelen);\n080ab56a 120) if (0 <= pos)\n080ab56a 121) return down->cache_tree->down[pos]->cache_tree;\n080ab56a 122) return NULL;\n080ab56a 124) down = find_subtree(it, path, namelen, 0);\n080ab56a 125) path = slash + 1;\n080ab56a 128) return NULL;\n\nDerrick Stolee\tc1d024b8 http: make http_get_file() external\nhttp.c\nc1d024b8 1992) int http_get_file(const char *url, const char *filename,\n\nDerrick Stolee\t834e3520 remote: allow relative_url() to return an absolute url\nremote.c\n834e3520 2819) return xstrdup(url);\n\nFrantisek Hrbata\t35919bf1 transport: unify return values and exit point from transport_push()\ntransport.c\n35919bf1 1294) goto done;\n35919bf1 1297) goto done;\n\nGarrit Franke\t6b52f48b cli: add -v and -h shorthands\ngit.c\n6b52f48b 900) argv[0] = \"help\";\n\nJeff Hostetler\t9915e08f t/helper/hexdump: add helper to print hexdump of stdin\nt/helper/test-hexdump.c\n9915e08f 7) int cmd__hexdump(int argc, const char **argv)\n9915e08f 11) int have_data = 0;\n9915e08f 14) len = xread(0, buf, sizeof(buf));\n9915e08f 15) if (len < 0)\n9915e08f 16) die_errno(\"failure reading stdin\");\n9915e08f 17) if (!len)\n9915e08f 18) break;\n9915e08f 20) have_data = 1;\n9915e08f 22) for (i = 0; i < len; i++)\n9915e08f 23) printf(\"%02x \", (unsigned char)buf[i]);\n9915e08f 26) if (have_data)\n9915e08f 27) putchar('\\n');\n9915e08f 29) return 0;\n\nJeff Hostetler\tf954c7b8 fsmonitor: never set CE_FSMONITOR_VALID on submodules\nfsmonitor.c\nf954c7b8 584) continue;\n\nJeff Hostetler\t95a4e78a fsmonitor: optimize processing of directory events\nfsmonitor.c\n95a4e78a 207) if (pos < 0)\n95a4e78a 208) pos = -pos - 1;\n95a4e78a 211) for (i = pos; i < istate->cache_nr; i++) {\n95a4e78a 212) if (!starts_with(istate->cache[i]->name, name))\n95a4e78a 213) break;\n95a4e78a 214) istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n95a4e78a 245) if ((unsigned char)istate->cache[i]->name[len] > '/')\n95a4e78a 246) break;\n95a4e78a 247) if (istate->cache[i]->name[len] == '/')\n95a4e78a 248) istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n\nJeff Hostetler\t1e7be10d fsmonitor-settings: remote repos on macOS are incompatible\nfsmonitor-settings.c\n1e7be10d 211) case FSMONITOR_REASON_ERROR:\n1e7be10d 212) strbuf_addf(&msg,\n1e7be10d 214)     r->worktree);\n1e7be10d 215) goto done;\n1e7be10d 217) case FSMONITOR_REASON_REMOTE:\n1e7be10d 218) strbuf_addf(&msg,\n1e7be10d 220)     r->worktree);\n1e7be10d 221) goto done;\n\nJeff Hostetler\t5c58fbd2 fsmonitor-settings: VFS for Git virtual repos are incompatible\nfsmonitor-settings.c\n5c58fbd2 223) case FSMONITOR_REASON_VFS4GIT:\n5c58fbd2 224) strbuf_addf(&msg,\n5c58fbd2 226)     r->worktree);\n5c58fbd2 227) goto done;\n\nJeff Hostetler\t62a62a28 fsmonitor-settings: bare repos are incompatible with FSMonitor\nfsmonitor-settings.c\n62a62a28 108) lookup_fsmonitor_settings(r);\n62a62a28 128) if (!r->settings.fsmonitor)\n62a62a28 129) r->settings.fsmonitor = alloc_settings();\n62a62a28 132) r->settings.fsmonitor->reason = reason;\n62a62a28 176) r = the_repository;\n62a62a28 188) r = the_repository;\n62a62a28 190) lookup_fsmonitor_settings(r);\n62a62a28 201) case FSMONITOR_REASON_UNTESTED:\n62a62a28 203) goto done;\n\nJeff Hostetler\tddc5dacf fsmonitor-settings: NTFS and FAT32 on MacOS are incompatible\nfsmonitor-settings.c\nddc5dacf 229) case FSMONITOR_REASON_NOSOCKETS:\nddc5dacf 230) strbuf_addf(&msg,\nddc5dacf 232)     r->worktree);\nddc5dacf 233) goto done;\n\nJohannes Schindelin\tde1f68a9 archive --add-virtual-file: allow paths containing colons\narchive.c\nde1f68a9 557) char *save = path;\nde1f68a9 558) path = prefix_filename(args->prefix, path);\nde1f68a9 559) free(save);\n\nJohannes Schindelin\t237a1d13 archive: optionally add \"virtual\" files\narchive.c\n237a1d13 344) err = error_errno(_(\"cannot read '%s'\"), path);\n\nJonathan Tan\t7709acf7 fetch-pack: make unexpected peek result non-fatal\nfetch-pack.c\n7709acf7 1396) if (reader->line)\n\nJosh Steadmon\tce3986bb run-command: don't spam trace2_child_exit()\nrun-command.c\nce3986bb 987) if (ret != -1)\nce3986bb 988) trace2_child_exit(cmd, ret);\n\nJunio C Hamano\tafe8a907 tree-wide: apply equals-null.cocci\nbuiltin/mailsplit.c\nafe8a907 123) if (!(dir = opendir(name))) {\n\nhttp-fetch.c\nafe8a907 58) if (!preq)\n\nhttp-push.c\nafe8a907 256) if (!obj_req) {\nafe8a907 321) if (!preq) {\nafe8a907 523) if (request->headers)\nafe8a907 786) if (!c)\nafe8a907 814) if (!c)\nafe8a907 1896) if (!ref_lock) {\n\nhttp-walker.c\nafe8a907 62) if (!req) {\nafe8a907 109) if (obj_req->repo->next) {\nafe8a907 228) if (slot->finished)\nafe8a907 233) if (slot->finished)\nafe8a907 446) if (!preq)\nafe8a907 492) if (!obj_req)\nafe8a907 496) if (obj_req->req)\n\nhttp.c\nafe8a907 874) if (ssl_cainfo)\nafe8a907 877) if (http_proxy_ssl_ca_info)\nafe8a907 1126) if (slot->curl) {\nafe8a907 1167) if (cert_auth.password) {\nafe8a907 1173) if (proxy_cert_auth.password) {\nafe8a907 2147) if (preq->packfile) {\nafe8a907 2438) if (!freq->slot)\nafe8a907 2495) if (freq->slot) {\n\nmerge-recursive.c\nafe8a907 2120) if (!collision_ent)\n\nKevin Locke\tc37c6dc6 setup: don't die if realpath(3) fails on getcwd(3)\nsetup.c\nc37c6dc6 463) trace2_data_string(\"setup\", the_repository,\nc37c6dc6 465) trace2_data_string(\"setup\", the_repository,\nc37c6dc6 467) free((char*)tmp_original_cwd);\nc37c6dc6 468) tmp_original_cwd = NULL;\nc37c6dc6 469) return;\n\nKleber Tarcísio\tc0befa0c commit-graph: close file before returning NULL\ncommit-graph.c\nc0befa0c 518) fclose(fp);\nc0befa0c 519) return NULL;\n\nNeeraj Singh\t23a3a303 update-index: use the bulk-checkin infrastructure\nbuiltin/update-index.c\n23a3a303 68) flush_odb_transaction();\n\nRené Scharfe\t2c2db194 tempfile: add mks_tempfile_dt()\ntempfile.c\n2c2db194 67) rmdir(tempfile->filename.buf);\n2c2db194 227) errno = EINVAL;\n2c2db194 228) return NULL;\n2c2db194 238) int orig_errno = errno;\n2c2db194 239) strbuf_release(&sb);\n2c2db194 240) errno = orig_errno;\n2c2db194 241) return NULL;\n2c2db194 247) int orig_errno = errno;\n2c2db194 248) strbuf_setlen(&sb, directorylen);\n2c2db194 249) rmdir(sb.buf);\n2c2db194 250) strbuf_release(&sb);\n2c2db194 251) errno = orig_errno;\n2c2db194 252) return NULL;\n\nTao Klerks\te6a65355 untracked-cache: support '--untracked-files=all' if configured\ndir.c\ne6a65355 2789) new_untracked_cache(istate, -1);\n\nTaylor Blau\t2bd44278 t/helper: add 'pack-mtimes' test-tool\nt/helper/test-pack-mtimes.c\n2bd44278 35) usage(pack_mtimes_usage);\n\nTaylor Blau\t5b92477f builtin/gc.c: conditionally avoid pruning objects via loose\nbuiltin/gc.c\n5b92477f 337) strvec_push(&repack, \"--cruft\");\n5b92477f 338) if (prune_expire)\n5b92477f 339) strvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n\nTaylor Blau\t4090511e builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects\nbuiltin/pack-objects.c\n4090511e 1368) return -1;\n\nTaylor Blau\t5045759d builtin/pack-objects.c: ensure included `--stdin-packs` exist\nbuiltin/pack-objects.c\n5045759d 3238)     oid_to_hex(oid), p->pack_name);\n\nTaylor Blau\ta7d49383 builtin/pack-objects.c: --cruft with expiration\nbuiltin/pack-objects.c\na7d49383 3540) progress_state = start_progress(_(\"Enumerating cruft objects\"), 0);\na7d49383 3559) progress_state = start_progress(_(\"Traversing cruft objects\"), 0);\n\nTaylor Blau\t4571324b builtin/repack.c: allow configuring cruft pack generation\nbuiltin/repack.c\n4571324b 86) return git_config_string(&cruft_po_args->window_memory, var, value);\n4571324b 88) return git_config_string(&cruft_po_args->depth, var, value);\n4571324b 90) return git_config_string(&cruft_po_args->threads, var, value);\n\nTaylor Blau\tf9825d1c builtin/repack.c: support generating a cruft pack\nbuiltin/repack.c\nf9825d1c 680) strvec_pushf(&cmd.args, \"--cruft-expiration=%s\",\nf9825d1c 691) return ret;\nf9825d1c 708) fprintf(in, \"%s.pack\\n\", item->string);\nf9825d1c 992) return ret;\n\nTaylor Blau\t2fb90409 reachable: add options to add_unseen_recent_objects_to_traversal\nreachable.c\n2fb90409 120) return 0;\n2fb90409 131) return 0;\n2fb90409 162) return 0;\n\nTaylor Blau\t5dfaf49a pack-mtimes: support writing pack .mtimes files\npack-objects.c\n5dfaf49a 175) REALLOC_ARRAY(pdata->cruft_mtime, pdata->nr_alloc);\n\npack-write.c\n5dfaf49a 330) unlink(mtimes_name);\n5dfaf49a 331) fd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n\nTaylor Blau\t94cd775a pack-mtimes: support reading .mtimes files\npack-mtimes.c\n94cd775a 35) ret = -1;\n94cd775a 36) goto cleanup;\n94cd775a 39) ret = error_errno(_(\"failed to read %s\"), mtimes_file);\n94cd775a 40) goto cleanup;\n94cd775a 47) goto cleanup;\n94cd775a 58) goto cleanup;\n94cd775a 64) goto cleanup;\n94cd775a 70) goto cleanup;\n94cd775a 80) goto cleanup;\n94cd775a 85) if (data)\n94cd775a 86) munmap(data, mtimes_size);\n94cd775a 102) return ret; /* not a cruft pack */\n94cd775a 108) goto cleanup;\n\npackfile.c\n94cd775a 342) munmap((void *)p->mtimes_map, p->mtimes_size);\n94cd775a 343) p->mtimes_map = NULL;\n\nTaylor Blau\t44f9fd64 pack-bitmap.c: check preferred pack validity when opening MIDX bitmap\npack-bitmap.c\n44f9fd64 362)     bitmap_git->midx->pack_names[i]);\n44f9fd64 368) preferred->pack_name);\n44f9fd64 369) goto cleanup;\n\nTaylor Blau\td9fef9d9 chunk-format.h: extract oid_version()\nchunk-format.c\nd9fef9d9 192) default:\n\nTaylor Blau\tb7573536 builtin/pack-objects.c: --cruft without expiration\nbuiltin/pack-objects.c\nb7573536 3514) progress_state = start_progress(_(\"Enumerating cruft objects\"), 0);\nb7573536 3577) continue;\nb7573536 3802) if (errno == ENOENT)\nb7573536 3803) return 0;\nb7573536 3804) return error_errno(\"unable to stat %s\", oid_to_hex(oid));\nb7573536 4132) cruft = 0;\nb7573536 4133) cruft_expiration = 0;\n\n\n\n"},{"id":"457218","messageId":"3d1c6dfd-1df6-3393-df5e-692719375772@github.com","threadId":"58003","inReplyTo":"00a57a1d-0566-8f54-26b2-0f3558bde88d@github.com","subject":"Re: Git Test Coverage Report for v2.37.0-rc0","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T20:20:05Z","receivedAt":"2022-06-14T20:20:17Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/14/2022 4:18 PM, Derrick Stolee wrote:\n> Ævar Arnfjörð Bjarmason\t338959da remote.c: remove braces from one-statement \"for\"-loops\n> remote.c\n> 338959da 149) for (i = 0; i < remote->url_nr; i++)\n> 338959da 153) for (i = 0; i < remote->pushurl_nr; i++)\n> Ævar Arnfjörð Bjarmason\t323822c7 remote.c: don't dereference NULL in freeing loop\n> remote.c\n> 323822c7 151) FREE_AND_NULL(remote->url);\n\nJust noting that these lines are inside remote_clear() which is called by\nremote_state_clear(), which is called by repo_clear(). Apparently we have\nno tests that clear a 'struct repository' that loaded remotes? This sounds\nlikely since we don't close these unless they are not the_repository.\n\n> Ævar Arnfjörð Bjarmason\tfd3aaf53 run-command: add an \"ungroup\" option to run_process_parallel()\n> run-command.c\n> fd3aaf53 1645) code = pp->start_failure(pp->ungroup ? NULL :\n> fd3aaf53 1646)  &pp->children[i].err,\n> fd3aaf53 1649) if (!pp->ungroup) {\n> fd3aaf53 1650) strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n> fd3aaf53 1651) strbuf_reset(&pp->children[i].err);\n\nThis change seems sufficiently complicated that it would be good to look\ninto the test coverage here. It's possible that it is covered by the\nGIT_TEST_* variables that I didn't use when generating my test coverage.\n\n> Christian Couder\t511cfd3b http: add custom hostname to IP address resolutions\n> http.c\n\n(These untested lines are my fault for not having Apache installed. I'll\nbe sure to include that in my coverage next time.)\n\n> Derrick Stolee\tb56166ca multi-pack-index: use --object-dir real path\n> builtin/multi-pack-index.c\n> b56166ca 61) opts.object_dir = xstrdup(get_object_directory());\n\nThis is demonstrating that opts.object_dir is never populated before\nwe parse the options. We must be handling a NULL object_dir properly\nsomewhere else. I'll work on a patch to fix this.\n\n> Derrick Stolee\t080ab56a cache-tree: implement cache_tree_find_path()\n> cache-tree.c\n> 080ab56a 104) struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path)\n\nHm. This commit claims that this method will be used later, but it never is.\n\nI even checked our code in microsoft/git and it's dead code there, too. We\nshould probably revert this commit. (Or: will it be useful for Shaoxuan's\nGSoC work? Perhaps we hold onto it for a little longer?)\n\n> Jeff Hostetler\t9915e08f t/helper/hexdump: add helper to print hexdump of stdin\n\nI did my testing on Linux, so all of the FS Monitor stuff will be untested.\nEven if we did the testing on macOS, I doubt that the daemon code would be\ntracked properly.\n\n> Neeraj Singh\t23a3a303 update-index: use the bulk-checkin infrastructure\n> builtin/update-index.c\n> 23a3a303 68) flush_odb_transaction();\n\nThis is signalling that we never use 'git update-index --verbose' in our\ntest suite. Might be worth fixing that.\n\n> Taylor Blau\t5b92477f builtin/gc.c: conditionally avoid pruning objects via loose\n> builtin/gc.c\n> 5b92477f 337) strvec_push(&repack, \"--cruft\");\n> 5b92477f 338) if (prune_expire)\n> 5b92477f 339) strvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n\nThe context here is this:\n\nstatic void add_repack_all_option(struct string_list *keep_pack)\n{\n\tif (prune_expire && !strcmp(prune_expire, \"now\"))\n\t\tstrvec_push(&repack, \"-a\");\n\telse if (cruft_packs) {\n\t\tstrvec_push(&repack, \"--cruft\");\n\t\tif (prune_expire)\n\t\t\tstrvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n\t} else {\n\nThe only test that checks this behavior runs \"git gc --cruft --prune=now\",\nso the \"-a\" option is being added and we never add the \"--cruft\" option.\n\nWe should probably add a test for \"git gc --cruft\" with a different prune\ntime.\n\n> Taylor Blau\tf9825d1c builtin/repack.c: support generating a cruft pack\n> builtin/repack.c\n> f9825d1c 680) strvec_pushf(&cmd.args, \"--cruft-expiration=%s\",\n\nThe --cruft-expiration option is not tested.\n\n> f9825d1c 708) fprintf(in, \"%s.pack\\n\", item->string);\n\nThis is related to the existence of .keep packs. A corner case, but maybe\nworth exploring.\n\n> pack-write.c\n> 5dfaf49a 330) unlink(mtimes_name);\n> 5dfaf49a 331) fd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n\nThis appears to be an interesting case for the write_mtimes_file() method,\nsince its first parameter is 'mtimes_name' and all tested cases are\npassing NULL, it seems.\n\nThanks,\n-Stolee\n\n"},{"id":"457219","messageId":"cdc0cba8-bd9d-9299-a769-aa7b522520ae@github.com","threadId":"58003","inReplyTo":"3d1c6dfd-1df6-3393-df5e-692719375772@github.com","subject":"Re: Git Test Coverage Report for v2.37.0-rc0","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T20:34:18Z","receivedAt":"2022-06-14T20:34:33Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/14/2022 4:20 PM, Derrick Stolee wrote:\n> On 6/14/2022 4:18 PM, Derrick Stolee wrote:\n>> Derrick Stolee\tb56166ca multi-pack-index: use --object-dir real path\n>> builtin/multi-pack-index.c\n>> b56166ca 61) opts.object_dir = xstrdup(get_object_directory());\n> \n> This is demonstrating that opts.object_dir is never populated before\n> we parse the options. We must be handling a NULL object_dir properly\n> somewhere else. I'll work on a patch to fix this.\n\nI was looking at the wrong line. This line is testing the --no-object-dir\noption, which is fine to leave untested, probably.\n\nThanks,\n-Stolee\n"},{"id":"457230","messageId":"Yqkd5GNYAKBnPQxz@nand.local","threadId":"58003","inReplyTo":"3d1c6dfd-1df6-3393-df5e-692719375772@github.com","subject":"Re: Git Test Coverage Report for v2.37.0-rc0","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-14T23:46:44Z","receivedAt":"2022-06-14T23:46:49Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 14, 2022 at 04:20:05PM -0400, Derrick Stolee wrote:\n> > Taylor Blau\tf9825d1c builtin/repack.c: support generating a cruft pack\n> > builtin/repack.c\n> > f9825d1c 680) strvec_pushf(&cmd.args, \"--cruft-expiration=%s\",\n>\n> The --cruft-expiration option is not tested.\n\nNote that only the `--cruft-expiration` option of `git repack` is\nuntested. This code is just passing down the expiration from `repack`'s\nperspective down to pack-objects, which has extensive coverage of the\n`--cruft-expiration` option in t5329.\n\nIt does feel a little weird to leave this uncovered, but I don't think\nit's substantially different from the uncovered lines in setting up\n`cruft_po_args` on lines 86-90 of the repack code.\n\n> > f9825d1c 708) fprintf(in, \"%s.pack\\n\", item->string);\n>\n> This is related to the existence of .keep packs. A corner case, but maybe\n> worth exploring.\n\nProbably worth testing. We're ensuring that `.keep` packs in addition to\n`--keep-pack`s are excluded from the cruft pack. That isn't tested and\nprobably should be.\n>\n> > pack-write.c\n> > 5dfaf49a 330) unlink(mtimes_name);\n> > 5dfaf49a 331) fd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n>\n> This appears to be an interesting case for the write_mtimes_file() method,\n> since its first parameter is 'mtimes_name' and all tested cases are\n> passing NULL, it seems.\n\nUgh, yes :-). That's imitating a common pattern in the write_xyz_file()\nfor xyz referring to auxiliary pack data. The cleanup should be pretty\nstraightforward, I'll send a patch or two shortly...\n\nThanks,\nTaylor\n"},{"id":"457256","messageId":"220615.86mteexls9.gmgdl@evledraar.gmail.com","threadId":"58003","inReplyTo":"3d1c6dfd-1df6-3393-df5e-692719375772@github.com","subject":"Re: Git Test Coverage Report for v2.37.0-rc0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-15T10:16:53Z","receivedAt":"2022-06-15T10:29:18Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 14 2022, Derrick Stolee wrote:\n\nNeat, thanks! Re git-test-coverage: Maybe it's covered somewhere, but is\nthere tooling to reproduce this somewhere? I.e. we have \"make coverage\"\nin tree, but not this \"do the diff and blame on lines\" step, unless I've\nmissed it somewhere...\n\n> On 6/14/2022 4:18 PM, Derrick Stolee wrote:\n>> Ævar Arnfjörð Bjarmason\t338959da remote.c: remove braces from one-statement \"for\"-loops\n>> remote.c\n>> 338959da 149) for (i = 0; i < remote->url_nr; i++)\n>> 338959da 153) for (i = 0; i < remote->pushurl_nr; i++)\n>> Ævar Arnfjörð Bjarmason\t323822c7 remote.c: don't dereference NULL in freeing loop\n>> remote.c\n>> 323822c7 151) FREE_AND_NULL(remote->url);\n>\n> Just noting that these lines are inside remote_clear() which is called by\n> remote_state_clear(), which is called by repo_clear(). Apparently we have\n> no tests that clear a 'struct repository' that loaded remotes? This sounds\n> likely since we don't close these unless they are not the_repository.\n\nYeah, it's also why we didn't segfault in practice on the landmine I\nfixed in 323822c72be (remote.c: don't dereference NULL in freeing loop,\n2022-06-07).\n\n>> Ævar Arnfjörð Bjarmason\tfd3aaf53 run-command: add an \"ungroup\" option to run_process_parallel()\n>> run-command.c\n>> fd3aaf53 1645) code = pp->start_failure(pp->ungroup ? NULL :\n>> fd3aaf53 1646)  &pp->children[i].err,\n>> fd3aaf53 1649) if (!pp->ungroup) {\n>> fd3aaf53 1650) strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n>> fd3aaf53 1651) strbuf_reset(&pp->children[i].err);\n>\n> This change seems sufficiently complicated that it would be good to look\n> into the test coverage here. It's possible that it is covered by the\n> GIT_TEST_* variables that I didn't use when generating my test coverage.\n\nWe should definitely have test coverage for these, but FWIW it's all\npre-existing-not-tested code.\n\nI.e. this has presumably been untested as far back as c553c72eed6\n(run-command: add an asynchronous parallel child processor,\n2015-12-15). It's just:\n\n\tif (start_command(&pp->children[i].process))\n        \t/* this whole thing is untested, as no \"start command\" fails */\n\nThere's probably no easy way to automate this, but for significant bonus\npoints I think a script like this would be much more useful if once it\nidentifies given commit:line pairs it would check out $that_commit^, run\n\"make coverage\" again, see how pre-image faired (presumably --word-diff\nwould associate them), and post that diff on top. \n\nI see in this case it's as easy as tweaking t0061-run-command.sh to do:\n\n    test-tool run-command run-command-parallel 5 this-command-does-not-exist\n\nI.e. we just don't stress that failure path, but should.\n\nI'm planning to submit a cleanup series for the \"ungroup\" feature,\ni.e. the API is weird because we were trying to come up with a minimal\nregression fix. I'll make sure to include tests for this scenario in\nthat series. Thanks!\n"},{"id":"457278","messageId":"604c694e-7b8f-40bd-c8da-93b00dfe3541@github.com","threadId":"58003","inReplyTo":"220615.86mteexls9.gmgdl@evledraar.gmail.com","subject":"Re: Git Test Coverage Report for v2.37.0-rc0","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-15T13:21:48Z","receivedAt":"2022-06-15T13:21:53Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/15/22 6:16 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Jun 14 2022, Derrick Stolee wrote:\n> \n> Neat, thanks! Re git-test-coverage: Maybe it's covered somewhere, but is\n> there tooling to reproduce this somewhere? I.e. we have \"make coverage\"\n> in tree, but not this \"do the diff and blame on lines\" step, unless I've\n> missed it somewhere...\n\nThere is a script at contrib/coverage-diff.sh, but I then created a\nseparate project [1] that I could iterate on more quickly for new\nformats (like the commit-grouped output you see here).\n\nI was running builds on Azure Pipelines [2], but they started failing\na couple years ago (I think because of timeouts, but maybe also because\nof failures in 'seen'). I tried resurrecting them recently, but failed.\nSo, I just ran this locally on my own machine.\n\n[1] https://github.com/derrickstolee/git-test-coverage\n[2] https://dev.azure.com/git/git/_build?definitionId=12\n\n>>> Ævar Arnfjörð Bjarmason\tfd3aaf53 run-command: add an \"ungroup\" option to run_process_parallel()\n>>> run-command.c\n>>> fd3aaf53 1645) code = pp->start_failure(pp->ungroup ? NULL :\n>>> fd3aaf53 1646)  &pp->children[i].err,\n>>> fd3aaf53 1649) if (!pp->ungroup) {\n>>> fd3aaf53 1650) strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n>>> fd3aaf53 1651) strbuf_reset(&pp->children[i].err);\n>>\n>> This change seems sufficiently complicated that it would be good to look\n>> into the test coverage here. It's possible that it is covered by the\n>> GIT_TEST_* variables that I didn't use when generating my test coverage.\n> \n> We should definitely have test coverage for these, but FWIW it's all\n> pre-existing-not-tested code.\n\nYes, I agree that we should not hold up new changes just because old\ncode isn't tested properly. It is worth considering whether we can\nadd tests easily _or_ if we should just give the new code another look.\n\n> I.e. this has presumably been untested as far back as c553c72eed6\n> (run-command: add an asynchronous parallel child processor,\n> 2015-12-15). It's just:\n> \n> \tif (start_command(&pp->children[i].process))\n>         \t/* this whole thing is untested, as no \"start command\" fails */\n> \n> There's probably no easy way to automate this, but for significant bonus\n> points I think a script like this would be much more useful if once it\n> identifies given commit:line pairs it would check out $that_commit^, run\n> \"make coverage\" again, see how pre-image faired (presumably --word-diff\n> would associate them), and post that diff on top. \n\nI think it's worth pointing out which lines are changing untested\ncode.\n\nTwo core principles in \"Working Effectively with Legacy Code\" by\nMichael Feathers [3] are:\n\n1. Untested code is legacy code.\n2. Add tests before changing legacy code.\n\n[3] https://www.oreilly.com/library/view/working-effectively-with/0131177052/\n\nThe point of this report is to see how well we are fitting with those\nprinciples, but also to take a pragmatic approach to deciding whether\nnew tests are worth the effort.\n\n> I see in this case it's as easy as tweaking t0061-run-command.sh to do:\n> \n>     test-tool run-command run-command-parallel 5 this-command-does-not-exist\n> \n> I.e. we just don't stress that failure path, but should.\n> \n> I'm planning to submit a cleanup series for the \"ungroup\" feature,\n> i.e. the API is weird because we were trying to come up with a minimal\n> regression fix. I'll make sure to include tests for this scenario in\n> that series. Thanks!\n\nThank you!\n\nIt might also be worth pointing out that the 'coverage-test' build\ndisables threading due to colliding in the *.gcov output. That leads\nto untested code that is actually tested by the \"real\" test suite.\n\n-Stolee\n"}]}