{"thread":{"id":"53480","subject":"Git Test Coverage Report (v2.27.0-rc0)","startedAt":"2020-05-15T17:22:35Z","lastAt":"2020-05-23T12:22:16Z","messageCount":16,"participants":["Derrick Stolee","Jonathan Tan","René Scharfe","brian m. carlson","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"397904","messageId":"ef72aedf-4264-e386-9563-050c54483c93@gmail.com","threadId":"53480","inReplyTo":null,"subject":"Git Test Coverage Report (v2.27.0-rc0)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-15T17:22:29Z","receivedAt":"2020-05-15T17:22:35Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Here is the test coverage report for the release candidate\nv2.27.0-rc0. Please take close look at this report for code\nthat has been edited since the last release (v2.26.2) but is\nnot covered by the test suite. Bugs tend to hide in uncovered\ncode!\n\nThe full report also includes changes in 'next' and 'pu' [1].\n\nThanks,\n-Stolee\n\n[1] https://derrickstolee.github.io/git-test-coverage/reports/v2.27.0-rc0-commits.txt\n\n---\n\nUncovered code in 'v2.27.0-rc0' not in 'v2.26.2'\n--------------------------------------------------------\n\nCommits introducing uncovered code:\nÆvar Arnfjörð Bjarmason\t5cc044e0 get_short_oid: sort ambiguous objects by type, then SHA-1\noid-array.c\n5cc044e0 56) return ret;\n\nAlexandr Miloslavskiy\t49d3c4b4 get_superproject_working_tree(): return strbuf\nsubmodule.c\n49d3c4b4 2188) return 0;\n49d3c4b4 2191) return 0;\n49d3c4b4 2245) return 0;\n\nAlexandr Miloslavskiy\t3d7747e3 real_path: remove unsafe API\neditor.c\n3d7747e3 87) strbuf_release(&realpath);\n\nsetup.c\n3d7747e3 78) strbuf_release(&realpath);\n\nbrian m. carlson\t8b8f7189 builtin/init-db: allow specifying hash algorithm on command line\nbuiltin/init-db.c\n8b8f7189 376) repo_fmt->hash_algo = hash;\n8b8f7189 599) hash_algo = hash_algo_by_name(object_format);\n8b8f7189 600) if (hash_algo == GIT_HASH_UNKNOWN)\n\nbrian m. carlson\te02a7141 worktree: allow repository version 1\nworktree.c\ne02a7141 476) strbuf_release(&err);\n\nbrian m. carlson\tedc6dccf builtin/receive-pack: use constant-time comparison for HMAC value\nbuiltin/receive-pack.c\nedc6dccf 569) retval = NONCE_BAD;\nedc6dccf 570) goto leave;\n\nbrian m. carlson\tab90ecae convert: permit passing additional metadata to filter processes\nconvert.c\nab90ecae 862) goto done;\nab90ecae 868) goto done;\nab90ecae 874) goto done;\n\nbrian m. carlson\t768e30ea hash: implement and use a context cloning function\nsha1-file.c\n768e30ea 98) static void git_hash_sha256_clone(git_hash_ctx *dst, const git_hash_ctx *src)\n768e30ea 100) git_SHA256_Clone(&dst->sha256, &src->sha256);\n768e30ea 101) }\n768e30ea 118) static void git_hash_unknown_clone(git_hash_ctx *dst, const git_hash_ctx *src)\n\nbrian m. carlson\t3c9331a1 builtin/init-db: add environment variable for new repo hash\nbuiltin/init-db.c\n3c9331a1 378) int env_algo = hash_algo_by_name(env);\n3c9331a1 379) if (env_algo == GIT_HASH_UNKNOWN)\n3c9331a1 381) repo_fmt->hash_algo = env_algo;\n\nbrian m. carlson\t910650d2 Rename sha1_array to oid_array\nt/helper/test-oid-array.c\n910650d2 29) oid_array_clear(&array);\n\nbrian m. carlson\tddddf8d7 fast-import: permit reading multiple marks files\nfast-import.c\nddddf8d7 1158) insert_mark(marks, mark, e);\n\nbrian m. carlson\t1bdca816 fast-import: add options for rewriting submodules\nfast-import.c\n1bdca816 2192) return -1;\n1bdca816 3067) return;\n1bdca816 3325) die_errno(\"cannot read '%s'\", f);\n\nbrian m. carlson\t13e7ed6a builtin/checkout: compute checkout metadata for checkouts\nbuiltin/checkout.c\n13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :\n\nbrian m. carlson\tefa7ae36 init-db: move writing repo version into a function\nbuiltin/init-db.c\nefa7ae36 192) repo_version = GIT_REPO_VERSION_READ;\nefa7ae36 200) git_config_set(\"extensions.objectformat\",\n\nDenton Liu\t65c425a2 sequencer: stop leaking buf\nsequencer.c\n65c425a2 2557) goto done_rebase_i;\n\nDenton Liu\tbe1bb600 sequencer: make apply_autostash() accept a path\nsequencer.c\nbe1bb600 5203) apply_autostash(rebase_path_autostash());\n\nDenton Liu\tce6521e4 Lib-ify fmt-merge-msg\nfmt-merge-msg.c\nce6521e4 24) use_branch_desc = git_config_bool(key, value);\nce6521e4 75) continue;\nce6521e4 107) return 1;\nce6521e4 113) return 2;\nce6521e4 117) return 3;\nce6521e4 126) line[len - 1] = 0;\nce6521e4 167) origin = src;\nce6521e4 168) string_list_append(&src_data->generic, line);\nce6521e4 169) src_data->head_status |= 2;\nce6521e4 188) return;\nce6521e4 201) static void add_branch_desc(struct strbuf *out, const char *name)\nce6521e4 203) struct strbuf desc = STRBUF_INIT;\nce6521e4 205) if (!read_branch_desc(&desc, name)) {\nce6521e4 206) const char *bp = desc.buf;\nce6521e4 207) while (*bp) {\nce6521e4 208) const char *ep = strchrnul(bp, '\\n');\nce6521e4 209) if (*ep)\nce6521e4 210) ep++;\nce6521e4 211) strbuf_addf(out, \"  : %.*s\", (int)(ep - bp), bp);\nce6521e4 212) bp = ep;\nce6521e4 214) strbuf_complete_line(out);\nce6521e4 216) strbuf_release(&desc);\nce6521e4 217) }\nce6521e4 231) return;\nce6521e4 239) return;\nce6521e4 276) else if (people->nr)\nce6521e4 277) strbuf_addf(out, \"%s (%d) and others\",\nce6521e4 278)     people->items[0].string,\nce6521e4 279)     (int)util_as_integral(&people->items[0]));\nce6521e4 342) return;\nce6521e4 355) if (opts->credit_people)\nce6521e4 356) record_person('c', &committers, commit);\nce6521e4 372) string_list_append(&subjects,\nce6521e4 373)    oid_to_hex(&commit->object.oid));\nce6521e4 387) add_branch_desc(out, name);\nce6521e4 425) subsep = \", \";\nce6521e4 426) strbuf_addstr(out, \"HEAD\");\nce6521e4 446) strbuf_addstr(out, subsep);\nce6521e4 447) print_joined(\"commit \", \"commits \", &src_data->generic,\nce6521e4 507) if (tag_number == 2) {\nce6521e4 508) struct strbuf tagline = STRBUF_INIT;\nce6521e4 509) strbuf_addch(&tagline, '\\n');\nce6521e4 510) strbuf_add_commented_lines(&tagline,\nce6521e4 511) origins.items[first_tag].string,\nce6521e4 512) strlen(origins.items[first_tag].string));\nce6521e4 513) strbuf_insert(&tagbuf, 0, tagline.buf,\nce6521e4 515) strbuf_release(&tagline);\nce6521e4 517) strbuf_addch(&tagbuf, '\\n');\nce6521e4 518) strbuf_add_commented_lines(&tagbuf,\nce6521e4 519) origins.items[i].string,\nce6521e4 520) strlen(origins.items[i].string));\nce6521e4 521) fmt_tag_signature(&tagbuf, &sig, buf, len);\nce6521e4 566) continue;\n\nDenton Liu\t5b2f6d9c sequencer: make file exists check more efficient\nsequencer.c\n5b2f6d9c 431) warning_errno(_(\"could not read '%s'\"), path);\n\nDenton Liu\tb309a971 reset: extract reset_head() from rebase\nreset.c\nb309a971 37) ret = -1;\nb309a971 38) goto leave_reset_head;\nb309a971 43) goto leave_reset_head;\nb309a971 66) goto leave_reset_head;\nb309a971 72) goto leave_reset_head;\nb309a971 77) goto leave_reset_head;\nb309a971 81) ret = -1;\nb309a971 82) goto leave_reset_head;\nb309a971 90) goto leave_reset_head;\nb309a971 109) } else if (old_orig)\nb309a971 110) delete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n\nDenton Liu\t9460fd48 Lib-ify prune-packed\nprune-packed.c\n9460fd48 26) printf(\"rm -f %s\\n\", path);\n9460fd48 35) progress = start_delayed_progress(_(\"Removing duplicate objects\"), 256);\n\nDenton Liu\tefcf6cf0 rebase: use read_oneliner()\nbuiltin/rebase.c\nefcf6cf0 625) } else if (!read_oneliner(&buf, state_dir_path(\"head\", opts),\n\nDenton Liu\t7cd54d37 wrapper: indent with tabs\nwrapper.c\n7cd54d37 221) len = MAX_IO_SIZE;\n7cd54d37 243) len = MAX_IO_SIZE;\n\nDenton Liu\t86ed00af rebase: use apply_autostash() from sequencer.c\nbuiltin/rebase.c\n86ed00af 1040) apply_autostash(state_dir_path(\"autostash\", opts));\n\nDenton Liu\tf213f069 rebase: generify reset_head()\nbuiltin/rebase.c\nf213f069 879) reset_head(the_repository, &opts->orig_head, \"checkout\",\nf213f069 880)    opts->head_name, 0,\n\nDerrick Stolee\t0906ac2b blame: use changed-path Bloom filters\nblame.c\n0906ac2b 1281) return 1;\n0906ac2b 1302) bd->alloc *= 2;\n0906ac2b 1303) REALLOC_ARRAY(bd->keys, bd->alloc);\n0906ac2b 2899) return;\n\nDerrick Stolee\t3ce4ca0a multi-pack-index: respect repack.packKeptObjects=false\nmidx.c\n3ce4ca0a 1307) continue;\n\nDerrick Stolee\tc9f7a793 log-tree: make ref_filter_match() a helper method\nlog-tree.c\nc9f7a793 94)     (!*rest || *rest == '/'))\n\nDerrick Stolee\t8918e379 revision: complicated pathspecs disable filters\nrevision.c\n8918e379 658) return 1;\n8918e379 662) return 1;\n\nDerrick Stolee\t6c622f9f commit-graph: write commit-graph chains\ncommit-graph.c\n6c622f9f 1586) return -1;\n\nDerrick Stolee\te3696980 diff: halt tree-diff early after max_changes\ntree-diff.c\ne3696980 438) break;\n\nĐoàn Trần Công Danh\tc933b28d date.c: s/is_date/set_date/\ndate.c\nc933b28d 521) return -1;\nc933b28d 532) return -1;\n\nĐoàn Trần Công Danh\t4f89f4fc date.c: validate and set time in a helper function\ndate.c\n4f89f4fc 553) return -1;\n\nElijah Newren\t30e89c12 unpack-trees: pull sparse-checkout pattern reading into a new function\nunpack-trees.c\n30e89c12 1556) o->skip_sparse_checkout = 1;\n\nElijah Newren\t16846444 dir: include DIR_KEEP_UNTRACKED_CONTENTS handling in treat_directory()\ndir.c\n16846444 1924) FREE_AND_NULL(dir->entries[i]);\n\nElijah Newren\t7af7a258 unpack-trees: add a new update_sparsity() function\nunpack-trees.c\n7af7a258 1788) goto skip_sparse_checkout;\n7af7a258 1815) ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;\n\nElijah Newren\tb9cbd295 rebase: reinstate --no-keep-empty\nsequencer.c\nb9cbd295 4756) continue;\n\nEmily Shaffer\t709df95b help: move list_config_help to builtin/help\nbuiltin/help.c\n709df95b 123) puts(var);\n709df95b 124) continue;\n\nEmily Shaffer\t1411914a bugreport: add uname info\nbugreport.c\n1411914a 20) strbuf_addf(sys_info, _(\"uname() failed with error '%s' (%d)\\n\"),\n1411914a 22)     errno);\n\nEmily Shaffer\t617d5719 bugreport: gather git version and build info\nhelp.c\n617d5719 641) strbuf_addstr(buf, \"no commit associated with this build\\n\");\n\nGarima Singh\ta56b9464 revision.c: use Bloom filters to speed up path based revision walks\nbloom.c\na56b9464 286) return -1;\n\nrevision.c\na56b9464 721) return -1;\n\nGarima Singh\t3d112755 commit-graph: examine commits by generation number\ncommit-graph.c\n3d112755 1307) QSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);\n\nGarima Singh\ted591feb bloom.c: core Bloom filter implementation for changed paths.\nbloom.c\ned591feb 185) return NULL;\ned591feb 266) for (i = 0; i < diff_queued_diff.nr; i++)\ned591feb 267) diff_free_filepair(diff_queued_diff.queue[i]);\ned591feb 268) filter->data = NULL;\ned591feb 269) filter->len = 0;\n\nGarima Singh\t76ffbca7 commit-graph: write Bloom filters to commit graph file\ncommit-graph.c\n76ffbca7 337) chunk_repeated = 1;\n76ffbca7 344) chunk_repeated = 1;\n76ffbca7 351) break;\n76ffbca7 1095) progress = start_delayed_progress(\n76ffbca7 1097) ctx->commits.nr);\n76ffbca7 1120) progress = start_delayed_progress(\n76ffbca7 1122) ctx->commits.nr);\n\nGarima Singh\tf1294eaf bloom.c: introduce core Bloom filter constructs\nt/helper/test-bloom.c\nf1294eaf 25) printf(\"No filter.\\n\");\nf1294eaf 26) return;\n\nGarima Singh\tf97b9325 commit-graph: compute Bloom filters for changed paths\ncommit-graph.c\nf97b9325 1299) progress = start_delayed_progress(\nf97b9325 1301) ctx->commits.nr);\n\nHans Jerry Illikainen\t67948981 gpg-interface: prefer check_signature() for GPG verification\ngpg-interface.c\n67948981 275) error_errno(_(\"failed writing detached signature to '%s'\"),\n67948981 277) delete_tempfile(&temp);\n67948981 278) return -1;\n67948981 293) gpg_status = &buf;\n\nlog-tree.c\n67948981 512) show_sig_lines(opt, status, \"No signature\\n\");\n\nHeba Waly\tb3b18d16 advice: revamp advise API\nadvice.c\nb3b18d16 178) return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&\nb3b18d16 179)        advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;\n\nJeff King\td21ee7d1 commit-graph: examine changed-path objects in pack order\ncommit-graph.c\nd21ee7d1 77) return; /* should never happen, but be lenient */\nd21ee7d1 82) static int commit_pos_cmp(const void *va, const void *vb)\nd21ee7d1 84) const struct commit *a = *(const struct commit **)va;\nd21ee7d1 85) const struct commit *b = *(const struct commit **)vb;\nd21ee7d1 86) return commit_pos_at(&commit_pos, a) -\nd21ee7d1 87)        commit_pos_at(&commit_pos, b);\n\nJeff King\t16ddcd40 sha1_array: let callbacks interrupt iteration\noid-array.c\n16ddcd40 76) return ret;\n\nJeff King\t1b4c57fa test-bloom: check that we have expected arguments\nt/helper/test-bloom.c\n1b4c57fa 54) usage(bloom_usage);\n1b4c57fa 59) usage(bloom_usage);\n1b4c57fa 71) usage(bloom_usage);\n1b4c57fa 85) usage(bloom_usage);\n\nJeff King\td8410a81 fast-import: replace custom hash with hashmap.c\nfast-import.c\nd8410a81 60) e2 = container_of(entry_or_key, const struct object_entry, ent);\nd8410a81 61) return oidcmp(&e1->idx.oid, &e2->idx.oid);\n\nJeff King\t348482de config: reject parsing of files over INT_MAX\nconfig.c\n348482de 537) cf->eof = 1;\n348482de 538) return 0;\n\nJohannes Schindelin\t12294990 credential: handle `credential.<partial-URL>.<key>` again\nurlmatch.c\n12294990 581) retval = 0;\n\nJonathan Tan\tfbda77c6 commit-graph: avoid Memory Leaks\ncommit-graph.c\nfbda77c6 296) goto free_and_return;\nfbda77c6 363) goto free_and_return;\n\nJonathan Tan\t95acf11a diff: restrict when prefetching occurs\ndiffcore-rename.c\n95acf11a 137) static void prefetch(void *prefetch_options)\n95acf11a 139) struct prefetch_options *options = prefetch_options;\n95acf11a 141) struct oid_array to_fetch = OID_ARRAY_INIT;\n95acf11a 143) for (i = 0; i < rename_dst_nr; i++) {\n95acf11a 144) if (rename_dst[i].pair)\n95acf11a 149) continue; /* already found exact match */\n95acf11a 150) diff_add_if_missing(options->repo, &to_fetch,\n95acf11a 151)     rename_dst[i].two);\n95acf11a 153) for (i = 0; i < rename_src_nr; i++) {\n95acf11a 154) if (options->skip_unmodified &&\n95acf11a 155)     diff_unmodified_pair(rename_src[i].p))\n95acf11a 160) continue;\n95acf11a 161) diff_add_if_missing(options->repo, &to_fetch,\n95acf11a 162)     rename_src[i].p->one);\n95acf11a 164) promisor_remote_get_direct(options->repo, to_fetch.oid, to_fetch.nr);\n95acf11a 165) oid_array_clear(&to_fetch);\n95acf11a 166) }\n\nJorge Lopez Silva\taf026519 http: add environment variable support for HTTPS proxies\nhttp.c\naf026519 1221) proxy_ssl_cert_password_required = 1;\n\nJorge Lopez Silva\t88238e02 http: add client cert support for HTTPS proxies\nhttp.c\n88238e02 376) return git_config_string(&http_proxy_ssl_cert, var, value);\n88238e02 379) return git_config_string(&http_proxy_ssl_key, var, value);\n88238e02 382) return git_config_string(&http_proxy_ssl_ca_info, var, value);\n88238e02 385) proxy_ssl_cert_password_required = git_config_bool(var, value);\n88238e02 386) return 0;\n88238e02 966) if (ssl_cainfo != NULL)\n88238e02 967) curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);\n88238e02 1294) memset(proxy_cert_auth.password, 0, strlen(proxy_cert_auth.password));\n88238e02 1295) FREE_AND_NULL(proxy_cert_auth.password);\n\nJosh Steadmon\t3d3adaad trace2: teach Git to log environment variables\ntrace2/tr2_cfg.c\n3d3adaad 63) return tr2_cfg_env_vars_count;\n\nJunio C Hamano\t56a1d9ca Merge branch 'dl/libify-a-few'\nfmt-merge-msg.c\n56a1d9ca 498) strbuf_addstr(&sig, \"gpg verification failed.\\n\");\n\nKarsten Blees\tdefd7c7b dir.c: git-status --ignored: don't scan the work tree three times\ndir.c\ndefd7c7b 2177) return path_none;\n\nPatrick Steinhardt\tedc30691 refs: fix segfault when aborting empty transaction\nrefs/files-backend.c\nedc30691 2572) strbuf_release(&err);\n\nPatrick Steinhardt\ta65b8ac2 update-ref: organize commands in an array\nbuiltin/update-ref.c\na65b8ac2 419) continue;\n\nPhillip Wood\t430b75f7 commit: give correct advice for empty commit during a rebase\nsequencer.c\n430b75f7 1469)     return -1;\n\nRené Scharfe\t9068cfb2 fsck: report non-consecutive duplicate names in trees\nfsck.c\n9068cfb2 623) break;\n9068cfb2 626) if (is_less_than_slash(*p)) {\n9068cfb2 627) name_stack_push(candidates, f_name);\n9068cfb2 628) break;\n9068cfb2 630) }\n\nSon Luong Ngoc\te11d86de midx: teach \"git multi-pack-index repack\" honor \"git repack\" configurations\nmidx.c\ne11d86de 1420) argv_array_push(&cmd.args, \"--delta-islands\");\n\nTaylor Blau\t37b9dcab shallow.c: use '{commit,rollback}_shallow_file'\nbuiltin/receive-pack.c\n37b9dcab 897) rollback_shallow_file(the_repository, &shallow_lock);\n\nTaylor Blau\tfdbde82f builtin/commit-graph.c: introduce split strategy 'no-merge'\ncommit-graph.c\nfdbde82f 1789) break;\n\nVasil Dimov\t8cf51561 range-diff: fix a crash in parsing git-log output\nrange-diff.c\n8cf51561 115) string_list_clear(list, 1);\n8cf51561 116) strbuf_release(&buf);\n8cf51561 117) strbuf_release(&contents);\n8cf51561 118) finish_command(&cp);\n8cf51561 119) return -1;\n\n"},{"id":"398198","messageId":"853759d3-97c3-241f-98e1-990883cd204e@gmail.com","threadId":"53480","inReplyTo":"ef72aedf-4264-e386-9563-050c54483c93@gmail.com","subject":"Re: Git Test Coverage Report (v2.27.0-rc0)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-19T12:11:15Z","receivedAt":"2020-05-19T12:11:20Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/15/2020 1:22 PM, Derrick Stolee wrote:\n> Bugs tend to hide in uncovered\n> code!\n\nImmediately after sending the report, I regretted the ambiguity of\nthis sentence. Here are a couple things to point out:\n\n* Just because code is uncovered does not mean it is more likely\n  to contain a bug. It just means that code review is our only\n  protection. This is completely acceptable for things like error\n  conditions we never expect to show up. We also don't hold authors\n  accountable for existing bugs when they do a mechanical refactor\n  of existing, uncovered code.\n\n* Just because code is _covered_ does not mean it is less likely\n  to contain a bug. There are all sorts of reasons that coverage\n  is not enough to \"guarantee\" correct behavior.\n\nMy process in this email is to check each commit's diff and\neliminate any lines that are obviously part of error conditions\n(and there isn't a worry about a use-after-free for an error\nmessage or something).\n\nI've CC'd the authors for the commits that I think could use\na second look. Please evaluate the following:\n\n 1. Is this code definitely correct?\n 2. How difficult would it be to cover this with a test?\n\n> brian m. carlson\t13e7ed6a builtin/checkout: compute checkout metadata for checkouts\n> builtin/checkout.c\n> 13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :\n\nThis is part of the following hunk:\n\n@@ -619,6 +620,11 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n        opts.verbose_update = o->show_progress;\n        opts.src_index = &the_index;\n        opts.dst_index = &the_index;\n+       init_checkout_metadata(&opts.meta, info->refname,\n+                              info->commit ? &info->commit->object.oid :\n+                              is_null_oid(&info->oid) ? &tree->object.oid :\n+                              &info->oid,\n+                              NULL);\n        parse_tree(tree);\n        init_tree_desc(&tree_desc, tree->buffer, tree->size);\n        switch (unpack_trees(1, &tree_desc, &opts)) {\n\nThe double-nested ternary definitely complicates the coverage here.\nIt also points out that all tests have `info->commit` a non-NULL.\n\nThis certainly looks safe, but I don't know.\n\n> Denton Liu\tbe1bb600 sequencer: make apply_autostash() accept a path\n> sequencer.c\n> be1bb600 5203) apply_autostash(rebase_path_autostash());\n\nThis line is identical to four other lines inserted by the\nsame patch.\n\n> Denton Liu\tce6521e4 Lib-ify fmt-merge-msg\n> fmt-merge-msg.c\n\n(This patch moves a bunch of old code, so we should not\nfault the author for its coverage.)\n\n> Derrick Stolee\t0906ac2b blame: use changed-path Bloom filters\n> blame.c\n> 0906ac2b 1302) bd->alloc *= 2;\n> 0906ac2b 1303) REALLOC_ARRAY(bd->keys, bd->alloc);\n> 0906ac2b 2899) return;\n\nThis hunk is part of this method:\n\n+static void add_bloom_key(struct blame_bloom_data *bd,\n+                         const char *path)\n+{\n+       if (!bd)\n+               return;\n+\n+       if (bd->nr >= bd->alloc) {\n+               bd->alloc *= 2;\n+               REALLOC_ARRAY(bd->keys, bd->alloc);\n+       }\n+\n+       bd->keys[bd->nr] = xmalloc(sizeof(struct bloom_key));\n+       fill_bloom_key(path, strlen(path), bd->keys[bd->nr], bd->settings);\n+       bd->nr++;\n+}\n\nThis is not being covered because the blame tests must not\nbe splitting the files across enough renames to trigger the\nrealloc. I don't think it is worth creating such a complicated\ntest.\n\n> Derrick Stolee\tc9f7a793 log-tree: make ref_filter_match() a helper method\n> log-tree.c\n> c9f7a793 94)     (!*rest || *rest == '/'))\n\nThis is moved code:\n\n+               const char *rest;\n+               if (skip_prefix(refname, item->string, &rest) &&\n+                   (!*rest || *rest == '/'))\n+                       matched = 1;\n\nSince the \"matched = 1;\" line is covered, this must mean that\nany tests we use have \"!*rest\" be true and the \"*rest == '/'\"\nis not covered. \n\n> Elijah Newren\t30e89c12 unpack-trees: pull sparse-checkout pattern reading into a new function\n> unpack-trees.c\n> 30e89c12 1556) o->skip_sparse_checkout = 1;\n\nError condition (when sparse-checkout fails to parse).\n\n> Garima Singh\t3d112755 commit-graph: examine commits by generation number\n> commit-graph.c\n> 3d112755 1307) QSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);\n\nWhile this is blamed to 3d112755, it's actually moved from d21ee7d11 (commit-graph:\nexamine changed-path objects in pack order, 2020-03-30) which introduced the sort.\n\nThe reason this isn't covered is because all changed-path tests use --reachable\nand none use the default which scans pack-files.\n\n> Garima Singh\ted591feb bloom.c: core Bloom filter implementation for changed paths.\n> bloom.c\n> ed591feb 266) for (i = 0; i < diff_queued_diff.nr; i++)\n> ed591feb 267) diff_free_filepair(diff_queued_diff.queue[i]);\n> ed591feb 268) filter->data = NULL;\n> ed591feb 269) filter->len = 0;\n\nThis is the case when a commit has more than 512 changes. The plan is to\nadd a test after this 512 limit is adjustable by config. (Adding to my TODO\nlist [1])\n\n[1] https://github.com/microsoft/git/issues/272\n\n> Jeff King\td21ee7d1 commit-graph: examine changed-path objects in pack order\n> commit-graph.c\n> d21ee7d1 77) return; /* should never happen, but be lenient */\n> d21ee7d1 82) static int commit_pos_cmp(const void *va, const void *vb)\n> d21ee7d1 84) const struct commit *a = *(const struct commit **)va;\n> d21ee7d1 85) const struct commit *b = *(const struct commit **)vb;\n> d21ee7d1 86) return commit_pos_at(&commit_pos, a) -\n> d21ee7d1 87)        commit_pos_at(&commit_pos, b);\n\nThese would be called by the QSORT mentioned earlier.\n\n> Jonathan Tan\t95acf11a diff: restrict when prefetching occurs\n> diffcore-rename.c\n> 95acf11a 137) static void prefetch(void *prefetch_options)\n> 95acf11a 139) struct prefetch_options *options = prefetch_options;\n> 95acf11a 141) struct oid_array to_fetch = OID_ARRAY_INIT;\n> 95acf11a 143) for (i = 0; i < rename_dst_nr; i++) {\n> 95acf11a 144) if (rename_dst[i].pair)\n> 95acf11a 149) continue; /* already found exact match */\n> 95acf11a 150) diff_add_if_missing(options->repo, &to_fetch,\n> 95acf11a 151)     rename_dst[i].two);\n> 95acf11a 153) for (i = 0; i < rename_src_nr; i++) {\n> 95acf11a 154) if (options->skip_unmodified &&\n> 95acf11a 155)     diff_unmodified_pair(rename_src[i].p))\n> 95acf11a 160) continue;\n> 95acf11a 161) diff_add_if_missing(options->repo, &to_fetch,\n> 95acf11a 162)     rename_src[i].p->one);\n> 95acf11a 164) promisor_remote_get_direct(options->repo, to_fetch.oid, to_fetch.nr);\n> 95acf11a 165) oid_array_clear(&to_fetch);\n> 95acf11a 166) }\n\nOdd that this static method is never called, even once.\n\nThe only reference is here:\n\n+       struct prefetch_options prefetch_options = {r, skip_unmodified};\n+\n+       if (r == the_repository && has_promisor_remote()) {\n+               dpf_options.missing_object_cb = prefetch;\n+               dpf_options.missing_object_data = &prefetch_options;\n+       }\n\nbut this block is definitely being called. Perhaps the prefetch\nis never being called because we are never in a situation where there\nare any missing objects?\n\n> René Scharfe\t9068cfb2 fsck: report non-consecutive duplicate names in trees\n> fsck.c\n> 9068cfb2 623) break;\n> 9068cfb2 626) if (is_less_than_slash(*p)) {\n> 9068cfb2 627) name_stack_push(candidates, f_name);\n> 9068cfb2 628) break;\n> 9068cfb2 630) }\n \nThese are different conditions in verify_ordered() to check the\nspecial case of blobs and trees having the \"same\" name (but\nsorted differently due to appending '/' to trees before sorting).\n\nThere is another name_stack_push() that _is_ covered. I wonder\nwhat case would trigger this push?\n\nThanks,\n-Stolee\n"},{"id":"398207","messageId":"20200519183151.46138-1-jonathantanmy@google.com","threadId":"53480","inReplyTo":"ef72aedf-4264-e386-9563-050c54483c93@gmail.com","subject":"[PATCH] t4067: make rename detection test output raw diff","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-19T18:31:51Z","receivedAt":"2020-05-19T18:31:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"95acf11a3d (\"diff: restrict when prefetching occurs\", 2020-04-07) taught\ndiff to prefetch blobs in a more limited set of situations. These\nlimited situations include when the output format requires blob data,\nand when inexact rename detection is needed.\n\nThere is an existing test case that tests inexact rename detection, but\nit also uses an output format that requires blob data, resulting in the\ninexact-rename-detection-only code not being tested. Update this test to\nuse the raw output format, which does not require blob data.\n\nThanks to Derrick Stolee for noticing this lapse in code coverage and\nfor doing the preliminary analysis [1].\n\n[1] https://lore.kernel.org/git/853759d3-97c3-241f-98e1-990883cd204e@gmail.com/\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThanks, Stolee. Yes, we were never in a situation where there are any\nmissing objects at the point of inexact rename detection (despite having\na test exactly for this), but this situation is possible, and I've\nupdated the test so that we encounter this situation.\n---\n t/t4067-diff-partial-clone.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex c1ed1c2fc4..ef8e0e9cb0 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -125,8 +125,8 @@ test_expect_success 'diff with rename detection batches blobs' '\n \n \t# Ensure that there is exactly 1 negotiation by checking that there is\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n-\tgrep \"similarity index\" out &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD >out &&\n+\tgrep \":100644 100644.*R[0-9][0-9][0-9].*b.*c\" out &&\n \tgrep \"git> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n-- \n2.26.2.761.g0e0b3e54be-goog\n\n"},{"id":"398211","messageId":"f15b2d01-2486-57d5-d332-3214ae0609fa@gmail.com","threadId":"53480","inReplyTo":"20200519183151.46138-1-jonathantanmy@google.com","subject":"Re: [PATCH] t4067: make rename detection test output raw diff","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-19T19:00:28Z","receivedAt":"2020-05-19T19:00:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/19/2020 2:31 PM, Jonathan Tan wrote:\n> 95acf11a3d (\"diff: restrict when prefetching occurs\", 2020-04-07) taught\n> diff to prefetch blobs in a more limited set of situations. These\n> limited situations include when the output format requires blob data,\n> and when inexact rename detection is needed.\n> \n> There is an existing test case that tests inexact rename detection, but\n> it also uses an output format that requires blob data, resulting in the\n> inexact-rename-detection-only code not being tested. Update this test to\n> use the raw output format, which does not require blob data.\n> \n> Thanks to Derrick Stolee for noticing this lapse in code coverage and\n> for doing the preliminary analysis [1].\n> \n> [1] https://lore.kernel.org/git/853759d3-97c3-241f-98e1-990883cd204e@gmail.com/\n> \n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Thanks, Stolee. Yes, we were never in a situation where there are any\n> missing objects at the point of inexact rename detection (despite having\n> a test exactly for this), but this situation is possible, and I've\n> updated the test so that we encounter this situation.\n\nThanks for finding the test bug!\n\n> ---\n>  t/t4067-diff-partial-clone.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> index c1ed1c2fc4..ef8e0e9cb0 100755\n> --- a/t/t4067-diff-partial-clone.sh\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -125,8 +125,8 @@ test_expect_success 'diff with rename detection batches blobs' '\n>  \n>  \t# Ensure that there is exactly 1 negotiation by checking that there is\n>  \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n> -\tgrep \"similarity index\" out &&\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD >out &&\n> +\tgrep \":100644 100644.*R[0-9][0-9][0-9].*b.*c\" out &&\n\n--raw definitely does rename detection and I understand the need\nto adjust your grep command here.\n\nThanks,\n-Stolee\n\n\n"},{"id":"398216","messageId":"e0a0fc10-7141-f456-a6db-8d2b8c7dc6b9@web.de","threadId":"53480","inReplyTo":"853759d3-97c3-241f-98e1-990883cd204e@gmail.com","subject":"Re: Git Test Coverage Report (v2.27.0-rc0)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-19T20:07:00Z","receivedAt":"2020-05-19T20:07:31Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.05.20 um 14:11 schrieb Derrick Stolee:\n> On 5/15/2020 1:22 PM, Derrick Stolee wrote:\n>> René Scharfe\t9068cfb2 fsck: report non-consecutive duplicate names in trees\n>> fsck.c\n>> 9068cfb2 623) break;\n>> 9068cfb2 626) if (is_less_than_slash(*p)) {\n>> 9068cfb2 627) name_stack_push(candidates, f_name);\n>> 9068cfb2 628) break;\n>> 9068cfb2 630) }\n>\n> These are different conditions in verify_ordered() to check the\n> special case of blobs and trees having the \"same\" name (but\n> sorted differently due to appending '/' to trees before sorting).\n>\n> There is another name_stack_push() that _is_ covered. I wonder\n> what case would trigger this push?\n\nOur test has one entry between the conflicting two:\n\n\tx\n\tx.1\n\tx/\n\nThe branch is necessary if the check of a candidate tree consumes\na candidate file from the stack that we need to check against later\ncandidate trees, e.g.:\n\n\tx\n\tx.1.2\n\tx.1/\n\tx/\n\nLet's check each in sequence:\n\n  1. \"x\" is a candidate file because the next entry starts with \"x\" +\n     less-than-slash, so \"x/\" can still follow.  We push it onto the\n     stack.\n  2. \"x.1.2\" is not a candidate.  A hypothetical \"x.1.2/\" would come\n     before \"x.1/\", so we can be sure we don't have it.\n  3. \"x.1/\" is a candidate tree because it follows an entry that starts\n     with \"x.1\" + less-than-slash, so we could have seen a file named\n     \"x.1\"  earlier.  We pop \"x\" from the stack and see that it doesn't\n     match, but is a prefix.  \"x.1/\" is \"x\" + less-than-slash, so \"x/\"\n     can still follow.  We push it back onto the stack.  And that's\n     the branch in question.\n  4. \"x/\" is a candidate tree because it follows an entry that starts\n     with \"x\" + less-than-slash, so we could have seen a file named \"x\"\n     earlier.  We pop \"x\" from the stack, and have a match.\n\nBut here's one that slips by fsck and the test coverage check:\n\n\tx\n\tx.1\n\tx.1.2\n\tx/\n\nThis duplicate is not caught by the current code.  Both \"x\" and \"x.1\"\nare candidate files.  \"x.1.2\" is not a candidate.  \"x/\" is checked\nagainst \"x.1\" from the top of the stack, found not to be a prefix and\nthe search ends at that point, unsuccessfully.  I think we need this to\nfix it:\n\ndiff --git a/fsck.c b/fsck.c\nindex 8bb3ecf282..594e800015 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -620,7 +620,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n \t\t\tif (!f_name)\n \t\t\t\tbreak;\n \t\t\tif (!skip_prefix(name2, f_name, &p))\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\tif (!*p)\n \t\t\t\treturn TREE_HAS_DUPS;\n \t\t\tif (is_less_than_slash(*p)) {\n\n"},{"id":"398226","messageId":"20200519234208.GR6362@camp.crustytoothpaste.net","threadId":"53480","inReplyTo":"853759d3-97c3-241f-98e1-990883cd204e@gmail.com","subject":"Re: Git Test Coverage Report (v2.27.0-rc0)","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-19T23:42:08Z","receivedAt":"2020-05-19T23:42:18Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-19 at 12:11:15, Derrick Stolee wrote:\n> On 5/15/2020 1:22 PM, Derrick Stolee wrote:\n> > brian m. carlson\t13e7ed6a builtin/checkout: compute checkout metadata for checkouts\n> > builtin/checkout.c\n> > 13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :\n> \n> This is part of the following hunk:\n> \n> @@ -619,6 +620,11 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n>         opts.verbose_update = o->show_progress;\n>         opts.src_index = &the_index;\n>         opts.dst_index = &the_index;\n> +       init_checkout_metadata(&opts.meta, info->refname,\n> +                              info->commit ? &info->commit->object.oid :\n> +                              is_null_oid(&info->oid) ? &tree->object.oid :\n> +                              &info->oid,\n> +                              NULL);\n>         parse_tree(tree);\n>         init_tree_desc(&tree_desc, tree->buffer, tree->size);\n>         switch (unpack_trees(1, &tree_desc, &opts)) {\n> \n> The double-nested ternary definitely complicates the coverage here.\n> It also points out that all tests have `info->commit` a non-NULL.\n> \n> This certainly looks safe, but I don't know.\n\nThis is me trying to be defensive.  I think the code path that is not\ncovered here that can be is the info->oid code path; I don't technically\nbelieve the other case (using the tree) is possible, although the\ncheckout code is complex enough that I can't be certain.\n\nIt would look like this, I believe:\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 4bfffa9c31..a69ec3c4b7 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -408,6 +408,25 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\tEOF\n \t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n+\t\t# Make sure that the file appears dirty, so checkout below has to\n+\t\t# run the configured filter.\n+\t\tMETA=\"treeish=$MASTER\" &&\n+\t\trm -f test2.r \"testsubdir/test3 '\\''sq'\\'',\\$x=.r\" &&\n+\n+\t\tfilter_git checkout --quiet --no-progress $(git rev-parse master) &&\n+\t\tcat >expected.log <<-EOF &&\n+\t\t\tSTART\n+\t\t\tinit handshake complete\n+\t\t\tIN: smudge test.r $META blob=$M $S [OK] -- OUT: $S . [OK]\n+\t\t\tIN: smudge test2.r $META blob=$M2 $S2 [OK] -- OUT: $S2 . [OK]\n+\t\t\tIN: smudge test4-empty.r $META blob=$EMPTY 0 [OK] -- OUT: 0  [OK]\n+\t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x=.r $META blob=$M3 $S3 [OK] -- OUT: $S3 . [OK]\n+\t\t\tSTOP\n+\t\tEOF\n+\t\ttest_cmp_exclude_clean expected.log debug.log &&\n+\n+\t\tgit checkout empty-branch &&\n+\t\tMETA=\"ref=refs/heads/master treeish=$MASTER\" &&\n \t\tfilter_git checkout --quiet --no-progress master &&\n \t\tcat >expected.log <<-EOF &&\n \t\t\tSTART\n\n\nI'm about to go make dinner, but I'll try to get a patch written up with\nthis tonight or tomorrow.  If it's more urgent than that, you may add my\nsign-off.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"398228","messageId":"20200520013812.GS6362@camp.crustytoothpaste.net","threadId":"53480","inReplyTo":"20200519234208.GR6362@camp.crustytoothpaste.net","subject":"Re: Git Test Coverage Report (v2.27.0-rc0)","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-20T01:38:12Z","receivedAt":"2020-05-20T01:38:21Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-19 at 23:42:08, brian m. carlson wrote:\n> On 2020-05-19 at 12:11:15, Derrick Stolee wrote:\n> > On 5/15/2020 1:22 PM, Derrick Stolee wrote:\n> > > brian m. carlson\t13e7ed6a builtin/checkout: compute checkout metadata for checkouts\n> > > builtin/checkout.c\n> > > 13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :\n> > \n> > This is part of the following hunk:\n> > \n> > @@ -619,6 +620,11 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n> >         opts.verbose_update = o->show_progress;\n> >         opts.src_index = &the_index;\n> >         opts.dst_index = &the_index;\n> > +       init_checkout_metadata(&opts.meta, info->refname,\n> > +                              info->commit ? &info->commit->object.oid :\n> > +                              is_null_oid(&info->oid) ? &tree->object.oid :\n> > +                              &info->oid,\n> > +                              NULL);\n> >         parse_tree(tree);\n> >         init_tree_desc(&tree_desc, tree->buffer, tree->size);\n> >         switch (unpack_trees(1, &tree_desc, &opts)) {\n> > \n> > The double-nested ternary definitely complicates the coverage here.\n> > It also points out that all tests have `info->commit` a non-NULL.\n> > \n> > This certainly looks safe, but I don't know.\n> \n> This is me trying to be defensive.  I think the code path that is not\n> covered here that can be is the info->oid code path; I don't technically\n> believe the other case (using the tree) is possible, although the\n> checkout code is complex enough that I can't be certain.\n\nActually, with more research, it looks like this can just be simplified\nsince info->commit must be non-NULL.  I've analyzed the code and am\nconfident that's guaranteed to be the case, and the test suite has\nconfirmed no segfaults by making that assumption, so I'll send a patch\nimminently that just does that.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"398229","messageId":"20200520014156.1570124-1-sandals@crustytoothpaste.net","threadId":"53480","inReplyTo":"ef72aedf-4264-e386-9563-050c54483c93@gmail.com","subject":"[PATCH 1/1] builtin/checkout: simplify metadata initialization","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-20T01:41:56Z","receivedAt":"2020-05-20T01:42:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When we call init_checkout_metadata in reset_tree, we want to pass the\nobject ID of the commit in question so that it can be passed to filters,\nor if there is no commit, the tree.  We anticipated this latter case,\nwhich can occur elsewhere in the checkout code, but it cannot occur\nhere, since reset_tree is called only (indirectly) via switch_branches,\nwhich requires that we have a valid commit.  switch_branches dies if we\nlack a name and cannot produce a commit from HEAD, and its caller dies\nif we do have a branch name but still lack a commit pointer.\n\nSince we know we must always have a valid commit structure in this case,\nlet's remove the dead code paths and just refer to the commit structure.\nThis simplifies the code and makes it easier for the reader.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n builtin/checkout.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e9d111bb83..e53bdab5b8 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -620,11 +620,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \topts.verbose_update = o->show_progress;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n-\tinit_checkout_metadata(&opts.meta, info->refname,\n-\t\t\t       info->commit ? &info->commit->object.oid :\n-\t\t\t       is_null_oid(&info->oid) ? &tree->object.oid :\n-\t\t\t       &info->oid,\n-\t\t\t       NULL);\n+\tinit_checkout_metadata(&opts.meta, info->refname, &info->commit->object.oid, NULL);\n \tparse_tree(tree);\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n \tswitch (unpack_trees(1, &tree_desc, &opts)) {\n"},{"id":"398261","messageId":"xmqqmu62ws88.fsf@gitster.c.googlers.com","threadId":"53480","inReplyTo":"20200520014156.1570124-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 1/1] builtin/checkout: simplify metadata initialization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-20T15:17:43Z","receivedAt":"2020-05-20T15:17:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> When we call init_checkout_metadata in reset_tree, we want to pass the\n> object ID of the commit in question so that it can be passed to filters,\n> or if there is no commit, the tree.  We anticipated this latter case,\n> which can occur elsewhere in the checkout code, but it cannot occur\n> here, since reset_tree is called only (indirectly) via switch_branches,\n> which requires that we have a valid commit.  switch_branches dies if we\n> lack a name and cannot produce a commit from HEAD, and its caller dies\n> if we do have a branch name but still lack a commit pointer.\n>\n> Since we know we must always have a valid commit structure in this case,\n> let's remove the dead code paths and just refer to the commit structure.\n> This simplifies the code and makes it easier for the reader.\n\nbuiltin/checkout.c::merge_working_tree() has these lines in its\nearlier part:\n\n\tif (opts->new_orphan_branch && opts->orphan_from_empty_tree) {\n\t\tif (new_branch_info->commit)\n\t\t\tBUG(\"'switch --orphan' should never acc...\");\n\t\tnew_tree = parse_tree_indirect(the_hash_algo->empty_tree);\n\t} else\n\t\tnew_tree = get_commit_tree(new_branch_info->commit);\n\tif (opts->discard_changes) {\n\t\tret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);\n\t\tif (ret)\n\t\t\treturn ret;\n\t...\n\nSo, when orphan && orphan-from-empty are both set, we must not have\ncommit, and then if discard is also there, we end up passing\nnew_brnach_info that has NULL in its commit.\n\nThere are few more callers of reset_tree() in this function, which I\ndid not trace.\n\nPerhaps the \"orphan && orphan-from-empty\" is a dead combination and\nwe won't hit the codepath and that is why this change is safe?  I\ndunno.\n\n\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n>  builtin/checkout.c | 6 +-----\n>  1 file changed, 1 insertion(+), 5 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index e9d111bb83..e53bdab5b8 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -620,11 +620,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n>  \topts.verbose_update = o->show_progress;\n>  \topts.src_index = &the_index;\n>  \topts.dst_index = &the_index;\n> -\tinit_checkout_metadata(&opts.meta, info->refname,\n> -\t\t\t       info->commit ? &info->commit->object.oid :\n> -\t\t\t       is_null_oid(&info->oid) ? &tree->object.oid :\n> -\t\t\t       &info->oid,\n> -\t\t\t       NULL);\n> +\tinit_checkout_metadata(&opts.meta, info->refname, &info->commit->object.oid, NULL);\n>  \tparse_tree(tree);\n>  \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n>  \tswitch (unpack_trees(1, &tree_desc, &opts)) {\n"},{"id":"398307","messageId":"20200520223737.GT6362@camp.crustytoothpaste.net","threadId":"53480","inReplyTo":"xmqqmu62ws88.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/1] builtin/checkout: simplify metadata initialization","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-20T22:37:37Z","receivedAt":"2020-05-20T22:38:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-20 at 15:17:43, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > When we call init_checkout_metadata in reset_tree, we want to pass the\n> > object ID of the commit in question so that it can be passed to filters,\n> > or if there is no commit, the tree.  We anticipated this latter case,\n> > which can occur elsewhere in the checkout code, but it cannot occur\n> > here, since reset_tree is called only (indirectly) via switch_branches,\n> > which requires that we have a valid commit.  switch_branches dies if we\n> > lack a name and cannot produce a commit from HEAD, and its caller dies\n> > if we do have a branch name but still lack a commit pointer.\n> >\n> > Since we know we must always have a valid commit structure in this case,\n> > let's remove the dead code paths and just refer to the commit structure.\n> > This simplifies the code and makes it easier for the reader.\n> \n> builtin/checkout.c::merge_working_tree() has these lines in its\n> earlier part:\n> \n> \tif (opts->new_orphan_branch && opts->orphan_from_empty_tree) {\n> \t\tif (new_branch_info->commit)\n> \t\t\tBUG(\"'switch --orphan' should never acc...\");\n> \t\tnew_tree = parse_tree_indirect(the_hash_algo->empty_tree);\n> \t} else\n> \t\tnew_tree = get_commit_tree(new_branch_info->commit);\n> \tif (opts->discard_changes) {\n> \t\tret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \t...\n> \n> So, when orphan && orphan-from-empty are both set, we must not have\n> commit, and then if discard is also there, we end up passing\n> new_brnach_info that has NULL in its commit.\n\nGood point.  I missed that part.\n\n> Perhaps the \"orphan && orphan-from-empty\" is a dead combination and\n> we won't hit the codepath and that is why this change is safe?  I\n> dunno.\n\nIt looks like it's only triggered from git switch with --orphan, not\nwith git checkout.  And furthermore, it seems we require --force or\n--discard-changes to trigger that case, which we don't have anywhere in\nthe testsuite.\n\nI'll come up with a different patch.  I'll probably just set a NULL\nobject ID there, since if we're checking out a new orphan branch, we\nwon't have any files to check out, and therefore there's no possibility\nthat we'll actually use the value for a filter process (since there are\nno files to filter).\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"398319","messageId":"20200521020712.1620993-2-sandals@crustytoothpaste.net","threadId":"53480","inReplyTo":"20200521020712.1620993-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 1/2] builtin/checkout: simplify metadata initialization","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-21T02:07:11Z","receivedAt":"2020-05-21T02:07:23Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When we call init_checkout_metadata in reset_tree, we want to pass the\nobject ID of the commit in question so that it can be passed to filters,\nor if there is no commit, the tree.  We anticipated this latter case,\nwhich can occur elsewhere in the checkout code, but it cannot occur\nhere.  The only case in which we do not have a commit object is when\ninvoking git switch with --orphan.  Moreover, we can only hit this code\npath without a commit object additionally with either --force or\n--discard-changes.\n\nIn such a case, there is no point initializing the checkout metadata\nwith a commit or tree because (a) there is no commit, only the empty\ntree, and (b) we will never use the data, since no files will be smudged\nwhen checking out a branch with no files.  Pass the all-zeros object ID\nin this case, since we just need some value which is a valid pointer.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n builtin/checkout.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e9d111bb83..62b5e371bc 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -621,9 +621,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n \tinit_checkout_metadata(&opts.meta, info->refname,\n-\t\t\t       info->commit ? &info->commit->object.oid :\n-\t\t\t       is_null_oid(&info->oid) ? &tree->object.oid :\n-\t\t\t       &info->oid,\n+\t\t\t       info->commit ? &info->commit->object.oid : &null_oid,\n \t\t\t       NULL);\n \tparse_tree(tree);\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n"},{"id":"398318","messageId":"20200521020712.1620993-3-sandals@crustytoothpaste.net","threadId":"53480","inReplyTo":"20200521020712.1620993-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 2/2] t2060: add a test for switch with --orphan and --discard-changes","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-21T02:07:12Z","receivedAt":"2020-05-21T02:07:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We have several code paths in the checkout code which are traversed only\nin this case, due to switch having different defaults from checkout.\nLet's add a test that the combination of options works and produces the\nexpected behavior.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n t/t2060-switch.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\nindex f9efa29dfb..2c1b8c0d6d 100755\n--- a/t/t2060-switch.sh\n+++ b/t/t2060-switch.sh\n@@ -68,6 +68,14 @@ test_expect_success 'new orphan branch from empty' '\n \ttest_cmp expected tracked-files\n '\n \n+test_expect_success 'orphan branch works with --discard-changes' '\n+\ttest_when_finished git switch master &&\n+\techo foo >foo.txt &&\n+\tgit switch --discard-changes --orphan new-orphan2 &&\n+\tgit ls-files >tracked-files &&\n+\ttest_must_be_empty tracked-files\n+'\n+\n test_expect_success 'switching ignores file of same branch name' '\n \ttest_when_finished git switch master &&\n \t: >first-branch &&\n"},{"id":"398320","messageId":"20200521020712.1620993-1-sandals@crustytoothpaste.net","threadId":"53480","inReplyTo":"ef72aedf-4264-e386-9563-050c54483c93@gmail.com","subject":"[PATCH v2 0/2] Improve Fix code coverage for checkout","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-21T02:07:10Z","receivedAt":"2020-05-21T02:07:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Patch #1 reduces the number of options in the scenario which Stolee\nmentioned above.  There's now just a NULL and a non-NULL case, and the\nNULL case is now relatively straightforward and uninteresting.\n\nPatch #2 adds a test for the particular set of options which will\ntrigger this case as an independent test.  I didn't think it made sense\nto put this in t0021, since ultimately that set of options isn't about\nconversions and it would seem out of place there, so I put it in t2060.\n\nI'm ultimately on the fence for this case, because I think it's really a\ncorner case and testing this is probably not that interesting, so my\npreference is for us to pick up patch 1 and drop patch 2.  However, I\nadded patch 2 in case we do indeed want a test for this, and I'll let\nJunio and others decide on what's best.\n\nbrian m. carlson (2):\n  builtin/checkout: simplify metadata initialization\n  t2060: add a test for switch with --orphan and --discard-changes\n\n builtin/checkout.c | 4 +---\n t/t2060-switch.sh  | 8 ++++++++\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\n"},{"id":"398355","messageId":"d5845a16-df61-0497-5af9-75d0174a0fc1@gmail.com","threadId":"53480","inReplyTo":"20200521020712.1620993-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 0/2] Improve Fix code coverage for checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-21T12:38:23Z","receivedAt":"2020-05-21T12:38:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/20/2020 10:07 PM, brian m. carlson wrote:\n> Patch #1 reduces the number of options in the scenario which Stolee\n> mentioned above.  There's now just a NULL and a non-NULL case, and the\n> NULL case is now relatively straightforward and uninteresting.\n\nI'm glad you and Junio were able to simplify this case!\n\n> Patch #2 adds a test for the particular set of options which will\n> trigger this case as an independent test.  I didn't think it made sense\n> to put this in t0021, since ultimately that set of options isn't about\n> conversions and it would seem out of place there, so I put it in t2060.\n> \n> I'm ultimately on the fence for this case, because I think it's really a\n> corner case and testing this is probably not that interesting, so my\n> preference is for us to pick up patch 1 and drop patch 2.  However, I\n> added patch 2 in case we do indeed want a test for this, and I'll let\n> Junio and others decide on what's best.\n\nI think the test is helpful, since it is not very complicated to set up.\n\nThe test you created for t0021-conversion.sh earlier was quite complicated\nand required internal knowledge to get going. However, this test only uses\nporcelain commands to trigger the conditional.\n\nFrom the discussion here, it is not obvious that info->commit is ever NULL,\nso having the test can prevent a future change from making that same\nassumption.\n\nThis series is Reviewed-by: Derrick Stolee <dstolee@microsoft.com>\n\nThanks,\n-Stolee\n"},{"id":"398368","messageId":"xmqq1rndtcmd.fsf@gitster.c.googlers.com","threadId":"53480","inReplyTo":"20200521020712.1620993-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] builtin/checkout: simplify metadata initialization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-21T17:35:22Z","receivedAt":"2020-05-21T17:35:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> ...  The only case in which we do not have a commit object is when\n> invoking git switch with --orphan.  Moreover, we can only hit this code\n> path without a commit object additionally with either --force or\n> --discard-changes.\n\nIt was easy for me to trace the codepath to see when these options\nare used we end up with no commit object, but I ran out of time\ntrying to see if the \"forced orphan\" is the only way to end up with\na NULL in new_branch_info->commit.  Assuming that is true, of course\nthe following perfectly makes sense.\n\n> In such a case, there is no point initializing the checkout metadata\n> with a commit or tree because (a) there is no commit, only the empty\n> tree, and (b) we will never use the data, since no files will be smudged\n> when checking out a branch with no files.  Pass the all-zeros object ID\n> in this case, since we just need some value which is a valid pointer.\n>\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n>  builtin/checkout.c | 4 +---\n>  1 file changed, 1 insertion(+), 3 deletions(-)\n\nThanks.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index e9d111bb83..62b5e371bc 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -621,9 +621,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n>  \topts.src_index = &the_index;\n>  \topts.dst_index = &the_index;\n>  \tinit_checkout_metadata(&opts.meta, info->refname,\n> -\t\t\t       info->commit ? &info->commit->object.oid :\n> -\t\t\t       is_null_oid(&info->oid) ? &tree->object.oid :\n> -\t\t\t       &info->oid,\n> +\t\t\t       info->commit ? &info->commit->object.oid : &null_oid,\n>  \t\t\t       NULL);\n>  \tparse_tree(tree);\n>  \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n"},{"id":"398451","messageId":"20200523122205.GA1915090@camp.crustytoothpaste.net","threadId":"53480","inReplyTo":"xmqq1rndtcmd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/2] builtin/checkout: simplify metadata initialization","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-23T12:22:05Z","receivedAt":"2020-05-23T12:22:16Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-21 at 17:35:22, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > ...  The only case in which we do not have a commit object is when\n> > invoking git switch with --orphan.  Moreover, we can only hit this code\n> > path without a commit object additionally with either --force or\n> > --discard-changes.\n> \n> It was easy for me to trace the codepath to see when these options\n> are used we end up with no commit object, but I ran out of time\n> trying to see if the \"forced orphan\" is the only way to end up with\n> a NULL in new_branch_info->commit.  Assuming that is true, of course\n> the following perfectly makes sense.\n\nI believe it is.  The only case in which we have a NULL commit as far as\nI can tell is with switch and an orphan, and in merge_working_tree we\ncall reset_tree either if the changes are discarded or unpack_trees\ncouldn't do a trivial merge.  Since I'm pretty sure unpack_trees can\nindeed merge with the empty tree, we would only call reset_trees with\n--discard-changes or --force.  And reset_tree is only called from\nmerge_working_tree.\n\nIn addition, I did try other situations plus the entire testsuite with\nmy erroneous first patch and was unable to cause a segfault anywhere\n(which would have been a trivial NULL dereference) in case I missed\nsomething, which leads me to believe that this is in fact the only\nsituation in which this occurs.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}