{"thread":{"id":"53355","subject":"Git Test Coverage Report (April 30, 2020)","startedAt":"2020-04-30T11:21:41Z","lastAt":"2020-05-07T18:56:19Z","messageCount":7,"participants":["Derrick Stolee","Han-Wen Nienhuys","Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"396658","messageId":"fda6d0db-f79e-f44e-7c2b-b60ed1794cd0@gmail.com","threadId":"53355","inReplyTo":null,"subject":"Git Test Coverage Report (April 30, 2020)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-30T11:21:33Z","receivedAt":"2020-04-30T11:21:41Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Hello,\n\nHere is today's test coverage report.\n\nIt appears that _all_ of the reftable code shows up in this report as\nuntested. Perhaps that is the state of the world right now, or perhaps\nI need to initialize a GIT_TEST_ environment variable to ensure it runs?\nIn either way, do we have any test coverage of that massive contribution?\nPlease take a look at the online report [1] for that part of the report,\nas I snipped it out of this email.\n\nThanks,\n-Stolee\n\n[1] https://derrickstolee.github.io/git-test-coverage/reports/2020-04-30-commits.txt\n[2] https://derrickstolee.github.io/git-test-coverage/reports/2020-04-30.txt\n[3] https://derrickstolee.github.io/git-test-coverage/reports/2020-04-30.htm\n\n---\n\npu\t50ce81fe7726b7eea90e201c1d31567158bfe4c1\njch\ta2998880e147321b718420ae805807ef5fb483f3\nnext\t6140810f5a86d69e4bee74d360b936fb4a8a3b1f\nmaster\t86ab15cb154862b6fa5cc646dac27532f881e1fb\nmaster@{1}\tefe3874640e2e58209ddbb3b072f48f6b7094f34\n\n\nUncovered code in 'pu' not in 'jch'\n--------------------------------------------------------\n\nCommits introducing uncovered code:\nCarlo Marcelo Arenas Belón\t555c4709 credential-store: warn instead of fatal for bogus lines from store\ncredential-store.c\n555c4709 27) strbuf_reset(&redacted_line);\n\n[...snipped all reftable code...]\n\nJiang Xin\t2c26d674 receive-pack: new config receive.procReceiveRefs\nbuiltin/receive-pack.c\n2c26d674 237) return config_error_nonbool(var);\n2c26d674 1814) continue;\n\nJiang Xin\tbc3e2729 receive-pack: add new proc-receive hook\nbuiltin/receive-pack.c\nbc3e2729 1021) return code;\nbc3e2729 1024) proc.err = 0;\nbc3e2729 1029) if (use_sideband)\nbc3e2729 1030) finish_async(&muxer);\nbc3e2729 1031) return code;\nbc3e2729 2245) packet_buf_write(&buf, \"ng %s %s%c%s\\n\",\nbc3e2729 2246)  cmd->ref_name, cmd->error_string,\n\nt/helper/test-proc-receive.c\nbc3e2729 127) usage_msg_opt(\"Too many arguments.\", proc_receive_usage, options);\nbc3e2729 167) *p = '\\0';\nbc3e2729 168) p += 2;\nbc3e2729 169) packet_write_fmt(1, \"%s%c%s\\n\", item->string, '\\0', p);\n\nJiang Xin\td131afc3 send-pack: extension for client-side status report\nremote.c\nd131afc3 2362) return;\n\ntransport-helper.c\nd131afc3 756) status = REF_STATUS_UPTODATE;\nd131afc3 757) FREE_AND_NULL(msg);\nd131afc3 764) status = REF_STATUS_REJECT_ALREADY_EXISTS;\nd131afc3 765) FREE_AND_NULL(msg);\nd131afc3 768) status = REF_STATUS_REJECT_FETCH_FIRST;\nd131afc3 769) FREE_AND_NULL(msg);\nd131afc3 772) status = REF_STATUS_REJECT_NEEDS_FORCE;\nd131afc3 773) FREE_AND_NULL(msg);\nd131afc3 776) status = REF_STATUS_REJECT_STALE;\nd131afc3 777) FREE_AND_NULL(msg);\nd131afc3 780) forced = 1;\n\nMiriam Rubio\t9aadf776 bisect--helper: introduce new `write_in_file()` function\nbuiltin/bisect--helper.c\n9aadf776 91) return error_errno(_(\"cannot open file '%s' in mode '%s'\"), path, mode);\n9aadf776 96) fclose(fp);\n9aadf776 97) return error_errno(_(\"could not write to file '%s'\"), path);\n\nPhillip Wood\tcd5e0fd9 Revert \"Revert \"Merge branch 'ra/rebase-i-more-options'\"\"\nbuiltin/rebase.c\ncd5e0fd9 834) argv_array_push(&am.args, \"--ignore-whitespace\");\ncd5e0fd9 836) argv_array_push(&opts->git_am_opts, \"--committer-date-is-author-date\");\ncd5e0fd9 838) argv_array_push(&opts->git_am_opts, \"--ignore-date\");\n\nsequencer.c\ncd5e0fd9 877) return NULL;\ncd5e0fd9 1394) goto out;\ncd5e0fd9 1398) goto out;\ncd5e0fd9 1474) res = -1;\ncd5e0fd9 1475) goto out;\n\nPranit Bauva\t587c9fac bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C\nbuiltin/bisect--helper.c\n587c9fac 493) return error_errno(_(\"failed to write to '%s'\"), git_path_bisect_log());\n587c9fac 522) return error_errno(_(\"could not open '%s' for appending\"),\n\nPranit Bauva\tf0bf9482 bisect--helper: finish porting `bisect_start()` to C\nbuiltin/bisect--helper.c\nf0bf9482 730) return BISECT_FAILED;\nf0bf9482 751) res = BISECT_FAILED;\nf0bf9482 763) res = BISECT_FAILED;\nf0bf9482 775) res = BISECT_FAILED;\n\nPranit Bauva\t05a69202 bisect--helper: reimplement `bisect_autostart` shell function in C\nbuiltin/bisect--helper.c\n05a69202 818) fprintf(stderr, _(\"You need to start by \\\"git bisect \"\n05a69202 821) if (!isatty(STDIN_FILENO))\n05a69202 822) return 1;\n05a69202 829) yesno = git_prompt(_(\"Do you want me to do it for you \"\n05a69202 831) if (starts_with(yesno, _(\"n\")) || starts_with(yesno, _(\"N\")))\n05a69202 832) return 1;\n05a69202 834) return bisect_start(terms, 0, NULL, 0);\n\nPranit Bauva\t10520dbf bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C\nbuiltin/bisect--helper.c\n10520dbf 869) return BISECT_FAILED;\n10520dbf 885) oid_array_clear(&revs);\n10520dbf 886) return BISECT_FAILED;\n\nTaylor Blau\t7cb91802 commit-graph.c: ensure graph layers respect core.sharedRepository\ncommit-graph.c\n7cb91802 1591) return -1;\n\n\n\nUncovered code in 'jch' not in 'next'\n--------------------------------------------------------\n\nCommits introducing uncovered code:\nDamien Robert\t812a5889 remote.c: fix handling of %(push:remoteref)\nremote.c\n812a5889 1638) return NULL;\n812a5889 1645) return branch->refname;\n812a5889 1660) return NULL;\n812a5889 1774) return NULL;\n\nJohannes Schindelin\t12294990 credential: handle `credential.<partial-URL>.<key>` again\nurlmatch.c\n12294990 581) retval = 0;\n\n\n\nUncovered code in 'next' not in 'master'\n--------------------------------------------------------\n\nCommits introducing uncovered code:\nDenton Liu\tbe1bb600 sequencer: make apply_autostash() accept a path\nsequencer.c\nbe1bb600 5203) apply_autostash(rebase_path_autostash());\n\nDenton Liu\t65c425a2 sequencer: stop leaking buf\nsequencer.c\n65c425a2 2557) goto done_rebase_i;\n\nDenton Liu\t5b2f6d9c sequencer: make file exists check more efficient\nsequencer.c\n5b2f6d9c 431) warning_errno(_(\"could not read '%s'\"), path);\n\nDenton Liu\tefcf6cf0 rebase: use read_oneliner()\nbuiltin/rebase.c\nefcf6cf0 625) } else if (!read_oneliner(&buf, state_dir_path(\"head\", opts),\n\nDenton Liu\tf213f069 rebase: generify reset_head()\nbuiltin/rebase.c\nf213f069 880) reset_head(the_repository, &opts->orig_head, \"checkout\",\nf213f069 881)    opts->head_name, 0,\n\nDenton Liu\t86ed00af rebase: use apply_autostash() from sequencer.c\nbuiltin/rebase.c\n86ed00af 1041) apply_autostash(state_dir_path(\"autostash\", opts));\n\nDenton Liu\tb309a971 reset: extract reset_head() from rebase\nreset.c\nb309a971 37) ret = -1;\nb309a971 38) goto leave_reset_head;\nb309a971 43) goto leave_reset_head;\nb309a971 66) goto leave_reset_head;\nb309a971 72) goto leave_reset_head;\nb309a971 77) goto leave_reset_head;\nb309a971 81) ret = -1;\nb309a971 82) goto leave_reset_head;\nb309a971 90) goto leave_reset_head;\nb309a971 109) } else if (old_orig)\nb309a971 110) delete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n\nDerrick Stolee\te3696980 diff: halt tree-diff early after max_changes\ntree-diff.c\ne3696980 438) break;\n\nDerrick Stolee\t8918e379 revision: complicated pathspecs disable filters\nrevision.c\n8918e379 658) return 1;\n8918e379 662) return 1;\n\nDerrick Stolee\t0906ac2b blame: use changed-path Bloom filters\nblame.c\n0906ac2b 1281) return 1;\n0906ac2b 1302) bd->alloc *= 2;\n0906ac2b 1303) REALLOC_ARRAY(bd->keys, bd->alloc);\n0906ac2b 2899) return;\n\nĐoàn Trần Công Danh\tc933b28d date.c: s/is_date/set_date/\ndate.c\nc933b28d 521) return -1;\nc933b28d 532) return -1;\n\nĐoàn Trần Công Danh\t4f89f4fc date.c: validate and set time in a helper function\ndate.c\n4f89f4fc 553) return -1;\n\nElijah Newren\t7af7a258 unpack-trees: add a new update_sparsity() function\nunpack-trees.c\n7af7a258 1846) ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;\n\nElijah Newren\t16846444 dir: include DIR_KEEP_UNTRACKED_CONTENTS handling in treat_directory()\ndir.c\n16846444 1924) FREE_AND_NULL(dir->entries[i]);\n\nEmily Shaffer\t617d5719 bugreport: gather git version and build info\nhelp.c\n617d5719 641) strbuf_addstr(buf, \"no commit associated with this build\\n\");\n\nEmily Shaffer\t709df95b help: move list_config_help to builtin/help\nbuiltin/help.c\n709df95b 123) puts(var);\n709df95b 124) continue;\n\nEmily Shaffer\t1411914a bugreport: add uname info\nbugreport.c\n1411914a 18) strbuf_addf(sys_info, _(\"uname() failed with error '%s' (%d)\\n\"),\n1411914a 20)     errno);\n\nGarima Singh\tf97b9325 commit-graph: compute Bloom filters for changed paths\ncommit-graph.c\nf97b9325 1298) progress = start_delayed_progress(\nf97b9325 1300) ctx->commits.nr);\n\nGarima Singh\ted591feb bloom.c: core Bloom filter implementation for changed paths.\nbloom.c\ned591feb 172) return NULL;\ned591feb 246) for (i = 0; i < diff_queued_diff.nr; i++)\ned591feb 247) diff_free_filepair(diff_queued_diff.queue[i]);\ned591feb 248) filter->data = NULL;\ned591feb 249) filter->len = 0;\n\nGarima Singh\t76ffbca7 commit-graph: write Bloom filters to commit graph file\ncommit-graph.c\n76ffbca7 338) chunk_repeated = 1;\n76ffbca7 345) chunk_repeated = 1;\n76ffbca7 352) break;\n76ffbca7 1094) progress = start_delayed_progress(\n76ffbca7 1096) ctx->commits.nr);\n76ffbca7 1119) progress = start_delayed_progress(\n76ffbca7 1121) ctx->commits.nr);\n\nGarima Singh\ta56b9464 revision.c: use Bloom filters to speed up path based revision walks\nbloom.c\na56b9464 266) return -1;\n\nrevision.c\na56b9464 721) return -1;\n\nGarima Singh\t3d112755 commit-graph: examine commits by generation number\ncommit-graph.c\n3d112755 1306) QSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);\n\nGarima Singh\tf1294eaf bloom.c: introduce core Bloom filter constructs\nt/helper/test-bloom.c\nf1294eaf 25) printf(\"No filter.\\n\");\nf1294eaf 26) return;\n\nJeff King\td21ee7d1 commit-graph: examine changed-path objects in pack order\ncommit-graph.c\nd21ee7d1 76) return; /* should never happen, but be lenient */\nd21ee7d1 81) static int commit_pos_cmp(const void *va, const void *vb)\nd21ee7d1 83) const struct commit *a = *(const struct commit **)va;\nd21ee7d1 84) const struct commit *b = *(const struct commit **)vb;\nd21ee7d1 85) return commit_pos_at(&commit_pos, a) -\nd21ee7d1 86)        commit_pos_at(&commit_pos, b);\n\nJeff King\t1b4c57fa test-bloom: check that we have expected arguments\nt/helper/test-bloom.c\n1b4c57fa 54) usage(bloom_usage);\n1b4c57fa 59) usage(bloom_usage);\n1b4c57fa 71) usage(bloom_usage);\n1b4c57fa 85) usage(bloom_usage);\n\nKarsten Blees\tdefd7c7b dir.c: git-status --ignored: don't scan the work tree three times\ndir.c\ndefd7c7b 2177) return path_none;\n\nPatrick Steinhardt\tedc30691 refs: fix segfault when aborting empty transaction\nrefs/files-backend.c\nedc30691 2572) strbuf_release(&err);\n\nPatrick Steinhardt\ta65b8ac2 update-ref: organize commands in an array\nbuiltin/update-ref.c\na65b8ac2 419) continue;\n\nTaylor Blau\t37b9dcab shallow.c: use '{commit,rollback}_shallow_file'\nbuiltin/receive-pack.c\n37b9dcab 896) rollback_shallow_file(the_repository, &shallow_lock);\n\nTaylor Blau\tfdbde82f builtin/commit-graph.c: introduce split strategy 'no-merge'\ncommit-graph.c\nfdbde82f 1780) break;\n\n\n\nUncovered code in 'master' not in 'master@{1}'\n--------------------------------------------------------\n\nCommits introducing uncovered code:\nÆvar Arnfjörð Bjarmason\t5cc044e0 get_short_oid: sort ambiguous objects by type, then SHA-1\noid-array.c\n5cc044e0 56) return ret;\n\nbrian m. carlson\tedc6dccf builtin/receive-pack: use constant-time comparison for HMAC value\nbuiltin/receive-pack.c\nedc6dccf 568) retval = NONCE_BAD;\nedc6dccf 569) goto leave;\n\nbrian m. carlson\t910650d2 Rename sha1_array to oid_array\nt/helper/test-oid-array.c\n910650d2 29) oid_array_clear(&array);\n\nDenton Liu\t7cd54d37 wrapper: indent with tabs\nwrapper.c\n7cd54d37 221) len = MAX_IO_SIZE;\n7cd54d37 243) len = MAX_IO_SIZE;\n\nDenton Liu\t9460fd48 Lib-ify prune-packed\nprune-packed.c\n9460fd48 26) printf(\"rm -f %s\\n\", path);\n9460fd48 35) progress = start_delayed_progress(_(\"Removing duplicate objects\"), 256);\n\nDenton Liu\tce6521e4 Lib-ify fmt-merge-msg\nfmt-merge-msg.c\nce6521e4 24) use_branch_desc = git_config_bool(key, value);\nce6521e4 75) continue;\nce6521e4 107) return 1;\nce6521e4 113) return 2;\nce6521e4 117) return 3;\nce6521e4 126) line[len - 1] = 0;\nce6521e4 167) origin = src;\nce6521e4 168) string_list_append(&src_data->generic, line);\nce6521e4 169) src_data->head_status |= 2;\nce6521e4 188) return;\nce6521e4 201) static void add_branch_desc(struct strbuf *out, const char *name)\nce6521e4 203) struct strbuf desc = STRBUF_INIT;\nce6521e4 205) if (!read_branch_desc(&desc, name)) {\nce6521e4 206) const char *bp = desc.buf;\nce6521e4 207) while (*bp) {\nce6521e4 208) const char *ep = strchrnul(bp, '\\n');\nce6521e4 209) if (*ep)\nce6521e4 210) ep++;\nce6521e4 211) strbuf_addf(out, \"  : %.*s\", (int)(ep - bp), bp);\nce6521e4 212) bp = ep;\nce6521e4 214) strbuf_complete_line(out);\nce6521e4 216) strbuf_release(&desc);\nce6521e4 217) }\nce6521e4 231) return;\nce6521e4 239) return;\nce6521e4 276) else if (people->nr)\nce6521e4 277) strbuf_addf(out, \"%s (%d) and others\",\nce6521e4 278)     people->items[0].string,\nce6521e4 279)     (int)util_as_integral(&people->items[0]));\nce6521e4 342) return;\nce6521e4 355) if (opts->credit_people)\nce6521e4 356) record_person('c', &committers, commit);\nce6521e4 372) string_list_append(&subjects,\nce6521e4 373)    oid_to_hex(&commit->object.oid));\nce6521e4 387) add_branch_desc(out, name);\nce6521e4 425) subsep = \", \";\nce6521e4 426) strbuf_addstr(out, \"HEAD\");\nce6521e4 446) strbuf_addstr(out, subsep);\nce6521e4 447) print_joined(\"commit \", \"commits \", &src_data->generic,\nce6521e4 507) if (tag_number == 2) {\nce6521e4 508) struct strbuf tagline = STRBUF_INIT;\nce6521e4 509) strbuf_addch(&tagline, '\\n');\nce6521e4 510) strbuf_add_commented_lines(&tagline,\nce6521e4 511) origins.items[first_tag].string,\nce6521e4 512) strlen(origins.items[first_tag].string));\nce6521e4 513) strbuf_insert(&tagbuf, 0, tagline.buf,\nce6521e4 515) strbuf_release(&tagline);\nce6521e4 517) strbuf_addch(&tagbuf, '\\n');\nce6521e4 518) strbuf_add_commented_lines(&tagbuf,\nce6521e4 519) origins.items[i].string,\nce6521e4 520) strlen(origins.items[i].string));\nce6521e4 521) fmt_tag_signature(&tagbuf, &sig, buf, len);\nce6521e4 566) continue;\n\nDerrick Stolee\tc9f7a793 log-tree: make ref_filter_match() a helper method\nlog-tree.c\nc9f7a793 94)     (!*rest || *rest == '/'))\n\nElijah Newren\tb9cbd295 rebase: reinstate --no-keep-empty\nsequencer.c\nb9cbd295 4674) continue;\n\nJeff King\t24036686 credential: parse URL without host as empty host, not unset\nhttp.c\n24036686 582) cert_auth.host = xstrdup(\"\");\n\nJeff King\t16ddcd40 sha1_array: let callbacks interrupt iteration\noid-array.c\n16ddcd40 76) return ret;\n\nJeff King\t348482de config: reject parsing of files over INT_MAX\nconfig.c\n348482de 537) cf->eof = 1;\n348482de 538) return 0;\n\nJeff King\td8410a81 fast-import: replace custom hash with hashmap.c\nfast-import.c\nd8410a81 60) e2 = container_of(entry_or_key, const struct object_entry, ent);\nd8410a81 61) return oidcmp(&e1->idx.oid, &e2->idx.oid);\n\nJonathan Tan\t95acf11a diff: restrict when prefetching occurs\ndiffcore-rename.c\n95acf11a 137) static void prefetch(void *prefetch_options)\n95acf11a 139) struct prefetch_options *options = prefetch_options;\n95acf11a 141) struct oid_array to_fetch = OID_ARRAY_INIT;\n95acf11a 143) for (i = 0; i < rename_dst_nr; i++) {\n95acf11a 144) if (rename_dst[i].pair)\n95acf11a 149) continue; /* already found exact match */\n95acf11a 150) diff_add_if_missing(options->repo, &to_fetch,\n95acf11a 151)     rename_dst[i].two);\n95acf11a 153) for (i = 0; i < rename_src_nr; i++) {\n95acf11a 154) if (options->skip_unmodified &&\n95acf11a 155)     diff_unmodified_pair(rename_src[i].p))\n95acf11a 160) continue;\n95acf11a 161) diff_add_if_missing(options->repo, &to_fetch,\n95acf11a 162)     rename_src[i].p->one);\n95acf11a 164) promisor_remote_get_direct(options->repo, to_fetch.oid, to_fetch.nr);\n95acf11a 165) oid_array_clear(&to_fetch);\n95acf11a 166) }\n\nJosh Steadmon\t3d3adaad trace2: teach Git to log environment variables\ntrace2/tr2_cfg.c\n3d3adaad 63) return tr2_cfg_env_vars_count;\n\nJunio C Hamano\t56a1d9ca Merge branch 'dl/libify-a-few'\nfmt-merge-msg.c\n56a1d9ca 498) strbuf_addstr(&sig, \"gpg verification failed.\\n\");\n\nVasil Dimov\t8cf51561 range-diff: fix a crash in parsing git-log output\nrange-diff.c\n8cf51561 115) string_list_clear(list, 1);\n8cf51561 116) strbuf_release(&buf);\n8cf51561 117) strbuf_release(&contents);\n8cf51561 118) finish_command(&cp);\n8cf51561 119) return -1;\n\n\n"},{"id":"396664","messageId":"CAFQ2z_PP9Ld+GDctV-v2CDKFamF6zKdJZ_-jhahj_fcm3wy4Hw@mail.gmail.com","threadId":"53355","inReplyTo":"fda6d0db-f79e-f44e-7c2b-b60ed1794cd0@gmail.com","subject":"Re: Git Test Coverage Report (April 30, 2020)","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-04-30T15:12:18Z","receivedAt":"2020-04-30T15:12:33Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Apr 30, 2020 at 1:22 PM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> Hello,\n>\n> Here is today's test coverage report.\n>\n> It appears that _all_ of the reftable code shows up in this report as\n> untested. Perhaps that is the state of the world right now, or perhaps\n> I need to initialize a GIT_TEST_ environment variable to ensure it runs?\n\n> In either way, do we have any test coverage of that massive contribution?\n> Please take a look at the online report [1] for that part of the report,\n> as I snipped it out of this email.\n\n(again in plain text)\n\nThe reftable code is currently exercised in a small way by t0031-reftable.sh\n\nThe upstream library has unittests, see\nhttps://cs.bazel.build/search?q=r%3Areftable+f%3A_test.c&num=0, but I\nbelieve Git doesn't do unittests?\n\nI'll try to generate a coverage report for them.\n\nYou can set GIT_TEST_REFTABLE to see coverage for reftable, but I'm\nnot sure how useful it is given that ~15% of the tests fail.\n\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"396790","messageId":"nycvar.QRO.7.76.6.2005012316350.18039@tvgsbejvaqbjf.bet","threadId":"53355","inReplyTo":"CAFQ2z_PP9Ld+GDctV-v2CDKFamF6zKdJZ_-jhahj_fcm3wy4Hw@mail.gmail.com","subject":"Re: Git Test Coverage Report (April 30, 2020)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-01T22:08:41Z","receivedAt":"2020-05-01T22:08:48Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Han-Wen,\n\nOn Thu, 30 Apr 2020, Han-Wen Nienhuys wrote:\n\n> On Thu, Apr 30, 2020 at 1:22 PM Derrick Stolee <stolee@gmail.com> wrote:\n> >\n> > Hello,\n> >\n> > Here is today's test coverage report.\n> >\n> > It appears that _all_ of the reftable code shows up in this report as\n> > untested. Perhaps that is the state of the world right now, or perhaps\n> > I need to initialize a GIT_TEST_ environment variable to ensure it runs?\n>\n> > In either way, do we have any test coverage of that massive contribution?\n> > Please take a look at the online report [1] for that part of the report,\n> > as I snipped it out of this email.\n>\n> (again in plain text)\n>\n> The reftable code is currently exercised in a small way by\n> t0031-reftable.sh\n\nVery small, and yet even the simple change I made had uncovered\na segmentation fault that the unit tests you refer to below had not.\n\nWhich makes me think even more than I already thought that it would have\nmade a lot more sense to develop the reftable library inside of git.git\ninstead of completely separate from it.\n\nThat would also have staved off the many, many style issues that make this\nthing hard to review, not to mention the sheer size that threatens to dull\nany mind pouring over the diff.\n\nIt is probably not a secret by now that these issues keep me from\nreviewing the code.\n\nI mean, I want the reftables feature to finally address the problem on\nWindows where branch names are sometimes treated as case sensitive, but\nthen all of a sudden they are not (because of the file system backend). So\nthere is motivation on my side to help the patches along. Yet, the way the\npatches are presented does not invite me to do so.\n\nIn a private conversation I had recently, the situation was likened to\nhaving a neat home that you really like and take care of because you,\nwell, care, and therefore you ask guests to take off their shoes, but one\nof the guests just ignores the local customs and walks in with their muddy\nshoes, right over your white carpet.\n\nIt is a bit of a crass picture that was painted there because the\nsituation is not _all_ that bad. Yet... it _is_ very unenticing to have\nthe coding style stepped all over, as well as the convention to \"tell a\nstory with your patches and commit messages\" not exactly followed even to\nthe letter \"d\" (let alone \"t\").\n\nI understand that it seemed easier to first implement a library in a\nfamiliar language (Go) and then \"port\" it (where the \"port\" part shows by\nusing constructs that are uncommon in C).\n\nHowever, when I think about the potential users, there really are only\ntwo: Git and libgit2. And since libgit2 is relatively similar in origin to\nGit, I don't think it was the best idea to start with an abstraction layer\nover data types. It would have been a lot easier to build the code in\ngit.git first, test the heck out of it, and then abstract it _just_ enough\nto let libgit2 essentially use a copy (just like it does with the libxdiff\ncode).\n\nSpeaking of testing: there is no doubt in my mind that _iff_ these patches\nmake it into git.git, then the vast majority of bug fixes will flow _from_\ngit.git. And if that is indeed the case, it makes it very, very awkward to\nask every contributor to go to _another_ project to get those fixes in\nthere first, and only then have them trickle back to the actual user.\n\nIn light of this, I wonder whether there is anything you could do to\nchange the way you develop this patch series that would make it more in\nline with the code contributions the Git project enjoys? Or at least to\ntell a much more elegant story arc in the patch series and abide by the\ncoding conventions?\n\nI could imagine that just throwing away all that data type abstraction and\nreusing what is in git.git, including all the helper functions, would go a\nlong way of not only simplifying the structure of the patch series to\nallow for a meaningful review (in the current form, I don't think that\n_any_ human could possibly verify the correctness, as the complexity and\nthe size is just too daunting and too overwhelming), but it would also\navoid implementing redundant functionality, which would not only reduce\nthe size, but would also let us rely on tried-and-tested implementation in\ngit.git.\n\nJust to name an example, pretty much the entire `basics.c` could go away.\n\nTo give you a concrete data point for tried-and-tested function: the\n`string_list_split()` function has a short and sweet implementation that\nhas not had to be fixed in more than seven years. That is quite a track\nrecord. Using this function instead of `parse_names()` would give us the\ninstant benefit of relying on something we do _not_ have to review for\ncorrectness. And there is the distinct possibility that using a\n`string_list` with an explicit length (instead of throwing it away in\n`parse_names()` and then painstakingly re-calculating it in\n`names_length()`) might actually improve performance, too.\n\nI firmly believe that this patch series, in particular the huge patch, can\nbe transformed into something that looks a lot more like git.git code, is\na lot smaller, and is a _lot_ easier to review. Without the confidence\nsuch a reviewable shape provides, I would actually not trust it enough to\nput it into the hands of end-users.\n\n> The upstream library has unittests, see\n> https://cs.bazel.build/search?q=r%3Areftable+f%3A_test.c&num=0, but I\n> believe Git doesn't do unittests?\n\nThat is a misconception.\n\nWe usually implement some test helper in `t/helper/` (or a new subcommand\nin one test helper) and run that as part of the test suite.\n\nAnd to repeat my point above: I don't think that I would have found a\nsegfault _immediately_ after modifying t0031 in a rather trivial way if\nthis library was developed _inside_ git.git rather than outside of it,\nwith an incremental story accompanied by unit tests.\n\nAt the end of the day, it matters more that the reftable works in Git than\nit matters that it passes the unit tests when compiled somewhere else.\n\n> I'll try to generate a coverage report for them.\n>\n> You can set GIT_TEST_REFTABLE to see coverage for reftable, but I'm\n> not sure how useful it is given that ~15% of the tests fail.\n\n15%? That's a lot... I hope that that's mostly test cases that incorrectly\nassume that they can access refs directly on disk, in loose format.\n\nCiao,\nDscho\n"},{"id":"396866","messageId":"20200503095500.GF170902@coredump.intra.peff.net","threadId":"53355","inReplyTo":"nycvar.QRO.7.76.6.2005012316350.18039@tvgsbejvaqbjf.bet","subject":"Re: Git Test Coverage Report (April 30, 2020)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-03T09:55:00Z","receivedAt":"2020-05-03T09:55:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 02, 2020 at 12:08:41AM +0200, Johannes Schindelin wrote:\n\n> I firmly believe that this patch series, in particular the huge patch, can\n> be transformed into something that looks a lot more like git.git code, is\n> a lot smaller, and is a _lot_ easier to review. Without the confidence\n> such a reviewable shape provides, I would actually not trust it enough to\n> put it into the hands of end-users.\n\nI agree, and I think it's not just a question of review, but of\nmaintenance going forward.\n\nWe are willing to accept something like compat/regex or kwset having a\nvastly differing style and avoiding our usual constructs because they\nare pretty stand-alone. They are modular, battle-tested pieces of code\nwe've taken wholesale $from elsewhere, and we're generally able to treat\nthem like a black box (that usually works). We really consider them a\n\"vendor\" solution versus a system regex.\n\nBut I don't think that's the right attitude for something as critical to\nGit as ref storage, or something that is mainly being used by Git (and\nso we will be the likely ones to find bugs, not some third-party use).\nAs much as I do laud efforts to make code that can be used\nout-of-the-box by other implementations, within the git.git\nimplementation stability and maintainability are the _most_ important\nthings.\n\nAnother data point to contrast with kwset, etc, is xdiff. We also took a\nstylistically-unusual vendor copy of that into our tree. And I don't\nthink it has worked all that well. People are hesitant to touch the\ncode, and it has often lagged (or completely missed out) on project-wide\ncleanups like those around memory allocation, size_t overflows, etc. I'd\nbe even more afraid to see similar issues in something as critical as\nref storage.\n\n-Peff\n"},{"id":"396876","messageId":"xmqq1ro1j6qk.fsf@gitster.c.googlers.com","threadId":"53355","inReplyTo":"20200503095500.GF170902@coredump.intra.peff.net","subject":"Re: Git Test Coverage Report (April 30, 2020)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-03T16:58:11Z","receivedAt":"2020-05-03T16:58:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I agree, and I think it's not just a question of review, but of\n> maintenance going forward.\n> ...\n> ... I'd\n> be even more afraid to see similar issues in something as critical as\n> ref storage.\n\nThanks for saying these.  I have nothing to add.\n"},{"id":"397278","messageId":"CAFQ2z_Pf7u0k-PvMG8J-kWMdRXhamm5JgHZiUM6Ffu-2zGGubQ@mail.gmail.com","threadId":"53355","inReplyTo":"xmqq1ro1j6qk.fsf@gitster.c.googlers.com","subject":"Re: Git Test Coverage Report (April 30, 2020)","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-05-07T10:11:02Z","receivedAt":"2020-05-07T10:11:18Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Sun, May 3, 2020 at 6:58 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeff King <peff@peff.net> writes:\n>\n> > I agree, and I think it's not just a question of review, but of\n> > maintenance going forward.\n> > ...\n> > ... I'd\n> > be even more afraid to see similar issues in something as critical as\n> > ref storage.\n>\n> Thanks for saying these.  I have nothing to add.\n\nI have sent an updated patch to fix the header issue you pointed out.\n\nI'm discussing options on how to move forward with Jonathan Nieder.\nOnce we have sorted those out, we'll send out an email, hopefully\nearly next week.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"397334","messageId":"xmqqa72j4lrp.fsf@gitster.c.googlers.com","threadId":"53355","inReplyTo":"CAFQ2z_Pf7u0k-PvMG8J-kWMdRXhamm5JgHZiUM6Ffu-2zGGubQ@mail.gmail.com","subject":"Re: Git Test Coverage Report (April 30, 2020)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-07T18:56:10Z","receivedAt":"2020-05-07T18:56:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> I'm discussing options on how to move forward with Jonathan Nieder.\n> Once we have sorted those out, we'll send out an email, hopefully\n> early next week.\n\nThanks.\n"}]}