From: Derrick Stolee <derrickstolee@github•com>
To: "Git Mailing List" <git@vger•kernel.org>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail•com>,
"Taylor Blau" <me@ttaylorr•com>,
"Christian Couder" <christian.couder@gmail•com>,
"Jeff Hostetler" <git@jeffhostetler•com>,
"Neeraj Singh" <nksingh85@gmail•com>
Subject: Re: Git Test Coverage Report for v2.37.0-rc0
Date: Tue, 14 Jun 2022 16:20:05 -0400 [thread overview]
Message-ID: <3d1c6dfd-1df6-3393-df5e-692719375772@github.com> (raw)
In-Reply-To: <00a57a1d-0566-8f54-26b2-0f3558bde88d@github.com>
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
next prev parent reply other threads:[~2022-06-14 20:20 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-14 20:18 Git Test Coverage Report for v2.37.0-rc0 Derrick Stolee
2022-06-14 20:20 ` Derrick Stolee [this message]
2022-06-14 20:34 ` Derrick Stolee
2022-06-14 23:46 ` Taylor Blau
2022-06-15 10:16 ` Ævar Arnfjörð Bjarmason
2022-06-15 13:21 ` Derrick Stolee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3d1c6dfd-1df6-3393-df5e-692719375772@github.com \
--to=derrickstolee@github$(echo .)com \
--cc=avarab@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@jeffhostetler$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=me@ttaylorr$(echo .)com \
--cc=nksingh85@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox