Git Test Coverage Report (v2.27.0-rc0)

Git Test Coverage Report (v2.27.0-rc0)

[Derrick Stolee]

Here is the test coverage report for the release candidate
v2.27.0-rc0. Please take close look at this report for code
that has been edited since the last release (v2.26.2) but is
not covered by the test suite. Bugs tend to hide in uncovered
code!

The full report also includes changes in 'next' and 'pu' [1].

Thanks,
-Stolee

[1] https://derrickstolee.github.io/git-test-coverage/reports/v2.27.0-rc0-commits.txt

---

Uncovered code in 'v2.27.0-rc0' not in 'v2.26.2'
--------------------------------------------------------

Commits introducing uncovered code:
Ævar Arnfjörð Bjarmason	5cc044e0 get_short_oid: sort ambiguous objects by type, then SHA-1
oid-array.c
5cc044e0 56) return ret;

Alexandr Miloslavskiy	49d3c4b4 get_superproject_working_tree(): return strbuf
submodule.c
49d3c4b4 2188) return 0;
49d3c4b4 2191) return 0;
49d3c4b4 2245) return 0;

Alexandr Miloslavskiy	3d7747e3 real_path: remove unsafe API
editor.c
3d7747e3 87) strbuf_release(&realpath);

setup.c
3d7747e3 78) strbuf_release(&realpath);

brian m. carlson	8b8f7189 builtin/init-db: allow specifying hash algorithm on command line
builtin/init-db.c
8b8f7189 376) repo_fmt->hash_algo = hash;
8b8f7189 599) hash_algo = hash_algo_by_name(object_format);
8b8f7189 600) if (hash_algo == GIT_HASH_UNKNOWN)

brian m. carlson	e02a7141 worktree: allow repository version 1
worktree.c
e02a7141 476) strbuf_release(&err);

brian m. carlson	edc6dccf builtin/receive-pack: use constant-time comparison for HMAC value
builtin/receive-pack.c
edc6dccf 569) retval = NONCE_BAD;
edc6dccf 570) goto leave;

brian m. carlson	ab90ecae convert: permit passing additional metadata to filter processes
convert.c
ab90ecae 862) goto done;
ab90ecae 868) goto done;
ab90ecae 874) goto done;

brian m. carlson	768e30ea hash: implement and use a context cloning function
sha1-file.c
768e30ea 98) static void git_hash_sha256_clone(git_hash_ctx *dst, const git_hash_ctx *src)
768e30ea 100) git_SHA256_Clone(&dst->sha256, &src->sha256);
768e30ea 101) }
768e30ea 118) static void git_hash_unknown_clone(git_hash_ctx *dst, const git_hash_ctx *src)

brian m. carlson	3c9331a1 builtin/init-db: add environment variable for new repo hash
builtin/init-db.c
3c9331a1 378) int env_algo = hash_algo_by_name(env);
3c9331a1 379) if (env_algo == GIT_HASH_UNKNOWN)
3c9331a1 381) repo_fmt->hash_algo = env_algo;

brian m. carlson	910650d2 Rename sha1_array to oid_array
t/helper/test-oid-array.c
910650d2 29) oid_array_clear(&array);

brian m. carlson	ddddf8d7 fast-import: permit reading multiple marks files
fast-import.c
ddddf8d7 1158) insert_mark(marks, mark, e);

brian m. carlson	1bdca816 fast-import: add options for rewriting submodules
fast-import.c
1bdca816 2192) return -1;
1bdca816 3067) return;
1bdca816 3325) die_errno("cannot read '%s'", f);

brian m. carlson	13e7ed6a builtin/checkout: compute checkout metadata for checkouts
builtin/checkout.c
13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :

brian m. carlson	efa7ae36 init-db: move writing repo version into a function
builtin/init-db.c
efa7ae36 192) repo_version = GIT_REPO_VERSION_READ;
efa7ae36 200) git_config_set("extensions.objectformat",

Denton Liu	65c425a2 sequencer: stop leaking buf
sequencer.c
65c425a2 2557) goto done_rebase_i;

Denton Liu	be1bb600 sequencer: make apply_autostash() accept a path
sequencer.c
be1bb600 5203) apply_autostash(rebase_path_autostash());

Denton Liu	ce6521e4 Lib-ify fmt-merge-msg
fmt-merge-msg.c
ce6521e4 24) use_branch_desc = git_config_bool(key, value);
ce6521e4 75) continue;
ce6521e4 107) return 1;
ce6521e4 113) return 2;
ce6521e4 117) return 3;
ce6521e4 126) line[len - 1] = 0;
ce6521e4 167) origin = src;
ce6521e4 168) string_list_append(&src_data->generic, line);
ce6521e4 169) src_data->head_status |= 2;
ce6521e4 188) return;
ce6521e4 201) static void add_branch_desc(struct strbuf *out, const char *name)
ce6521e4 203) struct strbuf desc = STRBUF_INIT;
ce6521e4 205) if (!read_branch_desc(&desc, name)) {
ce6521e4 206) const char *bp = desc.buf;
ce6521e4 207) while (*bp) {
ce6521e4 208) const char *ep = strchrnul(bp, '\n');
ce6521e4 209) if (*ep)
ce6521e4 210) ep++;
ce6521e4 211) strbuf_addf(out, "  : %.*s", (int)(ep - bp), bp);
ce6521e4 212) bp = ep;
ce6521e4 214) strbuf_complete_line(out);
ce6521e4 216) strbuf_release(&desc);
ce6521e4 217) }
ce6521e4 231) return;
ce6521e4 239) return;
ce6521e4 276) else if (people->nr)
ce6521e4 277) strbuf_addf(out, "%s (%d) and others",
ce6521e4 278)     people->items[0].string,
ce6521e4 279)     (int)util_as_integral(&people->items[0]));
ce6521e4 342) return;
ce6521e4 355) if (opts->credit_people)
ce6521e4 356) record_person('c', &committers, commit);
ce6521e4 372) string_list_append(&subjects,
ce6521e4 373)    oid_to_hex(&commit->object.oid));
ce6521e4 387) add_branch_desc(out, name);
ce6521e4 425) subsep = ", ";
ce6521e4 426) strbuf_addstr(out, "HEAD");
ce6521e4 446) strbuf_addstr(out, subsep);
ce6521e4 447) print_joined("commit ", "commits ", &src_data->generic,
ce6521e4 507) if (tag_number == 2) {
ce6521e4 508) struct strbuf tagline = STRBUF_INIT;
ce6521e4 509) strbuf_addch(&tagline, '\n');
ce6521e4 510) strbuf_add_commented_lines(&tagline,
ce6521e4 511) origins.items[first_tag].string,
ce6521e4 512) strlen(origins.items[first_tag].string));
ce6521e4 513) strbuf_insert(&tagbuf, 0, tagline.buf,
ce6521e4 515) strbuf_release(&tagline);
ce6521e4 517) strbuf_addch(&tagbuf, '\n');
ce6521e4 518) strbuf_add_commented_lines(&tagbuf,
ce6521e4 519) origins.items[i].string,
ce6521e4 520) strlen(origins.items[i].string));
ce6521e4 521) fmt_tag_signature(&tagbuf, &sig, buf, len);
ce6521e4 566) continue;

Denton Liu	5b2f6d9c sequencer: make file exists check more efficient
sequencer.c
5b2f6d9c 431) warning_errno(_("could not read '%s'"), path);

Denton Liu	b309a971 reset: extract reset_head() from rebase
reset.c
b309a971 37) ret = -1;
b309a971 38) goto leave_reset_head;
b309a971 43) goto leave_reset_head;
b309a971 66) goto leave_reset_head;
b309a971 72) goto leave_reset_head;
b309a971 77) goto leave_reset_head;
b309a971 81) ret = -1;
b309a971 82) goto leave_reset_head;
b309a971 90) goto leave_reset_head;
b309a971 109) } else if (old_orig)
b309a971 110) delete_ref(NULL, "ORIG_HEAD", old_orig, 0);

Denton Liu	9460fd48 Lib-ify prune-packed
prune-packed.c
9460fd48 26) printf("rm -f %s\n", path);
9460fd48 35) progress = start_delayed_progress(_("Removing duplicate objects"), 256);

Denton Liu	efcf6cf0 rebase: use read_oneliner()
builtin/rebase.c
efcf6cf0 625) } else if (!read_oneliner(&buf, state_dir_path("head", opts),

Denton Liu	7cd54d37 wrapper: indent with tabs
wrapper.c
7cd54d37 221) len = MAX_IO_SIZE;
7cd54d37 243) len = MAX_IO_SIZE;

Denton Liu	86ed00af rebase: use apply_autostash() from sequencer.c
builtin/rebase.c
86ed00af 1040) apply_autostash(state_dir_path("autostash", opts));

Denton Liu	f213f069 rebase: generify reset_head()
builtin/rebase.c
f213f069 879) reset_head(the_repository, &opts->orig_head, "checkout",
f213f069 880)    opts->head_name, 0,

Derrick Stolee	0906ac2b blame: use changed-path Bloom filters
blame.c
0906ac2b 1281) return 1;
0906ac2b 1302) bd->alloc *= 2;
0906ac2b 1303) REALLOC_ARRAY(bd->keys, bd->alloc);
0906ac2b 2899) return;

Derrick Stolee	3ce4ca0a multi-pack-index: respect repack.packKeptObjects=false
midx.c
3ce4ca0a 1307) continue;

Derrick Stolee	c9f7a793 log-tree: make ref_filter_match() a helper method
log-tree.c
c9f7a793 94)     (!*rest || *rest == '/'))

Derrick Stolee	8918e379 revision: complicated pathspecs disable filters
revision.c
8918e379 658) return 1;
8918e379 662) return 1;

Derrick Stolee	6c622f9f commit-graph: write commit-graph chains
commit-graph.c
6c622f9f 1586) return -1;

Derrick Stolee	e3696980 diff: halt tree-diff early after max_changes
tree-diff.c
e3696980 438) break;

Đoàn Trần Công Danh	c933b28d date.c: s/is_date/set_date/
date.c
c933b28d 521) return -1;
c933b28d 532) return -1;

Đoàn Trần Công Danh	4f89f4fc date.c: validate and set time in a helper function
date.c
4f89f4fc 553) return -1;

Elijah Newren	30e89c12 unpack-trees: pull sparse-checkout pattern reading into a new function
unpack-trees.c
30e89c12 1556) o->skip_sparse_checkout = 1;

Elijah Newren	16846444 dir: include DIR_KEEP_UNTRACKED_CONTENTS handling in treat_directory()
dir.c
16846444 1924) FREE_AND_NULL(dir->entries[i]);

Elijah Newren	7af7a258 unpack-trees: add a new update_sparsity() function
unpack-trees.c
7af7a258 1788) goto skip_sparse_checkout;
7af7a258 1815) ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;

Elijah Newren	b9cbd295 rebase: reinstate --no-keep-empty
sequencer.c
b9cbd295 4756) continue;

Emily Shaffer	709df95b help: move list_config_help to builtin/help
builtin/help.c
709df95b 123) puts(var);
709df95b 124) continue;

Emily Shaffer	1411914a bugreport: add uname info
bugreport.c
1411914a 20) strbuf_addf(sys_info, _("uname() failed with error '%s' (%d)\n"),
1411914a 22)     errno);

Emily Shaffer	617d5719 bugreport: gather git version and build info
help.c
617d5719 641) strbuf_addstr(buf, "no commit associated with this build\n");

Garima Singh	a56b9464 revision.c: use Bloom filters to speed up path based revision walks
bloom.c
a56b9464 286) return -1;

revision.c
a56b9464 721) return -1;

Garima Singh	3d112755 commit-graph: examine commits by generation number
commit-graph.c
3d112755 1307) QSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);

Garima Singh	ed591feb bloom.c: core Bloom filter implementation for changed paths.
bloom.c
ed591feb 185) return NULL;
ed591feb 266) for (i = 0; i < diff_queued_diff.nr; i++)
ed591feb 267) diff_free_filepair(diff_queued_diff.queue[i]);
ed591feb 268) filter->data = NULL;
ed591feb 269) filter->len = 0;

Garima Singh	76ffbca7 commit-graph: write Bloom filters to commit graph file
commit-graph.c
76ffbca7 337) chunk_repeated = 1;
76ffbca7 344) chunk_repeated = 1;
76ffbca7 351) break;
76ffbca7 1095) progress = start_delayed_progress(
76ffbca7 1097) ctx->commits.nr);
76ffbca7 1120) progress = start_delayed_progress(
76ffbca7 1122) ctx->commits.nr);

Garima Singh	f1294eaf bloom.c: introduce core Bloom filter constructs
t/helper/test-bloom.c
f1294eaf 25) printf("No filter.\n");
f1294eaf 26) return;

Garima Singh	f97b9325 commit-graph: compute Bloom filters for changed paths
commit-graph.c
f97b9325 1299) progress = start_delayed_progress(
f97b9325 1301) ctx->commits.nr);

Hans Jerry Illikainen	67948981 gpg-interface: prefer check_signature() for GPG verification
gpg-interface.c
67948981 275) error_errno(_("failed writing detached signature to '%s'"),
67948981 277) delete_tempfile(&temp);
67948981 278) return -1;
67948981 293) gpg_status = &buf;

log-tree.c
67948981 512) show_sig_lines(opt, status, "No signature\n");

Heba Waly	b3b18d16 advice: revamp advise API
advice.c
b3b18d16 178) return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
b3b18d16 179)        advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;

Jeff King	d21ee7d1 commit-graph: examine changed-path objects in pack order
commit-graph.c
d21ee7d1 77) return; /* should never happen, but be lenient */
d21ee7d1 82) static int commit_pos_cmp(const void *va, const void *vb)
d21ee7d1 84) const struct commit *a = *(const struct commit **)va;
d21ee7d1 85) const struct commit *b = *(const struct commit **)vb;
d21ee7d1 86) return commit_pos_at(&commit_pos, a) -
d21ee7d1 87)        commit_pos_at(&commit_pos, b);

Jeff King	16ddcd40 sha1_array: let callbacks interrupt iteration
oid-array.c
16ddcd40 76) return ret;

Jeff King	1b4c57fa test-bloom: check that we have expected arguments
t/helper/test-bloom.c
1b4c57fa 54) usage(bloom_usage);
1b4c57fa 59) usage(bloom_usage);
1b4c57fa 71) usage(bloom_usage);
1b4c57fa 85) usage(bloom_usage);

Jeff King	d8410a81 fast-import: replace custom hash with hashmap.c
fast-import.c
d8410a81 60) e2 = container_of(entry_or_key, const struct object_entry, ent);
d8410a81 61) return oidcmp(&e1->idx.oid, &e2->idx.oid);

Jeff King	348482de config: reject parsing of files over INT_MAX
config.c
348482de 537) cf->eof = 1;
348482de 538) return 0;

Johannes Schindelin	12294990 credential: handle `credential.<partial-URL>.<key>` again
urlmatch.c
12294990 581) retval = 0;

Jonathan Tan	fbda77c6 commit-graph: avoid Memory Leaks
commit-graph.c
fbda77c6 296) goto free_and_return;
fbda77c6 363) goto free_and_return;

Jonathan Tan	95acf11a diff: restrict when prefetching occurs
diffcore-rename.c
95acf11a 137) static void prefetch(void *prefetch_options)
95acf11a 139) struct prefetch_options *options = prefetch_options;
95acf11a 141) struct oid_array to_fetch = OID_ARRAY_INIT;
95acf11a 143) for (i = 0; i < rename_dst_nr; i++) {
95acf11a 144) if (rename_dst[i].pair)
95acf11a 149) continue; /* already found exact match */
95acf11a 150) diff_add_if_missing(options->repo, &to_fetch,
95acf11a 151)     rename_dst[i].two);
95acf11a 153) for (i = 0; i < rename_src_nr; i++) {
95acf11a 154) if (options->skip_unmodified &&
95acf11a 155)     diff_unmodified_pair(rename_src[i].p))
95acf11a 160) continue;
95acf11a 161) diff_add_if_missing(options->repo, &to_fetch,
95acf11a 162)     rename_src[i].p->one);
95acf11a 164) promisor_remote_get_direct(options->repo, to_fetch.oid, to_fetch.nr);
95acf11a 165) oid_array_clear(&to_fetch);
95acf11a 166) }

Jorge Lopez Silva	af026519 http: add environment variable support for HTTPS proxies
http.c
af026519 1221) proxy_ssl_cert_password_required = 1;

Jorge Lopez Silva	88238e02 http: add client cert support for HTTPS proxies
http.c
88238e02 376) return git_config_string(&http_proxy_ssl_cert, var, value);
88238e02 379) return git_config_string(&http_proxy_ssl_key, var, value);
88238e02 382) return git_config_string(&http_proxy_ssl_ca_info, var, value);
88238e02 385) proxy_ssl_cert_password_required = git_config_bool(var, value);
88238e02 386) return 0;
88238e02 966) if (ssl_cainfo != NULL)
88238e02 967) curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
88238e02 1294) memset(proxy_cert_auth.password, 0, strlen(proxy_cert_auth.password));
88238e02 1295) FREE_AND_NULL(proxy_cert_auth.password);

Josh Steadmon	3d3adaad trace2: teach Git to log environment variables
trace2/tr2_cfg.c
3d3adaad 63) return tr2_cfg_env_vars_count;

Junio C Hamano	56a1d9ca Merge branch 'dl/libify-a-few'
fmt-merge-msg.c
56a1d9ca 498) strbuf_addstr(&sig, "gpg verification failed.\n");

Karsten Blees	defd7c7b dir.c: git-status --ignored: don't scan the work tree three times
dir.c
defd7c7b 2177) return path_none;

Patrick Steinhardt	edc30691 refs: fix segfault when aborting empty transaction
refs/files-backend.c
edc30691 2572) strbuf_release(&err);

Patrick Steinhardt	a65b8ac2 update-ref: organize commands in an array
builtin/update-ref.c
a65b8ac2 419) continue;

Phillip Wood	430b75f7 commit: give correct advice for empty commit during a rebase
sequencer.c
430b75f7 1469)     return -1;

René Scharfe	9068cfb2 fsck: report non-consecutive duplicate names in trees
fsck.c
9068cfb2 623) break;
9068cfb2 626) if (is_less_than_slash(*p)) {
9068cfb2 627) name_stack_push(candidates, f_name);
9068cfb2 628) break;
9068cfb2 630) }

Son Luong Ngoc	e11d86de midx: teach "git multi-pack-index repack" honor "git repack" configurations
midx.c
e11d86de 1420) argv_array_push(&cmd.args, "--delta-islands");

Taylor Blau	37b9dcab shallow.c: use '{commit,rollback}_shallow_file'
builtin/receive-pack.c
37b9dcab 897) rollback_shallow_file(the_repository, &shallow_lock);

Taylor Blau	fdbde82f builtin/commit-graph.c: introduce split strategy 'no-merge'
commit-graph.c
fdbde82f 1789) break;

Vasil Dimov	8cf51561 range-diff: fix a crash in parsing git-log output
range-diff.c
8cf51561 115) string_list_clear(list, 1);
8cf51561 116) strbuf_release(&buf);
8cf51561 117) strbuf_release(&contents);
8cf51561 118) finish_command(&cp);
8cf51561 119) return -1;

Re: Git Test Coverage Report (v2.27.0-rc0)

[Derrick Stolee]

On 5/15/2020 1:22 PM, Derrick Stolee wrote:
> Bugs tend to hide in uncovered
> code!

Immediately after sending the report, I regretted the ambiguity of
this sentence. Here are a couple things to point out:

* Just because code is uncovered does not mean it is more likely
  to contain a bug. It just means that code review is our only
  protection. This is completely acceptable for things like error
  conditions we never expect to show up. We also don't hold authors
  accountable for existing bugs when they do a mechanical refactor
  of existing, uncovered code.

* Just because code is _covered_ does not mean it is less likely
  to contain a bug. There are all sorts of reasons that coverage
  is not enough to "guarantee" correct behavior.

My process in this email is to check each commit's diff and
eliminate any lines that are obviously part of error conditions
(and there isn't a worry about a use-after-free for an error
message or something).

I've CC'd the authors for the commits that I think could use
a second look. Please evaluate the following:

 1. Is this code definitely correct?
 2. How difficult would it be to cover this with a test?

> brian m. carlson	13e7ed6a builtin/checkout: compute checkout metadata for checkouts
> builtin/checkout.c
> 13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :

This is part of the following hunk:

@@ -619,6 +620,11 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
        opts.verbose_update = o->show_progress;
        opts.src_index = &the_index;
        opts.dst_index = &the_index;
+       init_checkout_metadata(&opts.meta, info->refname,
+                              info->commit ? &info->commit->object.oid :
+                              is_null_oid(&info->oid) ? &tree->object.oid :
+                              &info->oid,
+                              NULL);
        parse_tree(tree);
        init_tree_desc(&tree_desc, tree->buffer, tree->size);
        switch (unpack_trees(1, &tree_desc, &opts)) {

The double-nested ternary definitely complicates the coverage here.
It also points out that all tests have `info->commit` a non-NULL.

This certainly looks safe, but I don't know.

> Denton Liu	be1bb600 sequencer: make apply_autostash() accept a path
> sequencer.c
> be1bb600 5203) apply_autostash(rebase_path_autostash());

This line is identical to four other lines inserted by the
same patch.

> Denton Liu	ce6521e4 Lib-ify fmt-merge-msg
> fmt-merge-msg.c

(This patch moves a bunch of old code, so we should not
fault the author for its coverage.)

> Derrick Stolee	0906ac2b blame: use changed-path Bloom filters
> blame.c
> 0906ac2b 1302) bd->alloc *= 2;
> 0906ac2b 1303) REALLOC_ARRAY(bd->keys, bd->alloc);
> 0906ac2b 2899) return;

This hunk is part of this method:

+static void add_bloom_key(struct blame_bloom_data *bd,
+                         const char *path)
+{
+       if (!bd)
+               return;
+
+       if (bd->nr >= bd->alloc) {
+               bd->alloc *= 2;
+               REALLOC_ARRAY(bd->keys, bd->alloc);
+       }
+
+       bd->keys[bd->nr] = xmalloc(sizeof(struct bloom_key));
+       fill_bloom_key(path, strlen(path), bd->keys[bd->nr], bd->settings);
+       bd->nr++;
+}

This is not being covered because the blame tests must not
be splitting the files across enough renames to trigger the
realloc. I don't think it is worth creating such a complicated
test.

> Derrick Stolee	c9f7a793 log-tree: make ref_filter_match() a helper method
> log-tree.c
> c9f7a793 94)     (!*rest || *rest == '/'))

This is moved code:

+               const char *rest;
+               if (skip_prefix(refname, item->string, &rest) &&
+                   (!*rest || *rest == '/'))
+                       matched = 1;

Since the "matched = 1;" line is covered, this must mean that
any tests we use have "!*rest" be true and the "*rest == '/'"
is not covered. 

> Elijah Newren	30e89c12 unpack-trees: pull sparse-checkout pattern reading into a new function
> unpack-trees.c
> 30e89c12 1556) o->skip_sparse_checkout = 1;

Error condition (when sparse-checkout fails to parse).

> Garima Singh	3d112755 commit-graph: examine commits by generation number
> commit-graph.c
> 3d112755 1307) QSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);

While this is blamed to 3d112755, it's actually moved from d21ee7d11 (commit-graph:
examine changed-path objects in pack order, 2020-03-30) which introduced the sort.

The reason this isn't covered is because all changed-path tests use --reachable
and none use the default which scans pack-files.

> Garima Singh	ed591feb bloom.c: core Bloom filter implementation for changed paths.
> bloom.c
> ed591feb 266) for (i = 0; i < diff_queued_diff.nr; i++)
> ed591feb 267) diff_free_filepair(diff_queued_diff.queue[i]);
> ed591feb 268) filter->data = NULL;
> ed591feb 269) filter->len = 0;

This is the case when a commit has more than 512 changes. The plan is to
add a test after this 512 limit is adjustable by config. (Adding to my TODO
list [1])

[1] https://github.com/microsoft/git/issues/272

> Jeff King	d21ee7d1 commit-graph: examine changed-path objects in pack order
> commit-graph.c
> d21ee7d1 77) return; /* should never happen, but be lenient */
> d21ee7d1 82) static int commit_pos_cmp(const void *va, const void *vb)
> d21ee7d1 84) const struct commit *a = *(const struct commit **)va;
> d21ee7d1 85) const struct commit *b = *(const struct commit **)vb;
> d21ee7d1 86) return commit_pos_at(&commit_pos, a) -
> d21ee7d1 87)        commit_pos_at(&commit_pos, b);

These would be called by the QSORT mentioned earlier.

> Jonathan Tan	95acf11a diff: restrict when prefetching occurs
> diffcore-rename.c
> 95acf11a 137) static void prefetch(void *prefetch_options)
> 95acf11a 139) struct prefetch_options *options = prefetch_options;
> 95acf11a 141) struct oid_array to_fetch = OID_ARRAY_INIT;
> 95acf11a 143) for (i = 0; i < rename_dst_nr; i++) {
> 95acf11a 144) if (rename_dst[i].pair)
> 95acf11a 149) continue; /* already found exact match */
> 95acf11a 150) diff_add_if_missing(options->repo, &to_fetch,
> 95acf11a 151)     rename_dst[i].two);
> 95acf11a 153) for (i = 0; i < rename_src_nr; i++) {
> 95acf11a 154) if (options->skip_unmodified &&
> 95acf11a 155)     diff_unmodified_pair(rename_src[i].p))
> 95acf11a 160) continue;
> 95acf11a 161) diff_add_if_missing(options->repo, &to_fetch,
> 95acf11a 162)     rename_src[i].p->one);
> 95acf11a 164) promisor_remote_get_direct(options->repo, to_fetch.oid, to_fetch.nr);
> 95acf11a 165) oid_array_clear(&to_fetch);
> 95acf11a 166) }

Odd that this static method is never called, even once.

The only reference is here:

+       struct prefetch_options prefetch_options = {r, skip_unmodified};
+
+       if (r == the_repository && has_promisor_remote()) {
+               dpf_options.missing_object_cb = prefetch;
+               dpf_options.missing_object_data = &prefetch_options;
+       }

but this block is definitely being called. Perhaps the prefetch
is never being called because we are never in a situation where there
are any missing objects?

> René Scharfe	9068cfb2 fsck: report non-consecutive duplicate names in trees
> fsck.c
> 9068cfb2 623) break;
> 9068cfb2 626) if (is_less_than_slash(*p)) {
> 9068cfb2 627) name_stack_push(candidates, f_name);
> 9068cfb2 628) break;
> 9068cfb2 630) }
 
These are different conditions in verify_ordered() to check the
special case of blobs and trees having the "same" name (but
sorted differently due to appending '/' to trees before sorting).

There is another name_stack_push() that _is_ covered. I wonder
what case would trigger this push?

Thanks,
-Stolee

[PATCH] t4067: make rename detection test output raw diff

[Jonathan Tan]

95acf11a3d ("diff: restrict when prefetching occurs", 2020-04-07) taught
diff to prefetch blobs in a more limited set of situations. These
limited situations include when the output format requires blob data,
and when inexact rename detection is needed.

There is an existing test case that tests inexact rename detection, but
it also uses an output format that requires blob data, resulting in the
inexact-rename-detection-only code not being tested. Update this test to
use the raw output format, which does not require blob data.

Thanks to Derrick Stolee for noticing this lapse in code coverage and
for doing the preliminary analysis [1].

[1] https://lore.kernel.org/git/853759d3-97c3-241f-98e1-990883cd204e@gmail.com/

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks, Stolee. Yes, we were never in a situation where there are any
missing objects at the point of inexact rename detection (despite having
a test exactly for this), but this situation is possible, and I've
updated the test so that we encounter this situation.
---
 t/t4067-diff-partial-clone.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh
index c1ed1c2fc4..ef8e0e9cb0 100755
--- a/t/t4067-diff-partial-clone.sh
+++ b/t/t4067-diff-partial-clone.sh
@@ -125,8 +125,8 @@ test_expect_success 'diff with rename detection batches blobs' '
 
 	# Ensure that there is exactly 1 negotiation by checking that there is
 	# only 1 "done" line sent. ("done" marks the end of negotiation.)
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client diff -M HEAD^ HEAD >out &&
-	grep "similarity index" out &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client diff --raw -M HEAD^ HEAD >out &&
+	grep ":100644 100644.*R[0-9][0-9][0-9].*b.*c" out &&
 	grep "git> done" trace >done_lines &&
 	test_line_count = 1 done_lines
 '
-- 
2.26.2.761.g0e0b3e54be-goog

Re: [PATCH] t4067: make rename detection test output raw diff

[Derrick Stolee]

On 5/19/2020 2:31 PM, Jonathan Tan wrote:
> 95acf11a3d ("diff: restrict when prefetching occurs", 2020-04-07) taught
> diff to prefetch blobs in a more limited set of situations. These
> limited situations include when the output format requires blob data,
> and when inexact rename detection is needed.
> 
> There is an existing test case that tests inexact rename detection, but
> it also uses an output format that requires blob data, resulting in the
> inexact-rename-detection-only code not being tested. Update this test to
> use the raw output format, which does not require blob data.
> 
> Thanks to Derrick Stolee for noticing this lapse in code coverage and
> for doing the preliminary analysis [1].
> 
> [1] https://lore.kernel.org/git/853759d3-97c3-241f-98e1-990883cd204e@gmail.com/
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Thanks, Stolee. Yes, we were never in a situation where there are any
> missing objects at the point of inexact rename detection (despite having
> a test exactly for this), but this situation is possible, and I've
> updated the test so that we encounter this situation.

Thanks for finding the test bug!

> ---
>  t/t4067-diff-partial-clone.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh
> index c1ed1c2fc4..ef8e0e9cb0 100755
> --- a/t/t4067-diff-partial-clone.sh
> +++ b/t/t4067-diff-partial-clone.sh
> @@ -125,8 +125,8 @@ test_expect_success 'diff with rename detection batches blobs' '
>  
>  	# Ensure that there is exactly 1 negotiation by checking that there is
>  	# only 1 "done" line sent. ("done" marks the end of negotiation.)
> -	GIT_TRACE_PACKET="$(pwd)/trace" git -C client diff -M HEAD^ HEAD >out &&
> -	grep "similarity index" out &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C client diff --raw -M HEAD^ HEAD >out &&
> +	grep ":100644 100644.*R[0-9][0-9][0-9].*b.*c" out &&

--raw definitely does rename detection and I understand the need
to adjust your grep command here.

Thanks,
-Stolee

Re: Git Test Coverage Report (v2.27.0-rc0)

[René Scharfe]

Am 19.05.20 um 14:11 schrieb Derrick Stolee:
> On 5/15/2020 1:22 PM, Derrick Stolee wrote:
>> René Scharfe	9068cfb2 fsck: report non-consecutive duplicate names in trees
>> fsck.c
>> 9068cfb2 623) break;
>> 9068cfb2 626) if (is_less_than_slash(*p)) {
>> 9068cfb2 627) name_stack_push(candidates, f_name);
>> 9068cfb2 628) break;
>> 9068cfb2 630) }
>
> These are different conditions in verify_ordered() to check the
> special case of blobs and trees having the "same" name (but
> sorted differently due to appending '/' to trees before sorting).
>
> There is another name_stack_push() that _is_ covered. I wonder
> what case would trigger this push?

Our test has one entry between the conflicting two:

	x
	x.1
	x/

The branch is necessary if the check of a candidate tree consumes
a candidate file from the stack that we need to check against later
candidate trees, e.g.:

	x
	x.1.2
	x.1/
	x/

Let's check each in sequence:

  1. "x" is a candidate file because the next entry starts with "x" +
     less-than-slash, so "x/" can still follow.  We push it onto the
     stack.
  2. "x.1.2" is not a candidate.  A hypothetical "x.1.2/" would come
     before "x.1/", so we can be sure we don't have it.
  3. "x.1/" is a candidate tree because it follows an entry that starts
     with "x.1" + less-than-slash, so we could have seen a file named
     "x.1"  earlier.  We pop "x" from the stack and see that it doesn't
     match, but is a prefix.  "x.1/" is "x" + less-than-slash, so "x/"
     can still follow.  We push it back onto the stack.  And that's
     the branch in question.
  4. "x/" is a candidate tree because it follows an entry that starts
     with "x" + less-than-slash, so we could have seen a file named "x"
     earlier.  We pop "x" from the stack, and have a match.

But here's one that slips by fsck and the test coverage check:

	x
	x.1
	x.1.2
	x/

This duplicate is not caught by the current code.  Both "x" and "x.1"
are candidate files.  "x.1.2" is not a candidate.  "x/" is checked
against "x.1" from the top of the stack, found not to be a prefix and
the search ends at that point, unsuccessfully.  I think we need this to
fix it:

diff --git a/fsck.c b/fsck.c
index 8bb3ecf282..594e800015 100644
--- a/fsck.c
+++ b/fsck.c
@@ -620,7 +620,7 @@ static int verify_ordered(unsigned mode1, const char *name1,
 			if (!f_name)
 				break;
 			if (!skip_prefix(name2, f_name, &p))
-				break;
+				continue;
 			if (!*p)
 				return TREE_HAS_DUPS;
 			if (is_less_than_slash(*p)) {

Re: Git Test Coverage Report (v2.27.0-rc0)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]

On 2020-05-19 at 12:11:15, Derrick Stolee wrote:
> On 5/15/2020 1:22 PM, Derrick Stolee wrote:
> > brian m. carlson	13e7ed6a builtin/checkout: compute checkout metadata for checkouts
> > builtin/checkout.c
> > 13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :
> 
> This is part of the following hunk:
> 
> @@ -619,6 +620,11 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>         opts.verbose_update = o->show_progress;
>         opts.src_index = &the_index;
>         opts.dst_index = &the_index;
> +       init_checkout_metadata(&opts.meta, info->refname,
> +                              info->commit ? &info->commit->object.oid :
> +                              is_null_oid(&info->oid) ? &tree->object.oid :
> +                              &info->oid,
> +                              NULL);
>         parse_tree(tree);
>         init_tree_desc(&tree_desc, tree->buffer, tree->size);
>         switch (unpack_trees(1, &tree_desc, &opts)) {
> 
> The double-nested ternary definitely complicates the coverage here.
> It also points out that all tests have `info->commit` a non-NULL.
> 
> This certainly looks safe, but I don't know.

This is me trying to be defensive.  I think the code path that is not
covered here that can be is the info->oid code path; I don't technically
believe the other case (using the tree) is possible, although the
checkout code is complex enough that I can't be certain.

It would look like this, I believe:

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 4bfffa9c31..a69ec3c4b7 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -408,6 +408,25 @@ test_expect_success PERL 'required process filter should filter data' '
 		EOF
 		test_cmp_exclude_clean expected.log debug.log &&
 
+		# Make sure that the file appears dirty, so checkout below has to
+		# run the configured filter.
+		META="treeish=$MASTER" &&
+		rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x=.r" &&
+
+		filter_git checkout --quiet --no-progress $(git rev-parse master) &&
+		cat >expected.log <<-EOF &&
+			START
+			init handshake complete
+			IN: smudge test.r $META blob=$M $S [OK] -- OUT: $S . [OK]
+			IN: smudge test2.r $META blob=$M2 $S2 [OK] -- OUT: $S2 . [OK]
+			IN: smudge test4-empty.r $META blob=$EMPTY 0 [OK] -- OUT: 0  [OK]
+			IN: smudge testsubdir/test3 '\''sq'\'',\$x=.r $META blob=$M3 $S3 [OK] -- OUT: $S3 . [OK]
+			STOP
+		EOF
+		test_cmp_exclude_clean expected.log debug.log &&
+
+		git checkout empty-branch &&
+		META="ref=refs/heads/master treeish=$MASTER" &&
 		filter_git checkout --quiet --no-progress master &&
 		cat >expected.log <<-EOF &&
 			START


I'm about to go make dinner, but I'll try to get a patch written up with
this tonight or tomorrow.  If it's more urgent than that, you may add my
sign-off.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: Git Test Coverage Report (v2.27.0-rc0)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

On 2020-05-19 at 23:42:08, brian m. carlson wrote:
> On 2020-05-19 at 12:11:15, Derrick Stolee wrote:
> > On 5/15/2020 1:22 PM, Derrick Stolee wrote:
> > > brian m. carlson	13e7ed6a builtin/checkout: compute checkout metadata for checkouts
> > > builtin/checkout.c
> > > 13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :
> > 
> > This is part of the following hunk:
> > 
> > @@ -619,6 +620,11 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
> >         opts.verbose_update = o->show_progress;
> >         opts.src_index = &the_index;
> >         opts.dst_index = &the_index;
> > +       init_checkout_metadata(&opts.meta, info->refname,
> > +                              info->commit ? &info->commit->object.oid :
> > +                              is_null_oid(&info->oid) ? &tree->object.oid :
> > +                              &info->oid,
> > +                              NULL);
> >         parse_tree(tree);
> >         init_tree_desc(&tree_desc, tree->buffer, tree->size);
> >         switch (unpack_trees(1, &tree_desc, &opts)) {
> > 
> > The double-nested ternary definitely complicates the coverage here.
> > It also points out that all tests have `info->commit` a non-NULL.
> > 
> > This certainly looks safe, but I don't know.
> 
> This is me trying to be defensive.  I think the code path that is not
> covered here that can be is the info->oid code path; I don't technically
> believe the other case (using the tree) is possible, although the
> checkout code is complex enough that I can't be certain.

Actually, with more research, it looks like this can just be simplified
since info->commit must be non-NULL.  I've analyzed the code and am
confident that's guaranteed to be the case, and the test suite has
confirmed no segfaults by making that assumption, so I'll send a patch
imminently that just does that.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH 1/1] builtin/checkout: simplify metadata initialization

[brian m. carlson]

When we call init_checkout_metadata in reset_tree, we want to pass the
object ID of the commit in question so that it can be passed to filters,
or if there is no commit, the tree.  We anticipated this latter case,
which can occur elsewhere in the checkout code, but it cannot occur
here, since reset_tree is called only (indirectly) via switch_branches,
which requires that we have a valid commit.  switch_branches dies if we
lack a name and cannot produce a commit from HEAD, and its caller dies
if we do have a branch name but still lack a commit pointer.

Since we know we must always have a valid commit structure in this case,
let's remove the dead code paths and just refer to the commit structure.
This simplifies the code and makes it easier for the reader.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/checkout.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e9d111bb83..e53bdab5b8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -620,11 +620,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	opts.verbose_update = o->show_progress;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
-	init_checkout_metadata(&opts.meta, info->refname,
-			       info->commit ? &info->commit->object.oid :
-			       is_null_oid(&info->oid) ? &tree->object.oid :
-			       &info->oid,
-			       NULL);
+	init_checkout_metadata(&opts.meta, info->refname, &info->commit->object.oid, NULL);
 	parse_tree(tree);
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
 	switch (unpack_trees(1, &tree_desc, &opts)) {

Re: [PATCH 1/1] builtin/checkout: simplify metadata initialization

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> When we call init_checkout_metadata in reset_tree, we want to pass the
> object ID of the commit in question so that it can be passed to filters,
> or if there is no commit, the tree.  We anticipated this latter case,
> which can occur elsewhere in the checkout code, but it cannot occur
> here, since reset_tree is called only (indirectly) via switch_branches,
> which requires that we have a valid commit.  switch_branches dies if we
> lack a name and cannot produce a commit from HEAD, and its caller dies
> if we do have a branch name but still lack a commit pointer.
>
> Since we know we must always have a valid commit structure in this case,
> let's remove the dead code paths and just refer to the commit structure.
> This simplifies the code and makes it easier for the reader.

builtin/checkout.c::merge_working_tree() has these lines in its
earlier part:

	if (opts->new_orphan_branch && opts->orphan_from_empty_tree) {
		if (new_branch_info->commit)
			BUG("'switch --orphan' should never acc...");
		new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
	} else
		new_tree = get_commit_tree(new_branch_info->commit);
	if (opts->discard_changes) {
		ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
		if (ret)
			return ret;
	...

So, when orphan && orphan-from-empty are both set, we must not have
commit, and then if discard is also there, we end up passing
new_brnach_info that has NULL in its commit.

There are few more callers of reset_tree() in this function, which I
did not trace.

Perhaps the "orphan && orphan-from-empty" is a dead combination and
we won't hit the codepath and that is why this change is safe?  I
dunno.


> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/checkout.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e9d111bb83..e53bdab5b8 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -620,11 +620,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>  	opts.verbose_update = o->show_progress;
>  	opts.src_index = &the_index;
>  	opts.dst_index = &the_index;
> -	init_checkout_metadata(&opts.meta, info->refname,
> -			       info->commit ? &info->commit->object.oid :
> -			       is_null_oid(&info->oid) ? &tree->object.oid :
> -			       &info->oid,
> -			       NULL);
> +	init_checkout_metadata(&opts.meta, info->refname, &info->commit->object.oid, NULL);
>  	parse_tree(tree);
>  	init_tree_desc(&tree_desc, tree->buffer, tree->size);
>  	switch (unpack_trees(1, &tree_desc, &opts)) {

Re: [PATCH 1/1] builtin/checkout: simplify metadata initialization

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]

On 2020-05-20 at 15:17:43, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > When we call init_checkout_metadata in reset_tree, we want to pass the
> > object ID of the commit in question so that it can be passed to filters,
> > or if there is no commit, the tree.  We anticipated this latter case,
> > which can occur elsewhere in the checkout code, but it cannot occur
> > here, since reset_tree is called only (indirectly) via switch_branches,
> > which requires that we have a valid commit.  switch_branches dies if we
> > lack a name and cannot produce a commit from HEAD, and its caller dies
> > if we do have a branch name but still lack a commit pointer.
> >
> > Since we know we must always have a valid commit structure in this case,
> > let's remove the dead code paths and just refer to the commit structure.
> > This simplifies the code and makes it easier for the reader.
> 
> builtin/checkout.c::merge_working_tree() has these lines in its
> earlier part:
> 
> 	if (opts->new_orphan_branch && opts->orphan_from_empty_tree) {
> 		if (new_branch_info->commit)
> 			BUG("'switch --orphan' should never acc...");
> 		new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
> 	} else
> 		new_tree = get_commit_tree(new_branch_info->commit);
> 	if (opts->discard_changes) {
> 		ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
> 		if (ret)
> 			return ret;
> 	...
> 
> So, when orphan && orphan-from-empty are both set, we must not have
> commit, and then if discard is also there, we end up passing
> new_brnach_info that has NULL in its commit.

Good point.  I missed that part.

> Perhaps the "orphan && orphan-from-empty" is a dead combination and
> we won't hit the codepath and that is why this change is safe?  I
> dunno.

It looks like it's only triggered from git switch with --orphan, not
with git checkout.  And furthermore, it seems we require --force or
--discard-changes to trigger that case, which we don't have anywhere in
the testsuite.

I'll come up with a different patch.  I'll probably just set a NULL
object ID there, since if we're checking out a new orphan branch, we
won't have any files to check out, and therefore there's no possibility
that we'll actually use the value for a filter process (since there are
no files to filter).
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v2 0/2] Improve Fix code coverage for checkout

[brian m. carlson]

Patch #1 reduces the number of options in the scenario which Stolee
mentioned above.  There's now just a NULL and a non-NULL case, and the
NULL case is now relatively straightforward and uninteresting.

Patch #2 adds a test for the particular set of options which will
trigger this case as an independent test.  I didn't think it made sense
to put this in t0021, since ultimately that set of options isn't about
conversions and it would seem out of place there, so I put it in t2060.

I'm ultimately on the fence for this case, because I think it's really a
corner case and testing this is probably not that interesting, so my
preference is for us to pick up patch 1 and drop patch 2.  However, I
added patch 2 in case we do indeed want a test for this, and I'll let
Junio and others decide on what's best.

brian m. carlson (2):
  builtin/checkout: simplify metadata initialization
  t2060: add a test for switch with --orphan and --discard-changes

 builtin/checkout.c | 4 +---
 t/t2060-switch.sh  | 8 ++++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

[PATCH v2 1/2] builtin/checkout: simplify metadata initialization

[brian m. carlson]

When we call init_checkout_metadata in reset_tree, we want to pass the
object ID of the commit in question so that it can be passed to filters,
or if there is no commit, the tree.  We anticipated this latter case,
which can occur elsewhere in the checkout code, but it cannot occur
here.  The only case in which we do not have a commit object is when
invoking git switch with --orphan.  Moreover, we can only hit this code
path without a commit object additionally with either --force or
--discard-changes.

In such a case, there is no point initializing the checkout metadata
with a commit or tree because (a) there is no commit, only the empty
tree, and (b) we will never use the data, since no files will be smudged
when checking out a branch with no files.  Pass the all-zeros object ID
in this case, since we just need some value which is a valid pointer.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/checkout.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e9d111bb83..62b5e371bc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -621,9 +621,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	init_checkout_metadata(&opts.meta, info->refname,
-			       info->commit ? &info->commit->object.oid :
-			       is_null_oid(&info->oid) ? &tree->object.oid :
-			       &info->oid,
+			       info->commit ? &info->commit->object.oid : &null_oid,
 			       NULL);
 	parse_tree(tree);
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);

[PATCH v2 2/2] t2060: add a test for switch with --orphan and --discard-changes

[brian m. carlson]

We have several code paths in the checkout code which are traversed only
in this case, due to switch having different defaults from checkout.
Let's add a test that the combination of options works and produces the
expected behavior.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t2060-switch.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index f9efa29dfb..2c1b8c0d6d 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -68,6 +68,14 @@ test_expect_success 'new orphan branch from empty' '
 	test_cmp expected tracked-files
 '
 
+test_expect_success 'orphan branch works with --discard-changes' '
+	test_when_finished git switch master &&
+	echo foo >foo.txt &&
+	git switch --discard-changes --orphan new-orphan2 &&
+	git ls-files >tracked-files &&
+	test_must_be_empty tracked-files
+'
+
 test_expect_success 'switching ignores file of same branch name' '
 	test_when_finished git switch master &&
 	: >first-branch &&

Re: [PATCH v2 0/2] Improve Fix code coverage for checkout

[Derrick Stolee]

On 5/20/2020 10:07 PM, brian m. carlson wrote:
> Patch #1 reduces the number of options in the scenario which Stolee
> mentioned above.  There's now just a NULL and a non-NULL case, and the
> NULL case is now relatively straightforward and uninteresting.

I'm glad you and Junio were able to simplify this case!

> Patch #2 adds a test for the particular set of options which will
> trigger this case as an independent test.  I didn't think it made sense
> to put this in t0021, since ultimately that set of options isn't about
> conversions and it would seem out of place there, so I put it in t2060.
> 
> I'm ultimately on the fence for this case, because I think it's really a
> corner case and testing this is probably not that interesting, so my
> preference is for us to pick up patch 1 and drop patch 2.  However, I
> added patch 2 in case we do indeed want a test for this, and I'll let
> Junio and others decide on what's best.

I think the test is helpful, since it is not very complicated to set up.

The test you created for t0021-conversion.sh earlier was quite complicated
and required internal knowledge to get going. However, this test only uses
porcelain commands to trigger the conditional.

From the discussion here, it is not obvious that info->commit is ever NULL,
so having the test can prevent a future change from making that same
assumption.

This series is Reviewed-by: Derrick Stolee <dstolee@microsoft.com>

Thanks,
-Stolee

Re: [PATCH v2 1/2] builtin/checkout: simplify metadata initialization

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> ...  The only case in which we do not have a commit object is when
> invoking git switch with --orphan.  Moreover, we can only hit this code
> path without a commit object additionally with either --force or
> --discard-changes.

It was easy for me to trace the codepath to see when these options
are used we end up with no commit object, but I ran out of time
trying to see if the "forced orphan" is the only way to end up with
a NULL in new_branch_info->commit.  Assuming that is true, of course
the following perfectly makes sense.

> In such a case, there is no point initializing the checkout metadata
> with a commit or tree because (a) there is no commit, only the empty
> tree, and (b) we will never use the data, since no files will be smudged
> when checking out a branch with no files.  Pass the all-zeros object ID
> in this case, since we just need some value which is a valid pointer.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/checkout.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Thanks.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e9d111bb83..62b5e371bc 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -621,9 +621,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>  	opts.src_index = &the_index;
>  	opts.dst_index = &the_index;
>  	init_checkout_metadata(&opts.meta, info->refname,
> -			       info->commit ? &info->commit->object.oid :
> -			       is_null_oid(&info->oid) ? &tree->object.oid :
> -			       &info->oid,
> +			       info->commit ? &info->commit->object.oid : &null_oid,
>  			       NULL);
>  	parse_tree(tree);
>  	init_tree_desc(&tree_desc, tree->buffer, tree->size);

Re: [PATCH v2 1/2] builtin/checkout: simplify metadata initialization

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

On 2020-05-21 at 17:35:22, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > ...  The only case in which we do not have a commit object is when
> > invoking git switch with --orphan.  Moreover, we can only hit this code
> > path without a commit object additionally with either --force or
> > --discard-changes.
> 
> It was easy for me to trace the codepath to see when these options
> are used we end up with no commit object, but I ran out of time
> trying to see if the "forced orphan" is the only way to end up with
> a NULL in new_branch_info->commit.  Assuming that is true, of course
> the following perfectly makes sense.

I believe it is.  The only case in which we have a NULL commit as far as
I can tell is with switch and an orphan, and in merge_working_tree we
call reset_tree either if the changes are discarded or unpack_trees
couldn't do a trivial merge.  Since I'm pretty sure unpack_trees can
indeed merge with the empty tree, we would only call reset_trees with
--discard-changes or --force.  And reset_tree is only called from
merge_working_tree.

In addition, I did try other situations plus the entire testsuite with
my erroneous first patch and was unable to cause a segfault anywhere
(which would have been a trivial NULL dereference) in case I missed
something, which leads me to believe that this is in fact the only
situation in which this occurs.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]