Git Test Coverage Report (Thursday, May 30th)

Git Test Coverage Report (Thursday, May 30th)

[Derrick Stolee]

Here is today's test coverage report. You can view it in HTML [1] or
download the plain-text version [2], also pasted below.

Thanks,
-Stolee

[1] https://derrickstolee.github.io/git-test-coverage/reports/2019-05-30.htm
[2] https://derrickstolee.github.io/git-test-coverage/reports/2019-05-30.txt

---

pu	1feae2a559816e6df62e994d4ffdc226dee6631a
jch	1605b8c0b5622510a96ef6a358adbd142cf8222e
next	5d7573a151b918062087822a2f0d7661dc4bd707
master	aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6
master@{1}	ab15ad1a3b4b04a29415aef8c9afa2f64fc194a2


Uncovered code in 'pu' not in 'jch'
--------------------------------------------------------

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;
170072f9 951) max_search_distance_b = 0;
1fc73384 998) return;
8934ac8c 1190)     ent->ignored == next->ignored &&
8934ac8c 1191)     ent->unblamable == next->unblamable) {
43885768 1607) continue;
ae3f36de 2425) continue;

builtin/blame.c

builtin/cat-file.c

builtin/fetch-pack.c

builtin/pack-objects.c

builtin/rebase.c
d559f502 759) ret = error(_("could not remove '%s'"),
d559f502 1675) error(_("could not remove '%s'"),

builtin/remote.c
f39a9c65 1551) die(_("--save-to-push cannot be used with other options"));
f39a9c65 1575) die(_("--save-to-push can only be used when only one url is defined"));

fetch-pack.c

http-fetch.c

http.c
ee334603 2302) target ? hash_to_hex(target->hash) : base_url,

oidset.c

promisor-remote.c
0ba08c05 25) die(_("Remote with no URL"));
54248706 61) warning(_("promisor remote name cannot begin with '/': %s"),
54248706 63) return NULL;
7bdf0926 93) previous->next = r->next;
7b6e1b04 108) return git_config_string(&core_partial_clone_filter_default,
b21a55f3 139) return 0;
dcc8b4e9 202) static int remove_fetched_oids(struct object_id **oids, int oid_nr, int to_free)
dcc8b4e9 204) int i, missing_nr = 0;
dcc8b4e9 205) int *missing = xcalloc(oid_nr, sizeof(*missing));
dcc8b4e9 206) struct object_id *old_oids = *oids;
dcc8b4e9 208) int old_fetch_if_missing = fetch_if_missing;
dcc8b4e9 210) fetch_if_missing = 0;
dcc8b4e9 212) for (i = 0; i < oid_nr; i++)
dcc8b4e9 213) if (oid_object_info_extended(the_repository, &old_oids[i], NULL, 0)) {
dcc8b4e9 214) missing[i] = 1;
dcc8b4e9 215) missing_nr++;
dcc8b4e9 218) fetch_if_missing = old_fetch_if_missing;
dcc8b4e9 220) if (missing_nr) {
dcc8b4e9 221) int j = 0;
dcc8b4e9 222) new_oids = xcalloc(missing_nr, sizeof(*new_oids));
dcc8b4e9 223) for (i = 0; i < oid_nr; i++)
dcc8b4e9 224) if (missing[i])
dcc8b4e9 225) oidcpy(&new_oids[j++], &old_oids[i]);
dcc8b4e9 226) *oids = new_oids;
dcc8b4e9 227) if (to_free)
dcc8b4e9 228) free(old_oids);
dcc8b4e9 231) free(missing);
dcc8b4e9 233) return missing_nr;
dcc8b4e9 248) if (missing_nr == 1)
dcc8b4e9 249) continue;
dcc8b4e9 250) missing_nr = remove_fetched_oids(&missing_oids, missing_nr, to_free);
dcc8b4e9 251) if (missing_nr) {
dcc8b4e9 252) to_free = 1;
dcc8b4e9 253) continue;
dcc8b4e9 261) free(missing_oids);

protocol.c

ref-filter.c
2582083f 93) keydata_aka_refname ? keydata_aka_refname : k->wt->head_ref);

remote-curl.c

sequencer.c
37e9ee5c 293) ret = -1;
37e9ee5c 311) ret = error(_("could not remove '%s'"), buf.buf);

upload-pack.c
a8d662e3 130) return readsz;
820a5361 149) BUG("packfile_uris requires sideband-all");
a8d662e3 355) send_client_data(1, output_state.buffer, output_state.used);
820a5361 1386) string_list_clear(&data->uri_protocols, 0);

wrapper.c

Commits introducting uncovered code:
Barret Rhoden	1fc73384 blame: optionally track line fingerprints during fill_blame_origin()
Barret Rhoden	8934ac8c blame: add config options for the output of ignored or unblamable lines
Barret Rhoden	43885768 blame: use the fingerprint heuristic to match ignored lines
Barret Rhoden	ae3f36de blame: add the ability to ignore commits and their changes
Christian Couder	7b6e1b04 Move core_partial_clone_filter_default to promisor-remote.c
Christian Couder	dcc8b4e9 promisor-remote: implement promisor_remote_get_direct()
Christian Couder	b21a55f3 promisor-remote: parse remote.*.partialclonefilter
Christian Couder	7bdf0926 promisor-remote: use repository_format_partial_clone
Christian Couder	54248706 Add initial support for many promisor remotes
Christian Couder	0ba08c05 Remove fetch-object.{c,h} in favor of promisor-remote.{c,h}
Denton Liu	f39a9c65 remote: add --save-to-push option to git remote set-url
Jonathan Tan	a8d662e3 upload-pack: refactor reading of pack-objects out
Jonathan Tan	820a5361 upload-pack: send part of packfile response as uri
Junio C Hamano	ee334603 Merge branch 'jt/fetch-cdn-offload' into pu
Michael Platings	170072f9 blame: add a fingerprint heuristic to match ignored lines
Nickolai Belakovski	2582083f ref-filter: add worktreepath atom
Phillip Wood	d559f502 rebase --abort/--quit: cleanup refs/rewritten
Phillip Wood	37e9ee5c sequencer: return errors from sequencer_remove_state()


Uncovered code in 'jch' not in 'next'
--------------------------------------------------------

builtin/branch.c
1fde99cf 833) die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"

builtin/checkout.c

builtin/commit.c

builtin/gc.c
efeb229e 691) return 1;

builtin/rebase.c
526c03b5 1262) goto done;
10572de1 1278) goto done;

commit-graph.c
d83160e8 906) error(_("error opening index for %s"), packname.buf);
d83160e8 907) return 1;
63a8be62 946) continue;
93ba1867 969) display_progress(ctx->progress, ctx->approx_nr_objects);
8520d7fc 1039) error(_("unable to create leading directories of %s"),
8520d7fc 1041) return errno;
efeb229e 1158) error(_("the commit graph format cannot write %d commits"), count_distinct);
efeb229e 1159) res = 1;
efeb229e 1160) goto cleanup;
efeb229e 1169) error(_("too many commits to write graph"));
efeb229e 1170) res = 1;
efeb229e 1171) goto cleanup;

read-cache.c
ee70c128 1723) if (advice_unknown_index_extension) {
ee70c128 1724) warning(_("ignoring optional %.4s index extension"), ext);
ee70c128 1725) advise(_("This is likely due to the file having been written by a newer\n"

Commits introducting uncovered code:
Denton Liu	526c03b5 rebase: refactor can_fast_forward into goto tower
Denton Liu	10572de1 rebase: fast-forward --onto in more cases
Derrick Stolee	efeb229e commit-graph: return with errors during write
Derrick Stolee	d83160e8 commit-graph: extract fill_oids_from_packs()
Derrick Stolee	63a8be62 commit-graph: extract fill_oids_from_commit_hex()
Derrick Stolee	93ba1867 commit-graph: extract fill_oids_from_all_packs()
Derrick Stolee	8520d7fc commit-graph: extract write_commit_graph_file()
Jonathan Nieder	ee70c128 index: offer advice for unknown index extensions
Philip Oakley	1fde99cf doc branch: provide examples for listing remote tracking branches


Uncovered code in 'next' not in 'master'
--------------------------------------------------------

builtin/am.c
97387c8b 1662) die("unable to read from stdin; aborting");
6e7baf24 2336) die(_("interactive mode requires patches on the command line"));

builtin/bisect--helper.c
7877ac3d 574) retval = error(_("invalid ref: '%s'"), start_head.buf);
7877ac3d 575) goto finish;

builtin/fast-export.c
e80001f8 81) static int parse_opt_reencode_mode(const struct option *opt,
e80001f8 84) if (unset) {
e80001f8 85) reencode_mode = REENCODE_ABORT;
e80001f8 86) return 0;
e80001f8 89) switch (git_parse_maybe_bool(arg)) {
e80001f8 91) reencode_mode = REENCODE_NO;
e80001f8 92) break;
e80001f8 94) reencode_mode = REENCODE_YES;
e80001f8 95) break;
e80001f8 97) if (!strcasecmp(arg, "abort"))
e80001f8 98) reencode_mode = REENCODE_ABORT;
e80001f8 100) return error("Unknown reencoding mode: %s", arg);
e80001f8 103) return 0;
e80001f8 665) switch(reencode_mode) {
e80001f8 667) reencoded = reencode_string(message, "UTF-8", encoding);
e80001f8 668) break;
e80001f8 670) break;
e80001f8 672) die("Encountered commit-specific encoding %s in commit "
e80001f8 674)     encoding, oid_to_hex(&commit->object.oid));
ccbfc96d 686) printf("encoding %s\n", encoding);

builtin/index-pack.c
8a30a1ef 1365) continue;

builtin/log.c
13cdf780 873) return 0;

builtin/merge.c
f3f8311e 1290) usage_msg_opt(_("--quit expects no arguments"),

builtin/rebase.c
4c785c0e 1201) opts->flags &= ~REBASE_DIFFSTAT;

builtin/worktree.c
1de16aec 297) BUG("How come '%s' becomes empty after sanitization?", sb.buf);

builtin/write-tree.c
76a7bc09 53) die("%s: prefix %s not found", me, tree_prefix);

diff.c
8ef05193 5217) return error(_("%s expects a numerical value"), "--unified");

fast-import.c
3edfcc65 2612) read_next_command();
3edfcc65 2679) strbuf_addf(&new_data,

grep.c
de99eb0c 1784) BUG("grep call which could print a name requires "

parse-options-cb.c
f7e68a08 20) return error(_("option `%s' expects a numerical value"),

parse-options.c
f7e68a08 199) return error(_("%s expects a numerical value"),

progress.c
1aed1a5f 131)     cols - progress->title_len - 1 : 0;

read-cache.c
7bd9631b 2201) src_offset += load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);

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, '/');
1de16aec 215) BUG("sanitizing refname '%s' check returned error", refname);

server-info.c
f4f476b6 110) ret = -1;
f4f476b6 111) goto out;
f4f476b6 123) goto out;
f4f476b6 125) goto out;
f4f476b6 134) if (uic.cur_fp)
f4f476b6 135) fclose(uic.cur_fp);

trace2/tr2_tls.c
5fdae9d3 87) return pthread_getspecific(tr2tls_key) == tr2tls_thread_main;

Commits introducting uncovered code:
Denton Liu	13cdf780 format-patch: teach format.notes config option
Elijah Newren	e80001f8 fast-export: do automatic reencoding of commit messages only if requested
Elijah Newren	ccbfc96d fast-export: avoid stripping encoding header if we cannot reencode
Elijah Newren	3edfcc65 fast-import: support 'encoding' commit header
Emily Shaffer	de99eb0c grep: fail if call could output and name is null
Eric Wong	f4f476b6 update-server-info: avoid needless overwrites
Jeff Hostetler	5fdae9d3 trace2: fix tracing when NO_PTHREADS is defined
Jeff King	7bd9631b read-cache: drop unused parameter from threaded load
Jeff King	76a7bc09 cmd_{read,write}_tree: rename "unused" variable that is used
Jeff King	97387c8b am: read interactive input from stdin
Jeff King	6e7baf24 am: drop tty requirement for --interactive
Johannes Schindelin	7877ac3d bisect--helper: verify HEAD could be parsed before continuing
Johannes Schindelin	4c785c0e rebase: replace incorrect logical negation by correct bitwise one
Jonathan Tan	8a30a1ef index-pack: prefetch missing REF_DELTA bases
Nguyễn Thái Ngọc Duy	f3f8311e merge: add --quit
Nguyễn Thái Ngọc Duy	8ef05193 diff-parseopt: restore -U (no argument) behavior
Nguyễn Thái Ngọc Duy	f7e68a08 parse-options: check empty value in OPT_INTEGER and OPT_ABBREV
Nguyễn Thái Ngọc Duy	1de16aec worktree add: sanitize worktree names
SZEDER Gábor	1aed1a5f progress: avoid empty line when breaking the progress line


Uncovered code in 'master' not in 'master@{1}'
--------------------------------------------------------

builtin/difftool.c
f3a3a021 731) die(_("--dir-diff is incompatible with --no-index"));

http.c
5c3d5a38 2329) return posn / eltsize;

parse-options.c
5c387428 282) return 0;
5c387428 481) BUG("OPT_ALIAS() should not remain at this point. "
5c387428 661) BUG("An alias must have long option name");
5c387428 670) BUG("No please. Nested aliases are not supported.");
5c387428 685) BUG("could not find source option '%s' of alias '%s'",

sha1-name.c
af96fe33 161) return;

t/helper/test-read-cache.c
dc76852d 29) die("%s not in index", name);

upload-pack.c
8e712ef6 1073) precomposed_unicode = git_config_bool(var, value);

Commits introducting uncovered code:
Derrick Stolee	af96fe33 midx: add packs to packed_git linked list
Elijah Newren	8e712ef6 Honor core.precomposeUnicode in more places
Johannes Schindelin	f3a3a021 difftool --no-index: error out on --dir-diff (and don't crash)
Johannes Schindelin	dc76852d fsmonitor: demonstrate that it is not refreshed after discard_index()
Mike Hommey	5c3d5a38 Make fread/fwrite-like functions in http.c more like fread/fwrite.
Nguyễn Thái Ngọc Duy	5c387428 parse-options: don't emit "ambiguous option" for aliases

Re: Git Test Coverage Report (Thursday, May 30th)

[Derrick Stolee]

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

Re: Git Test Coverage Report (Thursday, May 30th)

[Derrick Stolee]

On 5/30/2019 2:24 PM, Derrick Stolee wrote:
> 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.)

These tests all failed during the second run that set optional GIT_TEST
environment variables. Specifically, GIT_TEST_REBASE_USE_BUILTIN=false
caused these tests to break. We now output this message:

	warning: the rebase.useBuiltin support has been removed!
	See its entry in 'git help config' for details.

I'm removing that variable from the build definition.

Thanks,
-Stolee

Re: Git Test Coverage Report (Thursday, May 30th)

[Johannes Schindelin]

Hi Stolee,

On Fri, 31 May 2019, Derrick Stolee wrote:

> On 5/30/2019 2:24 PM, Derrick Stolee wrote:
> > 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.)
>
> These tests all failed during the second run that set optional GIT_TEST
> environment variables. Specifically, GIT_TEST_REBASE_USE_BUILTIN=false
> caused these tests to break. We now output this message:
>
> 	warning: the rebase.useBuiltin support has been removed!
> 	See its entry in 'git help config' for details.
>
> I'm removing that variable from the build definition.

Would it make sense to have a file in t/ (or a script-let in ci/)
specifying all of the `GIT_TEST_*` variables that are currently supported
(and that actually make sense to be set)?

I saw a similar issue recently in a now-defunct Azure Pipeline that also
tried to replicate what half of the `linux-gcc` job [*1*] does: to run the
test suite with those variables overriding the defaults. That Pipeline
broke for the exact same reason you mentioned: we now handle
`GIT_TEST_REBASE_USE_BUILTIN` by showing that warning.

And issues like this could easily be avoided if we had, say,
`ci/non-standard-settings.sh` that simply set all those `GIT_TEST_*`
variables in the way that the `linux-gcc` job does (and of course, this
job should then source that file instead of duplicating those
assignments).

What do you think?
Dscho

Footnote *1*: It is a thorn in my side ever since I started work on our
Azure Pipeline support that the `linux-gcc` job actually runs *two* jobs:
it runs the vanilla test suite, and then it runs it again after setting
all supported `GIT_TEST_*` variables to the non-default settings. This
almost doubles the running time of that job, often making it the very last
job to finish, and it also makes it unclear whether a test failure stems
from said `GIT_TEST_*` settings or not.

I got so annoyed by this, in fact, that I finally broke down and opened
https://github.com/gitgitgadget/git/issues/242.

Re: Git Test Coverage Report (Thursday, May 30th)

[Michael Platings]

Thanks very much for this Derrick. I looked into it and it turns out
that the missing coverage in blame.c for "certainties[i] =
CERTAINTY_NOT_CALCULATED" was due to earlier code overwriting the same
value in most cases, thereby defeating an optimization. I've deleted
that earlier code and now coverage is as expected. I posted the patch
here: https://public-inbox.org/git/20190601210925.15339-1-michael@platin.gs/T/#u
I also deleted the other uncovered code that appeared in the same
patch as it was unreachable.

> 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.

Re: Git Test Coverage Report (Thursday, May 30th)

[Barret Rhoden]

Hi -

On 5/30/19 2:24 PM, Derrick Stolee wrote:
>> 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.

What is your setup for determining if a line is uncovered?  Are you 
running something like gcov for all of the tests in t/?

I removed this change, and none of the other blame tests appeared to 
trigger this code block either, independently of this change.  (I put an 
assert(0) inside the block).

However, two of our blame-ignore tests do get past the first two checks 
in the if clause, (the suspects are equal and the s_lno chunks are 
adjacent) and we do check the ignored/unblamable conditions.

Specifically, if I undo this change and put an assert(0) in that block, 
two of our tests hit that code, and one of our tests fails if I don't do 
the check for ignored/unblamable.

Thanks,
Barret

Re: Git Test Coverage Report (Thursday, May 30th)

[Derrick Stolee]

On 6/3/2019 2:11 PM, Barret Rhoden wrote:
> Hi -
> 
> On 5/30/19 2:24 PM, Derrick Stolee wrote:
>>> 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.
> 
> What is your setup for determining if a line is uncovered?  Are you running something like gcov for all of the tests in t/?
> 
> I removed this change, and none of the other blame tests appeared to trigger this code block either, independently of this change.  (I put an assert(0) inside the block).
> 
> However, two of our blame-ignore tests do get past the first two checks in the if clause, (the suspects are equal and the s_lno chunks are adjacent) and we do check the ignored/unblamable conditions.
> 
> Specifically, if I undo this change and put an assert(0) in that block, two of our tests hit that code, and one of our tests fails if I don't do the check for ignored/unblamable.

The tests use gcov while running the tests in t/. Here is the build [1].

There are some i/o errors happening in the build, which I have not
full diagnosed. It is entirely possible that you actually are covered,
but there was an error collecting the coverage statistics. The simplest
thing to do is to insert a die() statement and re-run the tests.

Thanks,
-Stolee

[1] https://dev.azure.com/git/git/_build/results?buildId=615

Re: Git Test Coverage Report (Thursday, May 30th)

[Barret Rhoden]

On 6/3/19 2:40 PM, Derrick Stolee wrote:
> On 6/3/2019 2:11 PM, Barret Rhoden wrote:
>> Hi -
>>
>> On 5/30/19 2:24 PM, Derrick Stolee wrote:
>>>> 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.
>>
>> What is your setup for determining if a line is uncovered?  Are you running something like gcov for all of the tests in t/?
>>
>> I removed this change, and none of the other blame tests appeared to trigger this code block either, independently of this change.  (I put an assert(0) inside the block).
>>
>> However, two of our blame-ignore tests do get past the first two checks in the if clause, (the suspects are equal and the s_lno chunks are adjacent) and we do check the ignored/unblamable conditions.
>>
>> Specifically, if I undo this change and put an assert(0) in that block, two of our tests hit that code, and one of our tests fails if I don't do the check for ignored/unblamable.
> 
> The tests use gcov while running the tests in t/. Here is the build [1].
> 
> There are some i/o errors happening in the build, which I have not
> full diagnosed. It is entirely possible that you actually are covered,
> but there was an error collecting the coverage statistics. The simplest
> thing to do is to insert a die() statement and re-run the tests.

It looks like no existing tests cover that block in blame_coalesce(), 
regardless of my commit.  That's based on putting die() in there and 
running make in t/.  So at the worst, my patch isn't decreasing 
coverage.  That's a pretty low bar.  =)

I'll try to come up with a test, independent of my blame-ignore work, 
that can get in that block.

Thanks,

Barret

Re: Git Test Coverage Report (Thursday, May 30th)

[Barret Rhoden]

Hi -

On 6/4/19 12:38 PM, Barret Rhoden wrote:
> I'll try to come up with a test, independent of my blame-ignore work, 
> that can get in that block.

I have a test that covers blame_coalesce(), which works both with and 
without my blame-ignore commit that started this thread.

However, the only thing we are really testing is that git blame didn't 
crash.  There is no detectable change to the output.  AFAIK, 
blame_coalesce() is a performance enhancement.

If you all are interested in that sort of test, I can put it in a patch. 
  Right now, I have this (below).

Thanks,

Barret


diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index c92a47b6d5b1..4c652b85a55b 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -275,4 +275,32 @@ test_expect_success 'blame file with CRLF 
core.autocrlf=true' '
  	grep "A U Thor" actual
  '

+test_expect_success 'blame coalesce' '
+	cat >giraffe <<-\EOF &&
+	ABC
+	DEF
+	EOF
+	git add giraffe &&
+	git commit -m "original file" &&
+	
+	cat >giraffe <<-\EOF &&
+	ABC
+	XXX
+	DEF
+	EOF
+	git add giraffe &&
+	git commit -m "interior XXX line" &&
+	
+	cat >giraffe <<-\EOF &&
+	ABC
+	DEF
+	EOF
+	git add giraffe &&
+	git commit -m "same contents as original" &&
+
+	git blame giraffe &&
+
+	true
+'
+
  test_done

Re: Git Test Coverage Report (Thursday, May 30th)

[Derrick Stolee]

On 6/4/2019 4:41 PM, Barret Rhoden wrote:
> Hi -
> 
> On 6/4/19 12:38 PM, Barret Rhoden wrote:
> However, the only thing we are really testing is that git blame didn't crash. 

This would not be enough.

> There is no detectable change to the output.  AFAIK, blame_coalesce() is a performance enhancement.

Thank you for stating that the output didn't change. I
tested this locally, and did see that the behavior was
identical.

I think you should just make the test be complete by
checking a post-condition. Please see the inserted lines
below (which _should_ work, I haven't actually ran this
in the test suite).

> +test_expect_success 'blame coalesce' '
> +    cat >giraffe <<-\EOF &&
> +    ABC
> +    DEF
> +    EOF
> +    git add giraffe &&
> +    git commit -m "original file" &&

oid=$(git rev-parse HEAD) &&

> +   
> +    cat >giraffe <<-\EOF &&
> +    ABC
> +    XXX
> +    DEF
> +    EOF
> +    git add giraffe &&
> +    git commit -m "interior XXX line" &&
> +   
> +    cat >giraffe <<-\EOF &&
> +    ABC
> +    DEF
> +    EOF
> +    git add giraffe &&
> +    git commit -m "same contents as original" &&
> +

cat >expect <<-\EOF &&
^$oid 1) ABC
^$oid 2) DEF
EOF
git -c core.abbrev=40 blame -s giraffe >actual &&
test_cmp expect actual

> +'
> +
>  test_done
> 

Re: Git Test Coverage Report (Thursday, May 30th)

[Barret Rhoden]

On 6/4/19 8:57 PM, Derrick Stolee wrote:
[snip]
> I think you should just make the test be complete by
> checking a post-condition. Please see the inserted lines
> below (which _should_ work, I haven't actually ran this
> in the test suite).

With a little massaging, this did the trick.  I'll roll the patch into 
my blame-ignore series, to keep things simple.

Thanks,

Barret