Git Test Coverage Report (April 30, 2020)

Git Test Coverage Report (April 30, 2020)

[Derrick Stolee]

Hello,

Here is today's test coverage report.

It appears that _all_ of the reftable code shows up in this report as
untested. Perhaps that is the state of the world right now, or perhaps
I need to initialize a GIT_TEST_ environment variable to ensure it runs?
In either way, do we have any test coverage of that massive contribution?
Please take a look at the online report [1] for that part of the report,
as I snipped it out of this email.

Thanks,
-Stolee

[1] https://derrickstolee.github.io/git-test-coverage/reports/2020-04-30-commits.txt
[2] https://derrickstolee.github.io/git-test-coverage/reports/2020-04-30.txt
[3] https://derrickstolee.github.io/git-test-coverage/reports/2020-04-30.htm

---

pu	50ce81fe7726b7eea90e201c1d31567158bfe4c1
jch	a2998880e147321b718420ae805807ef5fb483f3
next	6140810f5a86d69e4bee74d360b936fb4a8a3b1f
master	86ab15cb154862b6fa5cc646dac27532f881e1fb
master@{1}	efe3874640e2e58209ddbb3b072f48f6b7094f34


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

Commits introducing uncovered code:
Carlo Marcelo Arenas Belón	555c4709 credential-store: warn instead of fatal for bogus lines from store
credential-store.c
555c4709 27) strbuf_reset(&redacted_line);

[...snipped all reftable code...]

Jiang Xin	2c26d674 receive-pack: new config receive.procReceiveRefs
builtin/receive-pack.c
2c26d674 237) return config_error_nonbool(var);
2c26d674 1814) continue;

Jiang Xin	bc3e2729 receive-pack: add new proc-receive hook
builtin/receive-pack.c
bc3e2729 1021) return code;
bc3e2729 1024) proc.err = 0;
bc3e2729 1029) if (use_sideband)
bc3e2729 1030) finish_async(&muxer);
bc3e2729 1031) return code;
bc3e2729 2245) packet_buf_write(&buf, "ng %s %s%c%s\n",
bc3e2729 2246)  cmd->ref_name, cmd->error_string,

t/helper/test-proc-receive.c
bc3e2729 127) usage_msg_opt("Too many arguments.", proc_receive_usage, options);
bc3e2729 167) *p = '\0';
bc3e2729 168) p += 2;
bc3e2729 169) packet_write_fmt(1, "%s%c%s\n", item->string, '\0', p);

Jiang Xin	d131afc3 send-pack: extension for client-side status report
remote.c
d131afc3 2362) return;

transport-helper.c
d131afc3 756) status = REF_STATUS_UPTODATE;
d131afc3 757) FREE_AND_NULL(msg);
d131afc3 764) status = REF_STATUS_REJECT_ALREADY_EXISTS;
d131afc3 765) FREE_AND_NULL(msg);
d131afc3 768) status = REF_STATUS_REJECT_FETCH_FIRST;
d131afc3 769) FREE_AND_NULL(msg);
d131afc3 772) status = REF_STATUS_REJECT_NEEDS_FORCE;
d131afc3 773) FREE_AND_NULL(msg);
d131afc3 776) status = REF_STATUS_REJECT_STALE;
d131afc3 777) FREE_AND_NULL(msg);
d131afc3 780) forced = 1;

Miriam Rubio	9aadf776 bisect--helper: introduce new `write_in_file()` function
builtin/bisect--helper.c
9aadf776 91) return error_errno(_("cannot open file '%s' in mode '%s'"), path, mode);
9aadf776 96) fclose(fp);
9aadf776 97) return error_errno(_("could not write to file '%s'"), path);

Phillip Wood	cd5e0fd9 Revert "Revert "Merge branch 'ra/rebase-i-more-options'""
builtin/rebase.c
cd5e0fd9 834) argv_array_push(&am.args, "--ignore-whitespace");
cd5e0fd9 836) argv_array_push(&opts->git_am_opts, "--committer-date-is-author-date");
cd5e0fd9 838) argv_array_push(&opts->git_am_opts, "--ignore-date");

sequencer.c
cd5e0fd9 877) return NULL;
cd5e0fd9 1394) goto out;
cd5e0fd9 1398) goto out;
cd5e0fd9 1474) res = -1;
cd5e0fd9 1475) goto out;

Pranit Bauva	587c9fac bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
builtin/bisect--helper.c
587c9fac 493) return error_errno(_("failed to write to '%s'"), git_path_bisect_log());
587c9fac 522) return error_errno(_("could not open '%s' for appending"),

Pranit Bauva	f0bf9482 bisect--helper: finish porting `bisect_start()` to C
builtin/bisect--helper.c
f0bf9482 730) return BISECT_FAILED;
f0bf9482 751) res = BISECT_FAILED;
f0bf9482 763) res = BISECT_FAILED;
f0bf9482 775) res = BISECT_FAILED;

Pranit Bauva	05a69202 bisect--helper: reimplement `bisect_autostart` shell function in C
builtin/bisect--helper.c
05a69202 818) fprintf(stderr, _("You need to start by \"git bisect "
05a69202 821) if (!isatty(STDIN_FILENO))
05a69202 822) return 1;
05a69202 829) yesno = git_prompt(_("Do you want me to do it for you "
05a69202 831) if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))
05a69202 832) return 1;
05a69202 834) return bisect_start(terms, 0, NULL, 0);

Pranit Bauva	10520dbf bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
builtin/bisect--helper.c
10520dbf 869) return BISECT_FAILED;
10520dbf 885) oid_array_clear(&revs);
10520dbf 886) return BISECT_FAILED;

Taylor Blau	7cb91802 commit-graph.c: ensure graph layers respect core.sharedRepository
commit-graph.c
7cb91802 1591) return -1;



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

Commits introducing uncovered code:
Damien Robert	812a5889 remote.c: fix handling of %(push:remoteref)
remote.c
812a5889 1638) return NULL;
812a5889 1645) return branch->refname;
812a5889 1660) return NULL;
812a5889 1774) return NULL;

Johannes Schindelin	12294990 credential: handle `credential.<partial-URL>.<key>` again
urlmatch.c
12294990 581) retval = 0;



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

Commits introducing uncovered code:
Denton Liu	be1bb600 sequencer: make apply_autostash() accept a path
sequencer.c
be1bb600 5203) apply_autostash(rebase_path_autostash());

Denton Liu	65c425a2 sequencer: stop leaking buf
sequencer.c
65c425a2 2557) goto done_rebase_i;

Denton Liu	5b2f6d9c sequencer: make file exists check more efficient
sequencer.c
5b2f6d9c 431) warning_errno(_("could not read '%s'"), path);

Denton Liu	efcf6cf0 rebase: use read_oneliner()
builtin/rebase.c
efcf6cf0 625) } else if (!read_oneliner(&buf, state_dir_path("head", opts),

Denton Liu	f213f069 rebase: generify reset_head()
builtin/rebase.c
f213f069 880) reset_head(the_repository, &opts->orig_head, "checkout",
f213f069 881)    opts->head_name, 0,

Denton Liu	86ed00af rebase: use apply_autostash() from sequencer.c
builtin/rebase.c
86ed00af 1041) apply_autostash(state_dir_path("autostash", opts));

Denton Liu	b309a971 reset: extract reset_head() from rebase
reset.c
b309a971 37) ret = -1;
b309a971 38) goto leave_reset_head;
b309a971 43) goto leave_reset_head;
b309a971 66) goto leave_reset_head;
b309a971 72) goto leave_reset_head;
b309a971 77) goto leave_reset_head;
b309a971 81) ret = -1;
b309a971 82) goto leave_reset_head;
b309a971 90) goto leave_reset_head;
b309a971 109) } else if (old_orig)
b309a971 110) delete_ref(NULL, "ORIG_HEAD", old_orig, 0);

Derrick Stolee	e3696980 diff: halt tree-diff early after max_changes
tree-diff.c
e3696980 438) break;

Derrick Stolee	8918e379 revision: complicated pathspecs disable filters
revision.c
8918e379 658) return 1;
8918e379 662) return 1;

Derrick Stolee	0906ac2b blame: use changed-path Bloom filters
blame.c
0906ac2b 1281) return 1;
0906ac2b 1302) bd->alloc *= 2;
0906ac2b 1303) REALLOC_ARRAY(bd->keys, bd->alloc);
0906ac2b 2899) return;

Đoàn Trần Công Danh	c933b28d date.c: s/is_date/set_date/
date.c
c933b28d 521) return -1;
c933b28d 532) return -1;

Đoàn Trần Công Danh	4f89f4fc date.c: validate and set time in a helper function
date.c
4f89f4fc 553) return -1;

Elijah Newren	7af7a258 unpack-trees: add a new update_sparsity() function
unpack-trees.c
7af7a258 1846) ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;

Elijah Newren	16846444 dir: include DIR_KEEP_UNTRACKED_CONTENTS handling in treat_directory()
dir.c
16846444 1924) FREE_AND_NULL(dir->entries[i]);

Emily Shaffer	617d5719 bugreport: gather git version and build info
help.c
617d5719 641) strbuf_addstr(buf, "no commit associated with this build\n");

Emily Shaffer	709df95b help: move list_config_help to builtin/help
builtin/help.c
709df95b 123) puts(var);
709df95b 124) continue;

Emily Shaffer	1411914a bugreport: add uname info
bugreport.c
1411914a 18) strbuf_addf(sys_info, _("uname() failed with error '%s' (%d)\n"),
1411914a 20)     errno);

Garima Singh	f97b9325 commit-graph: compute Bloom filters for changed paths
commit-graph.c
f97b9325 1298) progress = start_delayed_progress(
f97b9325 1300) ctx->commits.nr);

Garima Singh	ed591feb bloom.c: core Bloom filter implementation for changed paths.
bloom.c
ed591feb 172) return NULL;
ed591feb 246) for (i = 0; i < diff_queued_diff.nr; i++)
ed591feb 247) diff_free_filepair(diff_queued_diff.queue[i]);
ed591feb 248) filter->data = NULL;
ed591feb 249) filter->len = 0;

Garima Singh	76ffbca7 commit-graph: write Bloom filters to commit graph file
commit-graph.c
76ffbca7 338) chunk_repeated = 1;
76ffbca7 345) chunk_repeated = 1;
76ffbca7 352) break;
76ffbca7 1094) progress = start_delayed_progress(
76ffbca7 1096) ctx->commits.nr);
76ffbca7 1119) progress = start_delayed_progress(
76ffbca7 1121) ctx->commits.nr);

Garima Singh	a56b9464 revision.c: use Bloom filters to speed up path based revision walks
bloom.c
a56b9464 266) return -1;

revision.c
a56b9464 721) return -1;

Garima Singh	3d112755 commit-graph: examine commits by generation number
commit-graph.c
3d112755 1306) QSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);

Garima Singh	f1294eaf bloom.c: introduce core Bloom filter constructs
t/helper/test-bloom.c
f1294eaf 25) printf("No filter.\n");
f1294eaf 26) return;

Jeff King	d21ee7d1 commit-graph: examine changed-path objects in pack order
commit-graph.c
d21ee7d1 76) return; /* should never happen, but be lenient */
d21ee7d1 81) static int commit_pos_cmp(const void *va, const void *vb)
d21ee7d1 83) const struct commit *a = *(const struct commit **)va;
d21ee7d1 84) const struct commit *b = *(const struct commit **)vb;
d21ee7d1 85) return commit_pos_at(&commit_pos, a) -
d21ee7d1 86)        commit_pos_at(&commit_pos, b);

Jeff King	1b4c57fa test-bloom: check that we have expected arguments
t/helper/test-bloom.c
1b4c57fa 54) usage(bloom_usage);
1b4c57fa 59) usage(bloom_usage);
1b4c57fa 71) usage(bloom_usage);
1b4c57fa 85) usage(bloom_usage);

Karsten Blees	defd7c7b dir.c: git-status --ignored: don't scan the work tree three times
dir.c
defd7c7b 2177) return path_none;

Patrick Steinhardt	edc30691 refs: fix segfault when aborting empty transaction
refs/files-backend.c
edc30691 2572) strbuf_release(&err);

Patrick Steinhardt	a65b8ac2 update-ref: organize commands in an array
builtin/update-ref.c
a65b8ac2 419) continue;

Taylor Blau	37b9dcab shallow.c: use '{commit,rollback}_shallow_file'
builtin/receive-pack.c
37b9dcab 896) rollback_shallow_file(the_repository, &shallow_lock);

Taylor Blau	fdbde82f builtin/commit-graph.c: introduce split strategy 'no-merge'
commit-graph.c
fdbde82f 1780) break;



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

Commits introducing uncovered code:
Ævar Arnfjörð Bjarmason	5cc044e0 get_short_oid: sort ambiguous objects by type, then SHA-1
oid-array.c
5cc044e0 56) return ret;

brian m. carlson	edc6dccf builtin/receive-pack: use constant-time comparison for HMAC value
builtin/receive-pack.c
edc6dccf 568) retval = NONCE_BAD;
edc6dccf 569) goto leave;

brian m. carlson	910650d2 Rename sha1_array to oid_array
t/helper/test-oid-array.c
910650d2 29) oid_array_clear(&array);

Denton Liu	7cd54d37 wrapper: indent with tabs
wrapper.c
7cd54d37 221) len = MAX_IO_SIZE;
7cd54d37 243) len = MAX_IO_SIZE;

Denton Liu	9460fd48 Lib-ify prune-packed
prune-packed.c
9460fd48 26) printf("rm -f %s\n", path);
9460fd48 35) progress = start_delayed_progress(_("Removing duplicate objects"), 256);

Denton Liu	ce6521e4 Lib-ify fmt-merge-msg
fmt-merge-msg.c
ce6521e4 24) use_branch_desc = git_config_bool(key, value);
ce6521e4 75) continue;
ce6521e4 107) return 1;
ce6521e4 113) return 2;
ce6521e4 117) return 3;
ce6521e4 126) line[len - 1] = 0;
ce6521e4 167) origin = src;
ce6521e4 168) string_list_append(&src_data->generic, line);
ce6521e4 169) src_data->head_status |= 2;
ce6521e4 188) return;
ce6521e4 201) static void add_branch_desc(struct strbuf *out, const char *name)
ce6521e4 203) struct strbuf desc = STRBUF_INIT;
ce6521e4 205) if (!read_branch_desc(&desc, name)) {
ce6521e4 206) const char *bp = desc.buf;
ce6521e4 207) while (*bp) {
ce6521e4 208) const char *ep = strchrnul(bp, '\n');
ce6521e4 209) if (*ep)
ce6521e4 210) ep++;
ce6521e4 211) strbuf_addf(out, "  : %.*s", (int)(ep - bp), bp);
ce6521e4 212) bp = ep;
ce6521e4 214) strbuf_complete_line(out);
ce6521e4 216) strbuf_release(&desc);
ce6521e4 217) }
ce6521e4 231) return;
ce6521e4 239) return;
ce6521e4 276) else if (people->nr)
ce6521e4 277) strbuf_addf(out, "%s (%d) and others",
ce6521e4 278)     people->items[0].string,
ce6521e4 279)     (int)util_as_integral(&people->items[0]));
ce6521e4 342) return;
ce6521e4 355) if (opts->credit_people)
ce6521e4 356) record_person('c', &committers, commit);
ce6521e4 372) string_list_append(&subjects,
ce6521e4 373)    oid_to_hex(&commit->object.oid));
ce6521e4 387) add_branch_desc(out, name);
ce6521e4 425) subsep = ", ";
ce6521e4 426) strbuf_addstr(out, "HEAD");
ce6521e4 446) strbuf_addstr(out, subsep);
ce6521e4 447) print_joined("commit ", "commits ", &src_data->generic,
ce6521e4 507) if (tag_number == 2) {
ce6521e4 508) struct strbuf tagline = STRBUF_INIT;
ce6521e4 509) strbuf_addch(&tagline, '\n');
ce6521e4 510) strbuf_add_commented_lines(&tagline,
ce6521e4 511) origins.items[first_tag].string,
ce6521e4 512) strlen(origins.items[first_tag].string));
ce6521e4 513) strbuf_insert(&tagbuf, 0, tagline.buf,
ce6521e4 515) strbuf_release(&tagline);
ce6521e4 517) strbuf_addch(&tagbuf, '\n');
ce6521e4 518) strbuf_add_commented_lines(&tagbuf,
ce6521e4 519) origins.items[i].string,
ce6521e4 520) strlen(origins.items[i].string));
ce6521e4 521) fmt_tag_signature(&tagbuf, &sig, buf, len);
ce6521e4 566) continue;

Derrick Stolee	c9f7a793 log-tree: make ref_filter_match() a helper method
log-tree.c
c9f7a793 94)     (!*rest || *rest == '/'))

Elijah Newren	b9cbd295 rebase: reinstate --no-keep-empty
sequencer.c
b9cbd295 4674) continue;

Jeff King	24036686 credential: parse URL without host as empty host, not unset
http.c
24036686 582) cert_auth.host = xstrdup("");

Jeff King	16ddcd40 sha1_array: let callbacks interrupt iteration
oid-array.c
16ddcd40 76) return ret;

Jeff King	348482de config: reject parsing of files over INT_MAX
config.c
348482de 537) cf->eof = 1;
348482de 538) return 0;

Jeff King	d8410a81 fast-import: replace custom hash with hashmap.c
fast-import.c
d8410a81 60) e2 = container_of(entry_or_key, const struct object_entry, ent);
d8410a81 61) return oidcmp(&e1->idx.oid, &e2->idx.oid);

Jonathan Tan	95acf11a diff: restrict when prefetching occurs
diffcore-rename.c
95acf11a 137) static void prefetch(void *prefetch_options)
95acf11a 139) struct prefetch_options *options = prefetch_options;
95acf11a 141) struct oid_array to_fetch = OID_ARRAY_INIT;
95acf11a 143) for (i = 0; i < rename_dst_nr; i++) {
95acf11a 144) if (rename_dst[i].pair)
95acf11a 149) continue; /* already found exact match */
95acf11a 150) diff_add_if_missing(options->repo, &to_fetch,
95acf11a 151)     rename_dst[i].two);
95acf11a 153) for (i = 0; i < rename_src_nr; i++) {
95acf11a 154) if (options->skip_unmodified &&
95acf11a 155)     diff_unmodified_pair(rename_src[i].p))
95acf11a 160) continue;
95acf11a 161) diff_add_if_missing(options->repo, &to_fetch,
95acf11a 162)     rename_src[i].p->one);
95acf11a 164) promisor_remote_get_direct(options->repo, to_fetch.oid, to_fetch.nr);
95acf11a 165) oid_array_clear(&to_fetch);
95acf11a 166) }

Josh Steadmon	3d3adaad trace2: teach Git to log environment variables
trace2/tr2_cfg.c
3d3adaad 63) return tr2_cfg_env_vars_count;

Junio C Hamano	56a1d9ca Merge branch 'dl/libify-a-few'
fmt-merge-msg.c
56a1d9ca 498) strbuf_addstr(&sig, "gpg verification failed.\n");

Vasil Dimov	8cf51561 range-diff: fix a crash in parsing git-log output
range-diff.c
8cf51561 115) string_list_clear(list, 1);
8cf51561 116) strbuf_release(&buf);
8cf51561 117) strbuf_release(&contents);
8cf51561 118) finish_command(&cp);
8cf51561 119) return -1;

Re: Git Test Coverage Report (April 30, 2020)

[Han-Wen Nienhuys]

On Thu, Apr 30, 2020 at 1:22 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> Hello,
>
> Here is today's test coverage report.
>
> It appears that _all_ of the reftable code shows up in this report as
> untested. Perhaps that is the state of the world right now, or perhaps
> I need to initialize a GIT_TEST_ environment variable to ensure it runs?

> In either way, do we have any test coverage of that massive contribution?
> Please take a look at the online report [1] for that part of the report,
> as I snipped it out of this email.

(again in plain text)

The reftable code is currently exercised in a small way by t0031-reftable.sh

The upstream library has unittests, see
https://cs.bazel.build/search?q=r%3Areftable+f%3A_test.c&num=0, but I
believe Git doesn't do unittests?

I'll try to generate a coverage report for them.

You can set GIT_TEST_REFTABLE to see coverage for reftable, but I'm
not sure how useful it is given that ~15% of the tests fail.


-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: Git Test Coverage Report (April 30, 2020)

[Johannes Schindelin]

Hi Han-Wen,

On Thu, 30 Apr 2020, Han-Wen Nienhuys wrote:

> On Thu, Apr 30, 2020 at 1:22 PM Derrick Stolee <stolee@gmail.com> wrote:
> >
> > Hello,
> >
> > Here is today's test coverage report.
> >
> > It appears that _all_ of the reftable code shows up in this report as
> > untested. Perhaps that is the state of the world right now, or perhaps
> > I need to initialize a GIT_TEST_ environment variable to ensure it runs?
>
> > In either way, do we have any test coverage of that massive contribution?
> > Please take a look at the online report [1] for that part of the report,
> > as I snipped it out of this email.
>
> (again in plain text)
>
> The reftable code is currently exercised in a small way by
> t0031-reftable.sh

Very small, and yet even the simple change I made had uncovered
a segmentation fault that the unit tests you refer to below had not.

Which makes me think even more than I already thought that it would have
made a lot more sense to develop the reftable library inside of git.git
instead of completely separate from it.

That would also have staved off the many, many style issues that make this
thing hard to review, not to mention the sheer size that threatens to dull
any mind pouring over the diff.

It is probably not a secret by now that these issues keep me from
reviewing the code.

I mean, I want the reftables feature to finally address the problem on
Windows where branch names are sometimes treated as case sensitive, but
then all of a sudden they are not (because of the file system backend). So
there is motivation on my side to help the patches along. Yet, the way the
patches are presented does not invite me to do so.

In a private conversation I had recently, the situation was likened to
having a neat home that you really like and take care of because you,
well, care, and therefore you ask guests to take off their shoes, but one
of the guests just ignores the local customs and walks in with their muddy
shoes, right over your white carpet.

It is a bit of a crass picture that was painted there because the
situation is not _all_ that bad. Yet... it _is_ very unenticing to have
the coding style stepped all over, as well as the convention to "tell a
story with your patches and commit messages" not exactly followed even to
the letter "d" (let alone "t").

I understand that it seemed easier to first implement a library in a
familiar language (Go) and then "port" it (where the "port" part shows by
using constructs that are uncommon in C).

However, when I think about the potential users, there really are only
two: Git and libgit2. And since libgit2 is relatively similar in origin to
Git, I don't think it was the best idea to start with an abstraction layer
over data types. It would have been a lot easier to build the code in
git.git first, test the heck out of it, and then abstract it _just_ enough
to let libgit2 essentially use a copy (just like it does with the libxdiff
code).

Speaking of testing: there is no doubt in my mind that _iff_ these patches
make it into git.git, then the vast majority of bug fixes will flow _from_
git.git. And if that is indeed the case, it makes it very, very awkward to
ask every contributor to go to _another_ project to get those fixes in
there first, and only then have them trickle back to the actual user.

In light of this, I wonder whether there is anything you could do to
change the way you develop this patch series that would make it more in
line with the code contributions the Git project enjoys? Or at least to
tell a much more elegant story arc in the patch series and abide by the
coding conventions?

I could imagine that just throwing away all that data type abstraction and
reusing what is in git.git, including all the helper functions, would go a
long way of not only simplifying the structure of the patch series to
allow for a meaningful review (in the current form, I don't think that
_any_ human could possibly verify the correctness, as the complexity and
the size is just too daunting and too overwhelming), but it would also
avoid implementing redundant functionality, which would not only reduce
the size, but would also let us rely on tried-and-tested implementation in
git.git.

Just to name an example, pretty much the entire `basics.c` could go away.

To give you a concrete data point for tried-and-tested function: the
`string_list_split()` function has a short and sweet implementation that
has not had to be fixed in more than seven years. That is quite a track
record. Using this function instead of `parse_names()` would give us the
instant benefit of relying on something we do _not_ have to review for
correctness. And there is the distinct possibility that using a
`string_list` with an explicit length (instead of throwing it away in
`parse_names()` and then painstakingly re-calculating it in
`names_length()`) might actually improve performance, too.

I firmly believe that this patch series, in particular the huge patch, can
be transformed into something that looks a lot more like git.git code, is
a lot smaller, and is a _lot_ easier to review. Without the confidence
such a reviewable shape provides, I would actually not trust it enough to
put it into the hands of end-users.

> The upstream library has unittests, see
> https://cs.bazel.build/search?q=r%3Areftable+f%3A_test.c&num=0, but I
> believe Git doesn't do unittests?

That is a misconception.

We usually implement some test helper in `t/helper/` (or a new subcommand
in one test helper) and run that as part of the test suite.

And to repeat my point above: I don't think that I would have found a
segfault _immediately_ after modifying t0031 in a rather trivial way if
this library was developed _inside_ git.git rather than outside of it,
with an incremental story accompanied by unit tests.

At the end of the day, it matters more that the reftable works in Git than
it matters that it passes the unit tests when compiled somewhere else.

> I'll try to generate a coverage report for them.
>
> You can set GIT_TEST_REFTABLE to see coverage for reftable, but I'm
> not sure how useful it is given that ~15% of the tests fail.

15%? That's a lot... I hope that that's mostly test cases that incorrectly
assume that they can access refs directly on disk, in loose format.

Ciao,
Dscho

Re: Git Test Coverage Report (April 30, 2020)

[Jeff King]

On Sat, May 02, 2020 at 12:08:41AM +0200, Johannes Schindelin wrote:

> I firmly believe that this patch series, in particular the huge patch, can
> be transformed into something that looks a lot more like git.git code, is
> a lot smaller, and is a _lot_ easier to review. Without the confidence
> such a reviewable shape provides, I would actually not trust it enough to
> put it into the hands of end-users.

I agree, and I think it's not just a question of review, but of
maintenance going forward.

We are willing to accept something like compat/regex or kwset having a
vastly differing style and avoiding our usual constructs because they
are pretty stand-alone. They are modular, battle-tested pieces of code
we've taken wholesale $from elsewhere, and we're generally able to treat
them like a black box (that usually works). We really consider them a
"vendor" solution versus a system regex.

But I don't think that's the right attitude for something as critical to
Git as ref storage, or something that is mainly being used by Git (and
so we will be the likely ones to find bugs, not some third-party use).
As much as I do laud efforts to make code that can be used
out-of-the-box by other implementations, within the git.git
implementation stability and maintainability are the _most_ important
things.

Another data point to contrast with kwset, etc, is xdiff. We also took a
stylistically-unusual vendor copy of that into our tree. And I don't
think it has worked all that well. People are hesitant to touch the
code, and it has often lagged (or completely missed out) on project-wide
cleanups like those around memory allocation, size_t overflows, etc. I'd
be even more afraid to see similar issues in something as critical as
ref storage.

-Peff

Re: Git Test Coverage Report (April 30, 2020)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I agree, and I think it's not just a question of review, but of
> maintenance going forward.
> ...
> ... I'd
> be even more afraid to see similar issues in something as critical as
> ref storage.

Thanks for saying these.  I have nothing to add.

Re: Git Test Coverage Report (April 30, 2020)

[Han-Wen Nienhuys]

On Sun, May 3, 2020 at 6:58 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > I agree, and I think it's not just a question of review, but of
> > maintenance going forward.
> > ...
> > ... I'd
> > be even more afraid to see similar issues in something as critical as
> > ref storage.
>
> Thanks for saying these.  I have nothing to add.

I have sent an updated patch to fix the header issue you pointed out.

I'm discussing options on how to move forward with Jonathan Nieder.
Once we have sorted those out, we'll send out an email, hopefully
early next week.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: Git Test Coverage Report (April 30, 2020)

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> I'm discussing options on how to move forward with Jonathan Nieder.
> Once we have sorted those out, we'll send out an email, hopefully
> early next week.

Thanks.