I decided to take a stab at a full review of the test coverage report in
order to try and understand all of the uncovered code. The snippets I
highlight below include uncovered code that is not immediately obvious as
an acceptable block to leave uncovered. (Some snippets required looking
around at the context to know that is the case.)
In at least one case, I found a block that is actually covered in my
local testing, so something is wrong with the build environment I use
to generate this report. I'm currently investigating.
On 5/30/2019 8:52 AM, Derrick Stolee wrote:
> blame.c
> 170072f9 846) (result[i] >= most_certain_line_a ||
> 170072f9 847) second_best_result[i] >= most_certain_line_a)) {
> 170072f9 848) certainties[i] = CERTAINTY_NOT_CALCULATED;
This section appears in the following block:
/* More invalidating of results that may be affected by the choice of
* most certain line.
* Discard the matches for lines in B that are currently matched with a
* line in A such that their ordering contradicts the ordering imposed
* by the choice of most certain line.
*/
for (i = most_certain_local_line_b - 1; i >= invalidate_min; --i) {
/* In this loop we discard results for lines in B that are
* before most-certain-line-B but are matched with a line in A
* that is after most-certain-line-A.
*/
if (certainties[i] >= 0 &&
(result[i] >= most_certain_line_a ||
second_best_result[i] >= most_certain_line_a)) {
certainties[i] = CERTAINTY_NOT_CALCULATED;
}
}
for (i = most_certain_local_line_b + 1; i < invalidate_max; ++i) {
/* In this loop we discard results for lines in B that are
* after most-certain-line-B but are matched with a line in A
* that is before most-certain-line-A.
*/
if (certainties[i] >= 0 &&
(result[i] <= most_certain_line_a ||
second_best_result[i] <= most_certain_line_a)) {
certainties[i] = CERTAINTY_NOT_CALCULATED;
}
}
Note that the first for loop includes the uncovered lines. The logical operands
are backwards of the conditions in the second for loop, which are covered. This
seems non-trivial enough to merit a test.
> 170072f9 951) max_search_distance_b = 0;
> 1fc73384 998) return;
> 8934ac8c 1190) ent->ignored == next->ignored &&
> 8934ac8c 1191) ent->unblamable == next->unblamable) {
These lines are part of this diff:
--- a/blame.c
+++ b/blame.c
@@ -479,7 +479,9 @@ void blame_coalesce(struct blame_scoreboard *sb)
for (ent = sb->ent; ent && (next = ent->next); ent = next) {
if (ent->suspect == next->suspect &&
- ent->s_lno + ent->num_lines == next->s_lno) {
+ ent->s_lno + ent->num_lines == next->s_lno &&
+ ent->ignored == next->ignored &&
+ ent->unblamable == next->unblamable) {
ent->num_lines += next->num_lines;
ent->next = next->next;
blame_origin_decref(next->suspect);
The fact that they are uncovered means that the && chain is short-circuited at
"ent->s_lno + ent->num_lines == next->s_lno" before the new conditions can be
checked. So, the block inside is never covered. It includes a call to
blame_origin_decref() and free(), so it would be good to try and exercise this region.
> http.c
> ee334603 2302) target ? hash_to_hex(target->hash) : base_url,
This line being uncovered means that 'target' is never NULL. In the code above,
base_url is used in all cases so this is safe enough.
> promisor-remote.c
> 7bdf0926 93) previous->next = r->next;
This isn't being hit because "previous" is always NULL in the call to
promisor_remote_move_to_tail(), which is filled by a call to
promisor_remote_lookup(). All of this code is rather difficult to read
(double pointers, for loops with two iterator variables) so it is hard
to do the mental math and guarantee that it is working.
I tried playing around with adding more promisor remotes to t0410-partial-clone.sh,
but could not get this line to hit.
> dcc8b4e9 202) static int remove_fetched_oids(struct object_id **oids, int oid_nr, int to_free)
This method isn't covered at all, so I responded directly to the patch thread.
> upload-pack.c
> a8d662e3 355) send_client_data(1, output_state.buffer, output_state.used);
This line looks like a copy-paste from a method refactor.
> 820a5361 1386) string_list_clear(&data->uri_protocols, 0);
This string_list_clear() is preceded by
if (data->uri_protocols.nr && !data->writer.use_sideband)
but earlier is populated by
if (skip_prefix(arg, "packfile-uris ", &p)) {
string_list_split(&data->uri_protocols, p, ',', -1);
continue;
}
Why don't we simply not use string_list_split() if !data->writer.use_sideband?
I would apply this diff to avoid calling string_list_split at all:
diff --git a/upload-pack.c b/upload-pack.c
index db74ca57bd..267b419521 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1373,7 +1373,7 @@ static void process_args(struct packet_reader *request,
continue;
}
- if (skip_prefix(arg, "packfile-uris ", &p)) {
+ if (skip_prefix(arg, "packfile-uris ", &p) && data->writer.use_sideband) {
string_list_split(&data->uri_protocols, p, ',', -1);
continue;
}
@@ -1381,9 +1381,6 @@ static void process_args(struct packet_reader *request,
/* ignore unknown lines maybe? */
die("unexpected line: '%s'", arg);
}
-
- if (data->uri_protocols.nr && !data->writer.use_sideband)
- string_list_clear(&data->uri_protocols, 0);
}
static int process_haves(struct oid_array *haves, struct oid_array *common,
> commit-graph.c
> 93ba1867 969) display_progress(ctx->progress, ctx->approx_nr_objects);
This line seemed suspicious, but is preceded by
if (ctx->progress_done < ctx->approx_nr_objects)
so is pretty harmless to leave uncovered.
> builtin/fast-export.c
> e80001f8 81) static int parse_opt_reencode_mode(const struct option *opt,
I'm always suspicious of a method that is never called by the test suite.
The only caller is given by this portion of the patch:
+ OPT_CALLBACK(0, "reencode", &reencode_mode, N_("mode"),
+ N_("select handling of commit messages in an alternate encoding"),
+ parse_opt_reencode_mode),
But we DO have tests that cover this flag, and inserting a die() in the
method triggers it on t9350-fast-export.sh. I'll investigate what went wrong
on the build [1] to cause this. I see a lot of these in the logs:
sh: echo: I/O error
So maybe some tests did not actually run. Further, these tests failed:
t3400-rebase.sh (Wstat: 256 Tests: 28 Failed: 2)
Failed tests: 20, 28
Non-zero exit status: 1
t3420-rebase-autostash.sh (Wstat: 256 Tests: 38 Failed: 6)
Failed tests: 6, 13, 16, 23, 26, 33
Non-zero exit status: 1
t3404-rebase-interactive.sh (Wstat: 256 Tests: 110 Failed: 5)
Failed tests: 3, 9-10, 100-101
Non-zero exit status: 1
t5521-pull-options.sh (Wstat: 256 Tests: 19 Failed: 1)
Failed test: 3
Non-zero exit status: 1
t5551-http-fetch-smart.sh (Wstat: 256 Tests: 37 Failed: 1)
Failed test: 26
Non-zero exit status: 1
They don't fail locally, so perhaps we shouldn't blindly trust the coverage data
until I work out why these errors occurred. (Many of the cases I called out
above I couldn't hit locally with a die() statement.)
[1] https://dev.azure.com/git/git/_build/results?buildId=606
> builtin/rebase.c
> 4c785c0e 1201) opts->flags &= ~REBASE_DIFFSTAT;
This is the only changed line in this commit, which the commit message states was
found by static analysis.
> fast-import.c
> 3edfcc65 2612) read_next_command();
> 3edfcc65 2679) strbuf_addf(&new_data,
I expect this is actually covered, but wasn't reported due to the issues listed above.
> progress.c
> 1aed1a5f 131) cols - progress->title_len - 1 : 0;
This is the only changed line in this commit.
> read-cache.c
> 7bd9631b 2201) src_offset += load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);
This was previously uncovered and this commit simply changed the method prototype.
> refs.c
> 1de16aec 111) sanitized->buf[sanitized->len-1] = '-';
> 1de16aec 170) if (sanitized)
> 1de16aec 171) strbuf_addch(sanitized, '-');
> 1de16aec 173) return -1;
> 1de16aec 178) strbuf_complete(sanitized, '/');
These are special cases of custom ref types used in "git worktree add" and some
of them are covered by tests, but these seem harmless.
Thanks,
-Stolee
