Subject: [PATCH v3 0/1] contrib: Add script to show uncovered "new" lines

From: "Derrick Stolee via GitGitGadget" <[email-masked]>
Date: Fri, 21 Sep 2018 08:15:54 -0700 (PDT)

We have coverage targets in our Makefile for using gcov to display line
coverage based on our test suite. The way I like to do it is to run:

make coverage-test
make coverage-report

This leaves the repo in a state where every X.c file that was covered has an
X.c.gcov file containing the coverage counts for every line, and "#####" at
every uncovered line.

There have been a few bugs in recent patches what would have been caught if
the test suite covered those blocks (including a few of mine). I want to
work towards a "sensible" amount of coverage on new topics. In my opinion,
this means that any logic should be covered, but the 'die()' blocks in error
cases do not need to be covered.

It is important to not measure the coverage of the codebase by what old code
is not covered. To help, I created the 'contrib/coverage-diff.sh' script.
After creating the coverage statistics at a version (say, 'topic') you can
then run

contrib/coverage-diff.sh base topic

to see the lines added between 'base' and 'topic' that are not covered by
the test suite. For example, I ran this against the 'next' branch (22e244b)
versus 'master' (150f307) and got the following output:

fsck.c
fb8952077df     (René Scharfe   2018-09-03 14:49:26 +0000       212)            die_errno("Could not read '%s'", path);
list-objects-filter-options.c
f12b8fc6d3b     (Matthew DeVore 2018-09-13 17:55:27 -0700       56)                     if (errbuf) {
f12b8fc6d3b     (Matthew DeVore 2018-09-13 17:55:27 -0700       57)                             strbuf_init(errbuf, 0);
f12b8fc6d3b     (Matthew DeVore 2018-09-13 17:55:27 -0700       58)                             strbuf_addstr(
f12b8fc6d3b     (Matthew DeVore 2018-09-13 17:55:27 -0700       62)                     return 1;
list-objects-filter.c
77d7a65d502     (Matthew DeVore 2018-09-13 17:55:26 -0700       47)             BUG("unknown filter_situation: %d", filter_situation);
f12b8fc6d3b     (Matthew DeVore 2018-09-13 17:55:27 -0700       100)    default:
f12b8fc6d3b     (Matthew DeVore 2018-09-13 17:55:27 -0700       101)            BUG("unknown filter_situation: %d", filter_situation);
77d7a65d502     (Matthew DeVore 2018-09-13 17:55:26 -0700       152)            BUG("unknown filter_situation: %d", filter_situation);
77d7a65d502     (Matthew DeVore 2018-09-13 17:55:26 -0700       257)            BUG("unknown filter_situation: %d", filter_situation);
77d7a65d502     (Matthew DeVore 2018-09-13 17:55:26 -0700       438)            BUG("invalid list-objects filter choice: %d",
list-objects.c
f447a499dbb     (Matthew DeVore 2018-08-13 11:14:28 -0700       197)                    ctx->show_object(obj, base->buf, ctx->show_data);
ll-merge.c
d64324cb60e     (Torsten Bögershausen   2018-09-12 21:32:02 +0200       379)                    marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
midx.c
56ee7ff1565     (Derrick Stolee 2018-09-13 11:02:13 -0700       949)            return 0;
cc6af73c029     (Derrick Stolee 2018-09-13 11:02:25 -0700       990)                    midx_report(_("failed to load pack-index for packfile %s"),
cc6af73c029     (Derrick Stolee 2018-09-13 11:02:25 -0700       991)                                e.p->pack_name);
cc6af73c029     (Derrick Stolee 2018-09-13 11:02:25 -0700       992)                    break;
remote-curl.c
c3b9bc94b9b     (Elijah Newren  2018-09-05 10:03:07 -0700       181)            options.filter = xstrdup(value);
submodule.c
df255b8cac7     (Brandon Williams       2018-08-08 15:33:22 -0700       1738)           die(_("could not create directory '%s'"), new_gitdir.buf);

Using this 'git blame' output, we can quickly inspect whether the uncovered
lines are appropriate. For instance:

 1. The line in builtin/commit.c is due to writing the commit-graph file
    when GIT_TEST_COMMIT_GRAPH is enabled, which is not on by default in the
    test suite. Being uncovered is expected here.

 2. The lines in builtin/worktree.c are all related to error conditions.
    This is acceptable.

 3. The line in builtin/rev-list.c is a flag replacement in a block that is
    otherwise unchanged. It must not be covered by the test suite normally.
    This could be worth adding a test to ensure the new logic maintains old
    behavior.

 4. The lines in read-cache.c are part of a new block for the condition "if
    (expand_name_field)" as part of an optimization. These lines should
    probably be covered before that series is merged to 'next'. I understand
    that Ben and Duy are continuing work in this direction [1].

I used this approach for 'next' over 'master' and got a larger list, some of
which I have already submitted tests to increase coverage [2] or will be
covered by topics not in 'next' [3].

Thanks, -Stolee

CHANGES IN V3: I took Junio's perl script verbatim, which speeds up the
performance greatly. Some of the other sed commands needed some massaging,
but also added extra cleanup. Thanks for the help!

[1] 
https://public-inbox.org/git/[email-masked]/T/#u

[2] https://public-inbox.org/git/[email-masked]/

[3] https://public-inbox.org/git/[email-masked]/

Derrick Stolee (1):
  contrib: add coverage-diff script

 contrib/coverage-diff.sh | 79 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100755 contrib/coverage-diff.sh

base-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-40%2Fderrickstolee%2Fcoverage-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-40/derrickstolee/coverage-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/40

Range-diff vs v2:

 1:  7714b0659e < -:  ---------- contrib: add coverage-diff script
 -:  ---------- > 1:  21214cc321 contrib: add coverage-diff script

-- 
gitgitgadget