On 11/26/2019 3:46 PM, Johannes Schindelin wrote:

Hi Stolee,

thank you for keeping doing these reports.

Thanks for reading it!

On Mon, 25 Nov 2019, Derrick Stolee wrote:

quoted

Here is today's test coverage report.

Sorry for the delay, but I had some issues working out the new
"ignored lines" logic. Now there are a few phrases that are
ignored automatically, including "BUG(" or "die(".

[...]
Johannes Schindelin	eea4a7f4 mingw: demonstrate that all file handles are inherited by child processes
t/helper/test-run-command.c
eea4a7f4 203) static int inherit_handle(const char *argv0)
eea4a7f4 205) struct child_process cp = CHILD_PROCESS_INIT;
eea4a7f4 210) xsnprintf(path, sizeof(path), "out-XXXXXX");
eea4a7f4 211) tmp = xmkstemp(path);
eea4a7f4 213) argv_array_pushl(&cp.args,
eea4a7f4 215) cp.in = -1;
eea4a7f4 216) cp.no_stdout = cp.no_stderr = 1;
eea4a7f4 217) if (start_command(&cp) < 0)
eea4a7f4 221) close(tmp);
eea4a7f4 222) if (unlink(path))
eea4a7f4 225) if (close(cp.in) < 0 || finish_command(&cp) < 0)
eea4a7f4 228) return 0;
eea4a7f4 231) static int inherit_handle_child(void)
eea4a7f4 233) struct strbuf buf = STRBUF_INIT;
eea4a7f4 235) if (strbuf_read(&buf, 0, 0) < 0)
eea4a7f4 237) printf("Received %s\n", buf.buf);
eea4a7f4 238) strbuf_release(&buf);
eea4a7f4 240) return 0;
eea4a7f4 251) exit(inherit_handle(argv[0]));
eea4a7f4 253) exit(inherit_handle_child());

I think this is actually covered, but only in the Windows build.

Sorry that I have not set up a Windows build, but the Linux test-coverage
build already takes a long time so adding Windows would be complicated.
(Not to mention that merging the uncovered lines across two platforms would
be a huge challenge.)

If anyone wants to consider such an effort, I'm willing to play along.

quoted

Johannes Schindelin	116a408b commit: give correct advice for empty commit during a rebase
builtin/commit.c
116a408b 478) else if (whence == FROM_CHERRY_PICK) {
116a408b 479) if (rebase_in_progress && !sequencer_in_use)
116a408b 1186) else if (whence == FROM_CHERRY_PICK) {
116a408b 1187) if (rebase_in_progress && !sequencer_in_use)

My understanding is that Phillip is working on a replacement for this
patch, so I'll leave this as-is for now.

quoted

Johannes Schindelin	0de7b036 built-in add -i: offer the `quit` command
add-interactive.c
[...]

Johannes Schindelin	d17e3a76 built-in add -i: prepare for multi-selection commands
add-interactive.c
[...]

Johannes Schindelin	dea080c8 built-in add -i: re-implement `revert` in C
add-interactive.c
[...]

Johannes Schindelin	daabf56b built-in add -i: re-implement `add-untracked` in C
add-interactive.c
[...]

Johannes Schindelin	4ba7c22a built-in add -i: implement the `update` command
add-interactive.c
[...]

Johannes Schindelin	9d86921e built-in add -i: implement the `patch` command
add-interactive.c
[...]

Johannes Schindelin	a5ced6db built-in add -i: allow filtering the modified files list
add-interactive.c
[...]

Johannes Schindelin	465a9699 built-in add -i: re-implement the `diff` command
add-interactive.c
[...]

Johannes Schindelin	68db1cbf built-in add -i: support `?` (prompt help)
add-interactive.c
[...]

Johannes Schindelin	76b74323 built-in add -i: show unique prefixes of the commands
add-interactive.c
[...]

Johannes Schindelin	6348bfba built-in add -i: implement the main loop
add-interactive.c
[...]

Johannes Schindelin	f83dff60 Start to implement a built-in version of `git add --interactive`
add-interactive.c
f83dff60 571) int run_add_i(struct repository *r, const struct pathspec *ps)

builtin/add.c
f83dff60 197) return !!run_add_i(the_repository, pathspec);

This ginormous amount of code without test coverage is exactly the type of
problem I pointed out with the approach I had to take in the built-in add
-i/-p series, and which I described in the commit message
https://public-inbox.org/git/5d9962d4344fa182b37cd8d969da01bc603414be.1573648866.git.gitgitgadget@gmail.com/
Most of it _will_ be covered by the tests, but _only_ at the very end,
when all six patch series will be merged.

I figured as much. When we have all six ready to go, then I can run a
"single branch" test for the feature against the base to see what is
uncovered from the entire effort.

quoted

Johannes Schindelin		rebase-merges: move labels' whitespace mangling into `label_oid()`
sequencer.c
867bc1d2 4539) }

Not sure why this closing parenthesis isn't covered :-D

quoted

Johannes Schindelin	4d17fd25 remote-curl: unbreak http.extraHeader with custom allocators
http.c
4d17fd25 417) string_list_clear(&extra_http_headers, 0);

I think it is okay to have that uncovered. This line is in the unlikely
code path where a user will try to unset the extra headers via `git -c
http.extraHeaders [...]`, i.e. without setting a value for that setting.

quoted

Johannes Schindelin	2e0afafe Add git-bundle: move objects and references by archive
builtin/bundle.c
2e0afafe 115) return 1;

I have to admit that I am slightly curious how this commit from 2007 ended
up in this list: 2e0afafebd8 (Add git-bundle: move objects and references
by archive, 2007-02-22). That's quite the blast of the past.

quoted

Johannes Schindelin	116d1fa6 vreportf(): avoid relying on stdio buffering
usage.c
116d1fa6 16) fprintf(stderr, "BUG!!! too long a prefix '%s'\n", prefix);
116d1fa6 17) abort();
116d1fa6 22) *p = '\0'; /* vsnprintf() failed, clip at prefix */

Those are defensive programming, so this is expected not to be covered.

I wonder why we are not using BUG() here (for the fprintf and abort).
 

quoted

Johannes Schindelin	d54dea77 fetch: let --jobs=<n> parallelize --multiple, too
builtin/fetch.c
d54dea77 113) fetch_parallel_config = git_config_int(k, v);
d54dea77 114) if (fetch_parallel_config < 0)
d54dea77 116) return 0;
d54dea77 1566) static int fetch_failed_to_start(struct strbuf *out, void *cb, void *task_cb)
d54dea77 1568) struct parallel_fetch_state *state = cb;
d54dea77 1569) const char *remote = task_cb;
d54dea77 1573) return 0;

I guess that yes, this points out that setting the number of parallel
fetches via `fetch.parallel` is untested... I am willing to leave it like
that until the time a regression is found in that code (if ever).

I believe that anything that is multi-threaded is untested in this build
target as to avoid issues with race conditions in the gcov data.

quoted

Johannes Schindelin	e145d993 rebase -r: support merge strategies other than `recursive`
sequencer.c
e145d993 3304) (!opts->strategy || !strcmp(opts->strategy, "recursive")) ?

I am a bit puzzled by this because the added test case in t3430 should
cover precisely this line.

It could be that we hit "!opts->strategy" and short-circuit the strcmp, which
would mark the line as uncovered.

quoted

Johannes Schindelin	e2683d51 Fix .git/ discovery at the root of UNC shares
setup.c
e2683d51 952)     !is_dir_sep(dir->buf[min_offset - 1])) {
e2683d51 953) strbuf_addch(dir, '/');
e2683d51 954) min_offset++;

I think this is only tested on Windows.

quoted

Johannes Schindelin	be5d88e1 test-tool run-command: learn to run (parts of) the testsuite
t/helper/test-run-command.c
be5d88e1 66) static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
be5d88e1 69) struct testsuite *suite = cb;
be5d88e1 71) if (suite->next >= suite->tests.nr)
be5d88e1 72) return 0;
be5d88e1 74) test = suite->tests.items[suite->next++].string;
be5d88e1 75) argv_array_pushl(&cp->args, "sh", test, NULL);
be5d88e1 76) if (suite->quiet)
be5d88e1 77) argv_array_push(&cp->args, "--quiet");
be5d88e1 78) if (suite->immediate)
be5d88e1 79) argv_array_push(&cp->args, "-i");
be5d88e1 80) if (suite->verbose)
be5d88e1 81) argv_array_push(&cp->args, "-v");
be5d88e1 82) if (suite->verbose_log)
be5d88e1 83) argv_array_push(&cp->args, "-V");
be5d88e1 84) if (suite->trace)
be5d88e1 85) argv_array_push(&cp->args, "-x");
be5d88e1 86) if (suite->write_junit_xml)
be5d88e1 87) argv_array_push(&cp->args, "--write-junit-xml");
be5d88e1 89) strbuf_addf(err, "Output of '%s':\n", test);
be5d88e1 90) *task_cb = (void *)test;
be5d88e1 92) return 1;
be5d88e1 95) static int test_finished(int result, struct strbuf *err, void *cb,
be5d88e1 98) struct testsuite *suite = cb;
be5d88e1 99) const char *name = (const char *)task_cb;
be5d88e1 101) if (result)
be5d88e1 102) string_list_append(&suite->failed, name);
be5d88e1 104) strbuf_addf(err, "%s: '%s'\n", result ? "FAIL" : "SUCCESS", name);
be5d88e1 106) return 0;
be5d88e1 109) static int test_failed(struct strbuf *out, void *cb, void *task_cb)
be5d88e1 111) struct testsuite *suite = cb;
be5d88e1 112) const char *name = (const char *)task_cb;
be5d88e1 114) string_list_append(&suite->failed, name);
be5d88e1 115) strbuf_addf(out, "FAILED TO START: '%s'\n", name);
be5d88e1 117) return 0;
be5d88e1 125) static int testsuite(int argc, const char **argv)
be5d88e1 127) struct testsuite suite = TESTSUITE_INIT;
be5d88e1 128) int max_jobs = 1, i, ret;
be5d88e1 131) struct option options[] = {
be5d88e1 145) memset(&suite, 0, sizeof(suite));
be5d88e1 146) suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
be5d88e1 148) argc = parse_options(argc, argv, NULL, options,
be5d88e1 151) if (max_jobs <= 0)
be5d88e1 152) max_jobs = online_cpus();
be5d88e1 154) dir = opendir(".");
be5d88e1 155) if (!dir)
be5d88e1 157) while ((d = readdir(dir))) {
be5d88e1 158) const char *p = d->d_name;
be5d88e1 160) if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
be5d88e1 161)     !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
be5d88e1 162)     !ends_with(p, ".sh"))
be5d88e1 163) continue;
be5d88e1 166) if (!argc) {
be5d88e1 167) string_list_append(&suite.tests, p);
be5d88e1 168) continue;
be5d88e1 171) for (i = 0; i < argc; i++)
be5d88e1 172) if (!wildmatch(argv[i], p, 0)) {
be5d88e1 173) string_list_append(&suite.tests, p);
be5d88e1 174) break;
be5d88e1 177) closedir(dir);
be5d88e1 179) if (!suite.tests.nr)
be5d88e1 181) if (max_jobs > suite.tests.nr)
be5d88e1 182) max_jobs = suite.tests.nr;
be5d88e1 184) fprintf(stderr, "Running %d tests (%d at a time)\n",
be5d88e1 187) ret = run_processes_parallel(max_jobs, next_test, test_failed,
be5d88e1 190) if (suite.failed.nr > 0) {
be5d88e1 191) ret = 1;
be5d88e1 192) fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
be5d88e1 193) for (i = 0; i < suite.failed.nr; i++)
be5d88e1 194) fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
be5d88e1 197) string_list_clear(&suite.tests, 0);
be5d88e1 198) string_list_clear(&suite.failed, 0);
be5d88e1 200) return !!ret;
be5d88e1 209) exit(testsuite(argc - 1, argv + 1));

This is exercised extensively in the Visual Studio job of our Azure
Pipeline.

Thanks,
DschO

Thank you!
-Stolee