Re: Git Test Coverage Report (v2.20.0-rc0)

From: Derrick Stolee <stolee@gmail.com>
To: "git@vger.kernel.org" <git@vger.kernel.org>,
	"Jeff King" <peff@peff.net>, "Stefan Beller" <sbeller@google.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Ben Peart" <benpeart@microsoft.com>
Subject: Re: Git Test Coverage Report (v2.20.0-rc0)
Date: Mon, 19 Nov 2018 10:40:53 -0500	[thread overview]
Message-ID: <6f532502-d4b6-17f6-0ec7-01079077ac90@gmail.com> (raw)
In-Reply-To: <9e293b1b-1845-1772-409b-031c0bf4d17b@gmail.com>

The test coverage reports started mid-way through this release cycle, so 
I thought it would be good to do a full review of the new uncovered code 
since the last release.

I eliminated most of the uncovered code due to the following cases:

1. Code was only moved or refactored.
2. Code was related to unusual error conditions (e.g. open_pack_index() 
fails)

The comments below are intended only to point out potential directions 
to improve test coverage. Some of it is for me to do!

Thanks,
-Stolee

On 11/18/2018 9:54 PM, Derrick Stolee wrote:
> 66ec0390e7 builtin/fsck.c 888) midx_argv[2] = "--object-dir";
> 66ec0390e7 builtin/fsck.c 889) midx_argv[3] = alt->path;
> 66ec0390e7 builtin/fsck.c 890) if (run_command(&midx_verify))
> 66ec0390e7 builtin/fsck.c 891) errors_found |= ERROR_COMMIT_GRAPH;
>

There are two things wrong here:

1. Not properly covering multi-pack-index fsck with alternates.
2. the ERROR_COMMIT_GRAPH flag when the multi-pack-index is being checked.

I'll submit a patch to fix this.

> 2fa233a554 builtin/pack-objects.c 1512) hashcpy(base_oid.hash, 
> base_sha1);
> 2fa233a554 builtin/pack-objects.c 1513) if 
> (!in_same_island(&delta->idx.oid, &base_oid))
> 2fa233a554 builtin/pack-objects.c 1514) return 0;

These lines are inside a block for the following if statement:

+       /*
+        * Otherwise, reachability bitmaps may tell us if the receiver 
has it,
+        * even if it was buried too deep in history to make it into the
+        * packing list.
+        */
+       if (thin && bitmap_has_sha1_in_uninteresting(bitmap_git, 
base_sha1)) {

Peff: is this difficult to test?

> 28b8a73080 builtin/pack-objects.c 2793) depth++;
> 108f530385 builtin/pack-objects.c 2797) oe_set_tree_depth(&to_pack, 
> ent, depth); 

This 'depth' variable is incremented as part of a for loop in this patch:

  static void show_object(struct object *obj, const char *name, void *data)
@@ -2686,6 +2706,19 @@ static void show_object(struct object *obj, const 
char *name, void *data)
         add_preferred_base_object(name);
         add_object_entry(&obj->oid, obj->type, name, 0);
         obj->flags |= OBJECT_ADDED;
+
+       if (use_delta_islands) {
+               const char *p;
+               unsigned depth = 0;
+               struct object_entry *ent;
+
+               for (p = strchr(name, '/'); p; p = strchr(p + 1, '/'))
+                       depth++;
+
+               ent = packlist_find(&to_pack, obj->oid.hash, NULL);
+               if (ent && depth > ent->tree_depth)
+                       ent->tree_depth = depth;
+       }
  }

And that 'ent->tree_depth = depth;' line is replaced with the 
oe_set_tree_depth() call in the report.

Since depth is never incremented, we are not covering this block. Is it 
possible to test?

> builtin/repack.c
> 16d75fa48d  48) use_delta_islands = git_config_bool(var, value);
> 16d75fa48d  49) return 0;

This is a simple config option check for "repack.useDeltaIslands". The 
logic it enables is tested, so this is an OK gap, in my opinion.

 > builtin/submodule--helper.c
> ee69b2a90c 1476) out->type = sub->update_strategy.type;
> ee69b2a90c 1477) out->command = sub->update_strategy.command;

This block was introduced by this part of the patch:

+       } else if (sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
+               trace_printf("loaded thing");
+               out->type = sub->update_strategy.type;
+               out->command = sub->update_strategy.command;

Which seems to be an important case, as the other SM_UPDATE_* types seem 
like interesting cases.

Stefan: what actions would trigger this block? Is it easy to test?

> delta-islands.c
> c8d521faf7  53) memcpy(b, old, size);

This memcpy happens when the 'old' island_bitmap is passed to 
island_bitmap_new(), but...

> c8d521faf7 187) b->refcount--;
> c8d521faf7 188) b = kh_value(island_marks, pos) = island_bitmap_new(b);

This block has the only non-NULL caller to island_bitmap_new().

> c8d521faf7 212) obj = ((struct tag *)obj)->tagged;
> c8d521faf7 213) if (obj) {
> c8d521faf7 214) parse_object(the_repository, &obj->oid);
> c8d521faf7 215) marks = create_or_get_island_marks(obj);
> c8d521faf7 216) island_bitmap_set(marks, island_counter);

It appears that this block would happen if we placed a tag in the delta 
island.

> c8d521faf7 397) strbuf_addch(&island_name, '-');

This block is inside the following patch:

+       if (matches[ARRAY_SIZE(matches) - 1].rm_so != -1)
+               warning(_("island regex from config has "
+                         "too many capture groups (max=%d)"),
+                       (int)ARRAY_SIZE(matches) - 2);
+
+       for (m = 1; m < ARRAY_SIZE(matches); m++) {
+               regmatch_t *match = &matches[m];
+
+               if (match->rm_so == -1)
+                       continue;
+
+               if (island_name.len)
+                       strbuf_addch(&island_name, '-');
+
+               strbuf_add(&island_name, refname + match->rm_so, 
match->rm_eo - match->rm_so);
+       }

This likely means that ARRAY_SIZE(matches) is never more than two.


> c8d521faf7 433) continue;
> c8d521faf7 436) list[dst] = list[src];

These blocks are inside the following nested loop in deduplicate_islands():

+       for (ref = 0; ref + 1 < island_count; ref++) {
+               for (src = ref + 1, dst = src; src < island_count; src++) {
+                       if (list[ref]->hash == list[src]->hash)
+                               continue;
+
+                       if (src != dst)
+                               list[dst] = list[src];
+
+                       dst++;
+               }
+               island_count = dst;
+       }

This means that our "deduplication" logic is never actually doing 
anything meaningful.

> entry.c
> b878579ae7 402) static void mark_colliding_entries(const struct 
> checkout *state,

(there is interesting logic in this method, but it is only enabled on 
case-insensitive filesystems. This run was done on a case-sensitive file 
system. Related changes happen in unpack-trees.c.)

> help.c
> 26c7d06783 help.c         500) static int get_alias(const char *var, 
> const char *value, void *data)
> 26c7d06783 help.c         502) struct string_list *list = data;
> 26c7d06783 help.c         504) if (skip_prefix(var, "alias.", &var))
> 26c7d06783 help.c         505) string_list_append(list, var)->util = 
> xstrdup(value);
> 26c7d06783 help.c         507) return 0;
> 26c7d06783 help.c         530) printf("\n%s\n", _("Command aliases"));
> 26c7d06783 help.c         531) ALLOC_ARRAY(aliases, alias_list.nr + 1);
> 26c7d06783 help.c         532) for (i = 0; i < alias_list.nr; i++) {
> 26c7d06783 help.c         533) aliases[i].name = 
> alias_list.items[i].string;
> 26c7d06783 help.c         534) aliases[i].help = 
> alias_list.items[i].util;
> 26c7d06783 help.c         535) aliases[i].category = 1;
> 26c7d06783 help.c         537) aliases[alias_list.nr].name = NULL;
> 26c7d06783 help.c         538) print_command_list(aliases, 1, longest);
> 26c7d06783 help.c         539) free(aliases);

This logic introduces alias help in 'git help -a'. This seems like a 
simple thing for adding a test to ensure that this works now and in the 
future.

>
> http.c
The code in here seems to be logic for Windows-specific SSL backends, so 
is not covered by this report.

> preload-index.c
> ae9af12287  63) struct progress_data *pd = p->progress;
> ae9af12287  65) pthread_mutex_lock(&pd->mutex);
> ae9af12287  66) pd->n += last_nr - nr;
> ae9af12287  67) display_progress(pd->progress, pd->n);
> ae9af12287  68) pthread_mutex_unlock(&pd->mutex);
> ae9af12287  69) last_nr = nr;
> ae9af12287  83) struct progress_data *pd = p->progress;
> ae9af12287  85) pthread_mutex_lock(&pd->mutex);
> ae9af12287  86) display_progress(pd->progress, pd->n + last_nr);
> ae9af12287  87) pthread_mutex_unlock(&pd->mutex);
> ae9af12287 118) pd.progress = start_delayed_progress(_("Refreshing 
> index"), index->cache_nr);
> ae9af12287 119) pthread_mutex_init(&pd.mutex, NULL);
> ae9af12287 132) p->progress = &pd;

There's a lot of stuff going on with showing progress on index writes. 
While the commit message states the progress doesn't show up for 3 
seconds, perhaps that can be tweaked to be in the millisecond range for 
a test?

> read-cache.c

(There's a lot of progress stuff here, too.)

There are a lot of lines introduced by the IEOT extension in these commits:

 > Ben Peart      3255089ad: ieot: add Index Entry Offset Table (IEOT) 
extension
 > Ben Peart      3b1d9e045: eoie: add End of Index Entry (EOIE) extension
 > Ben Peart      77ff1127a: read-cache: load cache entries on worker 
threads
 > Ben Peart      abb4bb838: read-cache: load cache extensions on a 
worker thread
 > Ben Peart      c780b9cfe: config: add new index.threads config setting
 > Ben Peart      d1664e73a: add: speed up cmd_add() by utilizing 
read_cache_preload()
 > Ben Peart      fa655d841: checkout: optimize "git checkout -b 
<new_branch>"

> revision.c
> b45424181e 2951) c->object.flags |= UNINTERESTING;
> b45424181e 2957) mark_parents_uninteresting(c);

These blocks are currently unreachable because we do not use the new 
topo-order logic when there are UNINTERESTING commits. (This will be 
replaced after we have generation numbers v2.) I could force using this 
logic in a `git log --topo-order A..B` query when GIT_TEST_COMMIT_GRAPH 
is enabled.

