On 6/14/2022 4:18 PM, Derrick Stolee wrote:
> Æ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.
> Æ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.
> 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
