Re: Git Test Coverage Report (Sept 19)

[Derrick Stolee <stolee@gmail.com>]

On 9/19/2019 10:23 AM, Derrick Stolee wrote:
> Here is today's test coverage report.

And I took a close look at the report, looking for interesting cases
that are not covered. Most of the uncovered lines were due to simple
refactors or error conditions. I point out a few below that took a
bit of digging to consider.

> sequencer.c
> ccafcb32 884) static char *read_author_date_or_null(void)
> ccafcb32 888) if (read_author_script(rebase_path_author_script(),
> ccafcb32 890) return NULL;
> ccafcb32 891) return date;

This method was added by this commit, but is not tested.
> ccafcb32 983) int res = -1;
> ccafcb32 984) struct strbuf datebuf = STRBUF_INIT;
> ccafcb32 985) char *date = read_author_date_or_null();
> ccafcb32 987) if (!date)
> ccafcb32 988) return -1;
> ccafcb32 990) strbuf_addf(&datebuf, "@%s", date);
> ccafcb32 991) res = setenv("GIT_COMMITTER_DATE", datebuf.buf, 1);
> ccafcb32 993) strbuf_release(&datebuf);
> ccafcb32 994) free(date);
> ccafcb32 996) if (res)
> ccafcb32 997) return -1;

These lines are inside an 'if (opts->committer_date_is_author_date)'
block, and there is another block that _is_ covered.

(Philip already pointed this out in his review. Thanks!)

> 7258d3d1 912) static void push_dates(struct child_process *child)
> 7258d3d1 914) time_t now = time(NULL);
> 7258d3d1 915) struct strbuf date = STRBUF_INIT;
> 7258d3d1 917) strbuf_addf(&date, "@%"PRIuMAX, (uintmax_t)now);
> 7258d3d1 918) argv_array_pushf(&child->env_array, "GIT_AUTHOR_DATE=%s", date.buf);
> 7258d3d1 919) argv_array_pushf(&child->env_array, "GIT_COMMITTER_DATE=%s", date.buf);
> 7258d3d1 920) strbuf_release(&date);
> 7258d3d1 921) }
> 7258d3d1 1016) push_dates(&cmd);
> 7258d3d1 1488) res = -1;
> 7258d3d1 1489) goto out;
> 7258d3d1 3605) push_dates(&cmd);

Similarly, the "push_dates()" method is not covered.

---

> builtin/pack-objects.c
> 7c59828b 2694) (reuse_packfile_bitmap &&
> 7c59828b 2695)  bitmap_walk_contains(bitmap_git, reuse_packfile_bitmap, oid));

This change to obj_is_packed(oid) is a small change in a
really big commit. Here is the change:

@@ -2571,7 +2706,9 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 
 static int obj_is_packed(const struct object_id *oid)
 {
-       return !!packlist_find(&to_pack, oid, NULL);
+       return packlist_find(&to_pack, oid, NULL) ||
+               (reuse_packfile_bitmap &&
+                bitmap_walk_contains(bitmap_git, reuse_packfile_bitmap, oid));
 }

So, every time this is called, the || is short-circuited because
packlist_find() returns true.

Does this change the meaning of this method? obj_is_packed() would
only return true if we found the object specifically in the to_pack
list. Now, we would also return true if the object is in the bitmap
walk.

If this is only a performance improvement, and the bitmap_walk_contains()
method would return the same as packlist_find(), then should the order
be swapped? Or rather, should reuse_packfile_bitmap indicate which to use
as the full result?

> d35b73c5 2847) allow_pack_reuse = git_config_bool(k, v);
> d35b73c5 2848) return 0;

I initially thought that introducing an untested config setting is
generally not a good idea, but allow_pack_reuse is on by default.
This config setting is just a safety valve to turn the feature _off_
if necessary.

--- 
> builtin/checkout.c
> 65c01c64 766) strbuf_add_unique_abbrev(&old_commit_shortname,
> 65c01c64 767)  &old_branch_info->commit->object.oid,
> 65c01c64 769) o.ancestor = old_commit_shortname.buf;

The entire point of this commit is to modify the output during
a 'git checkout -m' in a detached HEAD case. Odd that a test
was not added to demonstrate the expected output change.

Since the point of the test coverage report is to find places
where a bug may exist or may appear later, this block may be
small enough to ignore.

> cache-tree.c
> 724dd767 619) cache_tree_free(&index_state->cache_tree);
> 724dd767 620) cache_tree_valid = 0;
> 724dd767 633) return WRITE_TREE_PREFIX_ERROR;
> 724dd767 653) fprintf(stderr, "BUG: There are unmerged index entries:\n");
> 724dd767 654) for (i = 0; i < index_state->cache_nr; i++) {
> 724dd767 655) const struct cache_entry *ce = index_state->cache[i];
> 724dd767 656) if (ce_stage(ce))
> 724dd767 657) fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
> 724dd767 658) (int)ce_namelen(ce), ce->name);
> 724dd767 660) BUG("unmerged index entries when writing inmemory index");

These uncovered lines were moved during a refactor, which means they
were uncovered before. Lots of strange branching happening here, but
it must be in very rare cases.
 
> connect.c
> ebb8d2c9 921) path = host - 2; /* include the leading "//" */

> setup.c
> e2683d51 952)     !is_dir_sep(dir->buf[min_offset - 1])) {
> e2683d51 953) strbuf_addch(dir, '/');
> e2683d51 954) min_offset++;

(These lines are artifacts of not running test coverage on Windows.)

> promisor-remote.c

(We have discussed the test coverage for multiple promisor remotes before.)
 
---

Thanks, everyone. Things are looking good.

-Stolee