Re: Git Test Coverage Report (Thurs. June 27)

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 27, 2019, 17:35 UTC
Message-ID: <14689d27-eecd-2e0a-715d-796b20d573e5@gmail.com>
In-Reply-To: <49d98293-9f0b-44e9-cb07-d6b7ac791eb6@gmail.com>

Here are some interesting sections I found when examining the test coverage report. I am only highlighting these sections because they seem to include non-trivial logic. In some cases, maybe the code isn't needed.

On 6/27/2019 1:05 PM, Derrick Stolee wrote:

Show 33 quoted lines

> promisor-remote.c
> db27dca5 25) die(_("Remote with no URL"));
> 48de3158 61) warning(_("promisor remote name cannot begin with '/': %s"),
> 48de3158 63) return NULL;
> faf2abf4 93) previous->next = r->next;
> 4ca9474e 108) return git_config_string(&core_partial_clone_filter_default,
> fa3d1b63 139) return 0;
> 9e27beaa 202) static int remove_fetched_oids(struct repository *repo,
> 9e27beaa 206) int i, remaining_nr = 0;
> 9e27beaa 207) int *remaining = xcalloc(oid_nr, sizeof(*remaining));
> 9e27beaa 208) struct object_id *old_oids = *oids;
> 9e27beaa 211) for (i = 0; i < oid_nr; i++)
> 9e27beaa 212) if (oid_object_info_extended(repo, &old_oids[i], NULL,
> 9e27beaa 214) remaining[i] = 1;
> 9e27beaa 215) remaining_nr++;
> 9e27beaa 218) if (remaining_nr) {
> 9e27beaa 219) int j = 0;
> 9e27beaa 220) new_oids = xcalloc(remaining_nr, sizeof(*new_oids));
> 9e27beaa 221) for (i = 0; i < oid_nr; i++)
> 9e27beaa 222) if (remaining[i])
> 9e27beaa 223) oidcpy(&new_oids[j++], &old_oids[i]);
> 9e27beaa 224) *oids = new_oids;
> 9e27beaa 225) if (to_free)
> 9e27beaa 226) free(old_oids);
> 9e27beaa 229) free(remaining);
> 9e27beaa 231) return remaining_nr;
> 9e27beaa 248) if (remaining_nr == 1)
> 9e27beaa 249) continue;
> 9e27beaa 250) remaining_nr = remove_fetched_oids(repo, &remaining_oids,
> 9e27beaa 252) if (remaining_nr) {
> 9e27beaa 253) to_free = 1;
> 9e27beaa 254) continue;
> 9e27beaa 262) free(remaining_oids);

Christian: this section continues to be untested, but I think you were
working on creating tests for this.

Show 9 quoted lines

> repo-settings.c
> 0a01e977 13) int rate = git_config_int(key, value);
> 0a01e977 14) if (rate >= 3) {
> 0a01e977 15) UPDATE_DEFAULT(rs->core_commit_graph, 1);
> 0a01e977 16) UPDATE_DEFAULT(rs->gc_write_commit_graph, 1);
> c5c84f32 17) UPDATE_DEFAULT(rs->index_version, 4);
> 3172404b 19) if (rate >= 5) {
> 3172404b 20) UPDATE_DEFAULT(rs->pack_use_sparse, 1);
> 0a01e977 22) return 0;

These are mine. Since no one has been complaining about the design of core.featureAdoptionRate in ds/early-access [1], I'll move forward to add some tests for this setting. It may come in the form of a GIT_TEST_ADOPTION_RATE environment variable so it has wider coverage across the test suite. I may even add explicit tests that demonstrate the new defaults enabled by core.featureAdoptionRate are overridden by explicit config settings. index.version is a good candidate.

[1] https://public-inbox.org/git/pull.254.v2.git.gitgitgadget@gmail.com/

> t/helper/test-match-trees.c
> 3fe87a7f 23) shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1);

Duy: here is another example of a conversion to "struct repository *" but
since it is using the_repository here it is definitely safe (no worse than before).

> builtin/fetch.c
> cdbd70c4 88) fetch_show_forced_updates = git_config_bool(k, v);
> cdbd70c4 89) return 0;

Mine again. I should explicitly test the fetch.showForcedUpdates config option, especially against the '--no-show-forced-updates' argument (to show the argument wins).

> builtin/pull.c
> 3883c551 556) argv_array_push(&args, "--show-forced-updates");
> 3883c551 558) argv_array_push(&args, "--no-show-forced-updates");

Not sure if this is super-important to test. It's a simple argument pass-through. But also maybe I change my 'git fetch' tests to be 'git pull' tests and it covers the 'git fetch' builtin at the same time.

Show 20 quoted lines

> dir.c
> 3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,
> 3b2385cf 2845) jw_object_bool(jw, "valid", ucd->valid);
> 3b2385cf 2846) jw_object_bool(jw, "check-only", ucd->check_only);
> 3b2385cf 2847) jw_object_stat_data(jw, "stat", &ucd->stat_data);
> 3b2385cf 2848) jw_object_string(jw, "exclude-oid", oid_to_hex(&ucd->exclude_oid));
> 3b2385cf 2849) jw_object_inline_begin_array(jw, "untracked");
> 3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)
> 3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);
> 3b2385cf 2852) jw_end(jw);
> 3b2385cf 2854) jw_object_inline_begin_object(jw, "dirs");
> 3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {
> 3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);
> 3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);
> 3b2385cf 2858) jw_end(jw);
> 3b2385cf 2860) jw_end(jw);
> 3b2385cf 2861) }
> 3b2385cf 2958) jw_object_inline_begin_object(jw, "root");
> 3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);
> 3b2385cf 2960) jw_end(jw);

Duy: I know you were working on some tests for these options. This is specifically
in the "untracked cache" mode, so enabling the cache with at least one entry and
running --debug-json should be sufficient.

Show 5 quoted lines

> pack-bitmap-write.c
> 05805d74 378) static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)
> d2bc62b1 385) hash_pos = kh_get_oid_map(writer.reused, *oid);
> 05805d74 425) reused_bitmap = find_reused_bitmap(&chosen->object.oid);
> 05805d74 432) reused_bitmap = find_reused_bitmap(&cm->object.oid);

Peff: it is interesting that these portions are not covered previously. (Your change
is clearly mechanical and does not change the correctness.) In particular, lines 425
and 432 are in two blocks of an if/else with one further inside a loop. The loop
should always have at least one run, so this if/else isn't even covered.

Show 11 quoted lines

> read-cache.c
> 8eeabe15 1752) ret = error(_("index uses %.4s extension, which we do not understand"),
> ee70c128 1754) if (advice_unknown_index_extension) {
> ee70c128 1755) warning(_("ignoring optional %.4s index extension"), ext);
> ee70c128 1756) advise(_("This is likely due to the file having been written by a newer\n"
> 272b3f2a 2026) jw_object_true(jw, "assume_unchanged");
> 272b3f2a 2030) jw_object_true(jw, "skip_worktree");
> 272b3f2a 2032) jw_object_intmax(jw, "stage", ce_stage(ce));
> f0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);
> f0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(
> f0f544da 3673) return do_read_ieot_extension(istate, index, extsize);

Duy: more JSON output cases that could be interesting to cover.

> t/helper/test-example-decorate.c
> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);
> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);
> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);

Peff: again interesting that these lines you refactored were not covered, especially
because they are part of a test helper. Perhaps the tests they were intended for are
now defunct?

Show 7 quoted lines

> t/helper/test-oidmap.c
> 11510dec 52) if (get_oid(p1, &oid)) {
> 11510dec 53) printf("Unknown oid: %s\n", p1);
> 11510dec 54) continue;
> 11510dec 58) FLEX_ALLOC_STR(entry, name, p2);
> 11510dec 59) oidcpy(&entry->entry.oid, &oid);
> 11510dec 62) oidmap_put(&map, entry);

Christian: this block looks like the test-oidmap helper never uses the "add"
subcommand. Is that correct?

Show 6 quoted lines

> 11510dec 97) if (get_oid(p1, &oid)) {
> 11510dec 98) printf("Unknown oid: %s\n", p1);
> 11510dec 99) continue;
> 11510dec 103) entry = oidmap_remove(&map, &oid);
> 11510dec 106) puts(entry ? entry->name : "NULL");
> 11510dec 107) free(entry);

Similarly, this block means we are not using the "remove" subcommand.

Thanks, -Stolee