Re: Git Test Coverage Report for v2.37.0-rc0

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 14, 2022, 20:20 UTC
Message-ID: <3d1c6dfd-1df6-3393-df5e-692719375772@github.com>
In-Reply-To: <00a57a1d-0566-8f54-26b2-0f3558bde88d@github.com>

On 6/14/2022 4:18 PM, Derrick Stolee wrote:

Show 7 quoted lines

> Ævar Arnfjörð Bjarmason	338959da remote.c: remove braces from one-statement "for"-loops
> remote.c
> 338959da 149) for (i = 0; i < remote->url_nr; i++)
> 338959da 153) for (i = 0; i < remote->pushurl_nr; i++)
> Ævar Arnfjörð Bjarmason	323822c7 remote.c: don't dereference NULL in freeing loop
> remote.c
> 323822c7 151) FREE_AND_NULL(remote->url);

Just noting that these lines are inside remote_clear() which is called by remote_state_clear(), which is called by repo_clear(). Apparently we have no tests that clear a 'struct repository' that loaded remotes? This sounds likely since we don't close these unless they are not the_repository.

Show 7 quoted lines

> Ævar Arnfjörð Bjarmason	fd3aaf53 run-command: add an "ungroup" option to run_process_parallel()
> run-command.c
> fd3aaf53 1645) code = pp->start_failure(pp->ungroup ? NULL :
> fd3aaf53 1646)  &pp->children[i].err,
> fd3aaf53 1649) if (!pp->ungroup) {
> fd3aaf53 1650) strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
> fd3aaf53 1651) strbuf_reset(&pp->children[i].err);

This change seems sufficiently complicated that it would be good to look into the test coverage here. It's possible that it is covered by the GIT_TEST_* variables that I didn't use when generating my test coverage.

> Christian Couder	511cfd3b http: add custom hostname to IP address resolutions
> http.c

(These untested lines are my fault for not having Apache installed. I'll be sure to include that in my coverage next time.)

> Derrick Stolee	b56166ca multi-pack-index: use --object-dir real path
> builtin/multi-pack-index.c
> b56166ca 61) opts.object_dir = xstrdup(get_object_directory());

This is demonstrating that opts.object_dir is never populated before we parse the options. We must be handling a NULL object_dir properly somewhere else. I'll work on a patch to fix this.

> Derrick Stolee	080ab56a cache-tree: implement cache_tree_find_path()
> cache-tree.c
> 080ab56a 104) struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path)

Hm. This commit claims that this method will be used later, but it never is.

I even checked our code in microsoft/git and it's dead code there, too. We should probably revert this commit. (Or: will it be useful for Shaoxuan's GSoC work? Perhaps we hold onto it for a little longer?)

> Jeff Hostetler	9915e08f t/helper/hexdump: add helper to print hexdump of stdin

I did my testing on Linux, so all of the FS Monitor stuff will be untested. Even if we did the testing on macOS, I doubt that the daemon code would be tracked properly.

> Neeraj Singh	23a3a303 update-index: use the bulk-checkin infrastructure
> builtin/update-index.c
> 23a3a303 68) flush_odb_transaction();

This is signalling that we never use 'git update-index --verbose' in our test suite. Might be worth fixing that.

Show 5 quoted lines

> Taylor Blau	5b92477f builtin/gc.c: conditionally avoid pruning objects via loose
> builtin/gc.c
> 5b92477f 337) strvec_push(&repack, "--cruft");
> 5b92477f 338) if (prune_expire)
> 5b92477f 339) strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire);

The context here is this:

static void add_repack_all_option(struct string_list *keep_pack)
{
	if (prune_expire && !strcmp(prune_expire, "now"))
		strvec_push(&repack, "-a");
	else if (cruft_packs) {
		strvec_push(&repack, "--cruft");
		if (prune_expire)
			strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire);
	} else {

The only test that checks this behavior runs "git gc --cruft --prune=now", so the "-a" option is being added and we never add the "--cruft" option.

We should probably add a test for "git gc --cruft" with a different prune time.

> Taylor Blau	f9825d1c builtin/repack.c: support generating a cruft pack
> builtin/repack.c
> f9825d1c 680) strvec_pushf(&cmd.args, "--cruft-expiration=%s",

The --cruft-expiration option is not tested.

> f9825d1c 708) fprintf(in, "%s.pack\n", item->string);

This is related to the existence of .keep packs. A corner case, but maybe worth exploring.

> pack-write.c
> 5dfaf49a 330) unlink(mtimes_name);
> 5dfaf49a 331) fd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);

This appears to be an interesting case for the write_mtimes_file() method, since its first parameter is 'mtimes_name' and all tested cases are passing NULL, it seems.

Thanks, -Stolee