Git Test Coverage Report (Thurs. June 27)

Git Test Coverage Report (Thurs. June 27)

[Derrick Stolee]

Here is today's test coverage report.

Thanks,
-Stolee

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

---

pu	f8df19e612605e564ac8006e61428c8b95391624
jch	e1093b448f0c8fd5069bcff6cd20af2e1ab1bcef
next	ee2e6308ae95ce7ba2fabedcc43f5f741e71c152
master	8dca754b1e874719a732bc9ab7b0e14b21b1bc10
master@{1}	origin/maint


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

archive.c
47f956bd 421) err = get_tree_entry(ar_args->repo,
47f956bd 422)      &tree->object.oid,

builtin/cat-file.c
b14ed5ad 529) warning("This repository uses promisor remotes. Some objects may not be loaded.");

builtin/clone.c
fbb4a33c 407) die_errno(_("failed to create directory '%s'"), pathname);
fbb4a33c 409) die_errno(_("failed to stat '%s'"), pathname);
d95432d7 428) die_errno(_("failed to start iterator over '%s'"), src->buf);
d95432d7 466) strbuf_setlen(src, src_len);
d95432d7 467) die(_("failed to iterate over '%s'"), src->buf);

builtin/env--helper.c
b4f207f3 23) die(_("unrecognized --type argument, %s"), arg);
b4f207f3 67) default_int = 0;
b4f207f3 82) default_ulong = 0;
b4f207f3 90) BUG("unknown <type> value");

builtin/fetch-pack.c

builtin/pack-objects.c
820a5361 859) BUG("configured exclusion wasn't configured");
820a5361 2794) die(_("value of uploadpack.blobpackfileuri must be "
820a5361 2797) die(_("object already configured in another "

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"));

builtin/rev-list.c
9b93d269 476) die(

config.c
2e43cd4c 998)     value, name, cf->name, _(error_type));
2e43cd4c 1007)     value, name, cf->name, _(error_type));
2e43cd4c 1010)     value, name, cf->name, _(error_type));
2e43cd4c 1013)     value, name, cf->name, _(error_type));

dir-iterator.c
50e85c40 92) warning_errno("error closing directory '%s'",
655af733 126) warning_errno("failed to stat '%s'", iter->base.path.buf);
655af733 159) goto error_out;
50e85c40 174) warning_errno("error reading directory '%s'",
655af733 176) if (iter->flags & DIR_ITERATOR_PEDANTIC)
655af733 177) goto error_out;
655af733 188) if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
655af733 189) goto error_out;
c40f077a 216) int saved_errno = errno;
c40f077a 218) errno = saved_errno;
c40f077a 219) warning_errno("error closing directory '%s'",

fast-import.c
35d7cdbe 2565) char *buf = read_object_with_reference(the_repository,
35d7cdbe 2566)        &n->oid,

fetch-pack.c
820a5361 1397) die("expected '<hash> <uri>', got: %s\n", reader->line);
820a5361 1402) die("expected DELIM");
820a5361 1529) die("fetch-pack: unable to spawn http-fetch");
820a5361 1533) die("fetch-pack: expected keep then TAB at start of http-fetch output");
820a5361 1538) die("fetch-pack: expected hash then LF at end of http-fetch output");
820a5361 1545) die("fetch-pack: unable to finish http-fetch");
820a5361 1549) die("fetch-pack: pack downloaded from %s does not match expected hash %.*s",
820a5361 1550)     uri, (int) the_hash_algo->hexsz,
820a5361 1551)     packfile_uris.items[i].string);

http-fetch.c

http.c
3d908bb8 2304) target ? hash_to_hex(target->hash) : base_url,

list-objects-filter-options.c
d3d10e56 44) BUG("filter_options already populated");
d3d10e56 236) die(_("multiple filter-specs cannot be combined"));
9b93d269 286) BUG("no filter_spec available for this filter");
1e43301f 321) return;

list-objects-filter.c
1e43301f 583) BUG("expected oidset to be cleared already");

list-objects.c
aa36553a 210) ctx->show_object(obj, base->buf, ctx->show_data);

match-trees.c
3fe87a7f 294) if (get_tree_entry(r, hash2, del_prefix, shifted, &mode))

promisor-remote.c
db27dca5 25) die(_("Remote with no URL"));
48de3158 61) warning(_("promisor remote name cannot begin with '/': %s"),
48de3158 63) return NULL;
faf2abf4 93) previous->next = r->next;
4ca9474e 108) return git_config_string(&core_partial_clone_filter_default,
fa3d1b63 139) return 0;
9e27beaa 202) static int remove_fetched_oids(struct repository *repo,
9e27beaa 206) int i, remaining_nr = 0;
9e27beaa 207) int *remaining = xcalloc(oid_nr, sizeof(*remaining));
9e27beaa 208) struct object_id *old_oids = *oids;
9e27beaa 211) for (i = 0; i < oid_nr; i++)
9e27beaa 212) if (oid_object_info_extended(repo, &old_oids[i], NULL,
9e27beaa 214) remaining[i] = 1;
9e27beaa 215) remaining_nr++;
9e27beaa 218) if (remaining_nr) {
9e27beaa 219) int j = 0;
9e27beaa 220) new_oids = xcalloc(remaining_nr, sizeof(*new_oids));
9e27beaa 221) for (i = 0; i < oid_nr; i++)
9e27beaa 222) if (remaining[i])
9e27beaa 223) oidcpy(&new_oids[j++], &old_oids[i]);
9e27beaa 224) *oids = new_oids;
9e27beaa 225) if (to_free)
9e27beaa 226) free(old_oids);
9e27beaa 229) free(remaining);
9e27beaa 231) return remaining_nr;
9e27beaa 248) if (remaining_nr == 1)
9e27beaa 249) continue;
9e27beaa 250) remaining_nr = remove_fetched_oids(repo, &remaining_oids,
9e27beaa 252) if (remaining_nr) {
9e27beaa 253) to_free = 1;
9e27beaa 254) continue;
9e27beaa 262) free(remaining_oids);

protocol.c

remote-curl.c

repo-settings.c
0a01e977 13) int rate = git_config_int(key, value);
0a01e977 14) if (rate >= 3) {
0a01e977 15) UPDATE_DEFAULT(rs->core_commit_graph, 1);
0a01e977 16) UPDATE_DEFAULT(rs->gc_write_commit_graph, 1);
c5c84f32 17) UPDATE_DEFAULT(rs->index_version, 4);
3172404b 19) if (rate >= 5) {
3172404b 20) UPDATE_DEFAULT(rs->pack_use_sparse, 1);
0a01e977 22) return 0;

strbuf.c
fb819691 818) strbuf_addf(buf, "%u.%2.2u ",
fb819691 822) strbuf_addstr(buf, _("GiB"));

t/helper/test-dir-iterator.c
655af733 24) die("invalid option '%s'", *argv);
655af733 28) die("dir-iterator needs exactly one non-option argument");
9bd70db7 46) printf("[?] ");

t/helper/test-match-trees.c
3fe87a7f 23) shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1);

upload-pack.c
a8d662e3 130) return readsz;
820a5361 149) BUG("packfile_uris requires sideband-all");
9b93d269 221) sq_quote_buf(&buf, spec);
a8d662e3 354) send_client_data(1, output_state.buffer, output_state.used);
820a5361 1387) string_list_clear(&data->uri_protocols, 0);

wrapper.c

Commits introducting uncovered code:
Ævar Arnfjörð Bjarmason	b4f207f3 env--helper: new undocumented builtin wrapping git_env_*()
Ævar Arnfjörð Bjarmason	2e43cd4c config.c: refactor die_bad_number() to not call gettext() early
Christian Couder	fa3d1b63 promisor-remote: parse remote.*.partialclonefilter
Christian Couder	faf2abf4 promisor-remote: use repository_format_partial_clone
Christian Couder	4ca9474e Move core_partial_clone_filter_default to promisor-remote.c
Christian Couder	db27dca5 Remove fetch-object.{c,h} in favor of promisor-remote.{c,h}
Christian Couder	48de3158 Add initial support for many promisor remotes
Christian Couder	9e27beaa promisor-remote: implement promisor_remote_get_direct()
Christian Couder	b14ed5ad Use promisor_remote_get_direct() and has_promisor_remote()
Daniel Ferreira	9bd70db7 dir-iterator: add tests for dir-iterator API
Denton Liu	f39a9c65 remote: add --save-to-push option to git remote set-url
Derrick Stolee	0a01e977 repo-settings: create core.featureAdoptionRate setting
Derrick Stolee	c5c84f32 repo-settings: use index.version=4 by default
Derrick Stolee	3172404b repo-settings: pack.useSparse=true
Dimitriy Ryazantcev	fb819691 l10n: localizable upload progress messages
Jonathan Tan	820a5361 upload-pack: send part of packfile response as uri
Jonathan Tan	a8d662e3 upload-pack: refactor reading of pack-objects out
Junio C Hamano	3d908bb8 Merge branch 'jt/fetch-cdn-offload' into pu
Matheus Tavares	655af733 dir-iterator: add flags parameter to dir_iterator_begin
Matheus Tavares	50e85c40 dir-iterator: refactor state machine model
Matheus Tavares	d95432d7 clone: use dir-iterator to avoid explicit dir traversal
Matheus Tavares	fbb4a33c clone: extract function from copy_or_link_directory
Matheus Tavares	c40f077a dir-iterator: use warning_errno when possible
Matthew DeVore	aa36553a list-objects-filter: make API easier to use
Matthew DeVore	1e43301f list-objects-filter: implement composite filters
Matthew DeVore	d3d10e56 list-objects-filter-options: move error check up
Matthew DeVore	9b93d269 list-objects-filter-options: make filter_spec a string_list
Nguyễn Thái Ngọc Duy	35d7cdbe sha1-file.c: remove the_repo from read_object_with_reference()
Nguyễn Thái Ngọc Duy	3fe87a7f match-trees.c: remove the_repo from shift_tree*()
Nguyễn Thái Ngọc Duy	47f956bd tree-walk.c: remove the_repo from get_tree_entry()


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

blame.c
1fc73384 990) return;
a07a9776 1599) continue;
ae3f36de 2417) continue;

builtin/blame.c

builtin/checkout.c
d16dc428 1345) warning(_("you are switching branch while bisecting"));
3ec37ad1 1370) die(_("'%s' cannot be used with '%s'"), "--discard-changes", "--merge");
c9c935f6 1508) BUG("make up your mind, you need to take _something_");
183fb44f 1540) opts->checkout_index = 0;
183fb44f 1550) BUG("these flags should be non-negative by now");
c9c935f6 1611) die(_("could not resolve %s"), opts->from_treeish);

builtin/commit-graph.c
c2bc6e6a 203) return 1;

builtin/fetch.c
cdbd70c4 88) fetch_show_forced_updates = git_config_bool(k, v);
cdbd70c4 89) return 0;
377444b4 1011) warning(_("It took %.2f seconds to check forced updates. You can use '--no-show-forced-updates'\n"),
377444b4 1013) warning(_("or run 'git config fetch.showForcedUpdates false' to avoid this check.\n"));

builtin/ls-files.c
272b3f2a 672) die(_("--debug-json cannot be used with other file selection options"));
272b3f2a 674) die(_("--debug-json cannot be used with %s"), "--resolve-undo");
272b3f2a 676) die(_("--debug-json cannot be used with %s"), "--with-tree");
272b3f2a 678) die(_("--debug-json cannot be used with %s"), "--debug");
272b3f2a 701) die("index file corrupt");

builtin/multi-pack-index.c

builtin/pull.c
3883c551 556) argv_array_push(&args, "--show-forced-updates");
3883c551 558) argv_array_push(&args, "--no-show-forced-updates");

builtin/rebase.c
d559f502 759) ret = error(_("could not remove '%s'"),
526c03b5 1272) goto done;
10572de1 1288) goto done;
d559f502 1675) error(_("could not remove '%s'"),

cache-tree.c
fd335a21 605) ret = NULL; /* not the whole tree */

commit-graph.c
118bd570 277) chunk_repeated = 1;
118bd570 347) warning(_("commit-graph has no base graphs chunk"));
118bd570 348) return 0;
5c84b339 401) break;
d4f4d60f 550) BUG("NULL commit-graph");
d4f4d60f 553) die(_("invalid commit position. commit-graph is likely corrupt"));
d4f4d60f 613) die(_("invalid commit position. commit-graph is likely corrupt"));
6c622f9f 1057) continue;
6c622f9f 1331) error(_("failed to write correct number of base graph ids"));
6c622f9f 1332) return -1;
6c622f9f 1376) error(_("unable to create '%s'"), ctx->graph_name);
6c622f9f 1377) return -1;
6c622f9f 1451) return -1;
6c622f9f 1477) error(_("unable to open commit-graph chain file"));
6c622f9f 1478) return -1;
135a7123 1489) error(_("failed to rename base commit-graph file"));
6c622f9f 1509) error(_("failed to rename temporary commit-graph file"));
6c622f9f 1510) return -1;
c523035c 1539) break;
c523035c 1554) ctx->num_commit_graphs_after = 1;
c523035c 1555) ctx->new_base_graph = NULL;
1771be90 1631) die(_("unexpected duplicate commit id %s"),
1771be90 1632)     oid_to_hex(&ctx->commits.list[i]->object.oid));
c2bc6e6a 1806) ctx->oids.alloc = split_opts->max_commits;

config.c
07b2c0ea 283) return 0;

delta-islands.c
bdbdf42f 467) fprintf(stderr, _("Marked %d islands, done.\n"), island_counter);

dir.c
3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,
3b2385cf 2845) jw_object_bool(jw, "valid", ucd->valid);
3b2385cf 2846) jw_object_bool(jw, "check-only", ucd->check_only);
3b2385cf 2847) jw_object_stat_data(jw, "stat", &ucd->stat_data);
3b2385cf 2848) jw_object_string(jw, "exclude-oid", oid_to_hex(&ucd->exclude_oid));
3b2385cf 2849) jw_object_inline_begin_array(jw, "untracked");
3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)
3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);
3b2385cf 2852) jw_end(jw);
3b2385cf 2854) jw_object_inline_begin_object(jw, "dirs");
3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {
3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);
3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);
3b2385cf 2858) jw_end(jw);
3b2385cf 2860) jw_end(jw);
3b2385cf 2861) }
3b2385cf 2958) jw_object_inline_begin_object(jw, "root");
3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);
3b2385cf 2960) jw_end(jw);

fetch-pack.c
5a88583b 914) else if (args->depth > 0 || is_repository_shallow(the_repository))
0778b293 928) print_verbose(args, _("Server supports %s"), "multi_ack");
0778b293 936) print_verbose(args, _("Server supports %s"), "side-band");
5a88583b 974) } else if (args->deepen_since)
5a88583b 979) } else if (args->deepen_not)
5a88583b 983) else if (args->deepen_relative)

kwset.c
08e04506 45) BUG("Cannot allocate a negative amount: %ld", size);

midx.c
d01bf2e6 478) close_pack(packs->info[packs->nr].p);
d01bf2e6 479) FREE_AND_NULL(packs->info[packs->nr].p);
19575c7c 738) BUG("object %s is in an expired pack with int-id %d",
19575c7c 865) error(_("did not see pack-file %s to drop"),
19575c7c 867) drop_index++;
19575c7c 868) missing_drops++;
19575c7c 869) i--;
19575c7c 876) result = 1;
19575c7c 877) goto cleanup;
19575c7c 1194) return 0;
19575c7c 1209) continue;
ce1e4a10 1248) return 0;
ce1e4a10 1275) continue;
ce1e4a10 1295) continue;
ce1e4a10 1297) continue;
ce1e4a10 1329) return 0;
ce1e4a10 1350) error(_("could not start pack-objects"));
ce1e4a10 1351) result = 1;
ce1e4a10 1352) goto cleanup;
ce1e4a10 1369) error(_("could not finish pack-objects"));
ce1e4a10 1370) result = 1;
ce1e4a10 1371) goto cleanup;

oidmap.c
0a66ac47 42) hashmap_entry_init(&entry, oidhash(key));

oidset.c

pack-bitmap-write.c
05805d74 378) static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)
d2bc62b1 385) hash_pos = kh_get_oid_map(writer.reused, *oid);
05805d74 425) reused_bitmap = find_reused_bitmap(&chosen->object.oid);
05805d74 432) reused_bitmap = find_reused_bitmap(&cm->object.oid);

packfile.c
8434e85d 372) strbuf_release(&buf);
8434e85d 373) return;

pager.c
cd1096b2 197) fputs("\r\033[K", stderr);

progress.c
fbe464c0 121) fprintf(stderr, "  %s%s", counters_sb->buf,
fbe464c0 127) fprintf(stderr, "%s:\n  %s%s",

read-cache.c
8eeabe15 1752) ret = error(_("index uses %.4s extension, which we do not understand"),
ee70c128 1754) if (advice_unknown_index_extension) {
ee70c128 1755) warning(_("ignoring optional %.4s index extension"), ext);
ee70c128 1756) advise(_("This is likely due to the file having been written by a newer\n"
272b3f2a 2026) jw_object_true(jw, "assume_unchanged");
272b3f2a 2030) jw_object_true(jw, "skip_worktree");
272b3f2a 2032) jw_object_intmax(jw, "stage", ce_stage(ce));
f0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);
f0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(
f0f544da 3673) return do_read_ieot_extension(istate, index, extsize);

ref-filter.c
28438e84 1500) strbuf_addstr(&desc, _("no branch"));

sequencer.c
37e9ee5c 293) ret = -1;
37e9ee5c 311) ret = error(_("could not remove '%s'"), buf.buf);
30652115 2198)  (*bol == ' ' || *bol == '\t'))
30652115 2201) ret = -1;
516dc810 2668) in_progress_error = _("revert is already in progress");
516dc810 2669) in_progress_advice =
516dc810 2671) break;
516dc810 2678) BUG("unexpected action in create_seq_dir");
3fa2e2d8 2777) return error(_("cannot resolve HEAD"));
3fa2e2d8 2860) if (!rollback_is_safe())
3fa2e2d8 2861) goto give_advice;
3fa2e2d8 2873) BUG("unexpected action in sequencer_skip");
3fa2e2d8 2877) return error(_("failed to skip the commit"));

split-index.c
1f825794 29) goto done;

t/helper/test-example-decorate.c
0ebbcf70 29) one = lookup_unknown_object(&one_oid);
0ebbcf70 30) two = lookup_unknown_object(&two_oid);
0ebbcf70 59) three = lookup_unknown_object(&three_oid);

t/helper/test-oidmap.c
11510dec 52) if (get_oid(p1, &oid)) {
11510dec 53) printf("Unknown oid: %s\n", p1);
11510dec 54) continue;
11510dec 58) FLEX_ALLOC_STR(entry, name, p2);
11510dec 59) oidcpy(&entry->entry.oid, &oid);
11510dec 62) oidmap_put(&map, entry);
11510dec 97) if (get_oid(p1, &oid)) {
11510dec 98) printf("Unknown oid: %s\n", p1);
11510dec 99) continue;
11510dec 103) entry = oidmap_remove(&map, &oid);
11510dec 106) puts(entry ? entry->name : "NULL");
11510dec 107) free(entry);

upload-pack.c
62b89d43 534) if (parse_oid_hex(namebuf, &oid, &p) || *p != '\n')
d0229abd 537) o = lookup_object(the_repository, &oid);

wt-status.c
0a53561a 1114) strbuf_addf(&sb, _("\n"

Commits introducting uncovered code:
Barret Rhoden	1fc73384 blame: optionally track line fingerprints during fill_blame_origin()
Barret Rhoden	ae3f36de blame: add the ability to ignore commits and their changes
Barret Rhoden	a07a9776 blame: use the fingerprint heuristic to match ignored lines
Christian Couder	11510dec t/helper: add test-oidmap.c
Denton Liu	07b2c0ea config: learn the "onbranch:" includeIf condition
Denton Liu	526c03b5 rebase: refactor can_fast_forward into goto tower
Denton Liu	10572de1 rebase: fast-forward --onto in more cases
Derrick Stolee	ce1e4a10 midx: implement midx_repack()
Derrick Stolee	8434e85d repack: refactor pack deletion for future use
Derrick Stolee	d01bf2e6 midx: refactor permutation logic and pack sorting
Derrick Stolee	1771be90 commit-graph: merge commit-graph chains
Derrick Stolee	c523035c commit-graph: allow cross-alternate chains
Derrick Stolee	135a7123 commit-graph: add --split option to builtin
Derrick Stolee	6c622f9f commit-graph: write commit-graph chains
Derrick Stolee	d4f4d60f commit-graph: prepare for commit-graph chains
Derrick Stolee	19575c7c multi-pack-index: implement 'expire' subcommand
Derrick Stolee	118bd570 commit-graph: add base graphs chunk
Derrick Stolee	3883c551 pull: add --[no-]show-forced-updates passthrough
Derrick Stolee	377444b4 fetch: warn about forced updates in branch listing
Derrick Stolee	5c84b339 commit-graph: load commit-graph chains
Derrick Stolee	cdbd70c4 fetch: add --[no-]show-forced-updates argument
Derrick Stolee	c2bc6e6a commit-graph: create options for split files
Jeff Hostetler	0a53561a status: warn when a/b calculation takes too long
Jeff King	d0229abd object: convert lookup_object() to use object_id
Jeff King	62b89d43 upload-pack: rename a "sha1" variable to "oid"
Jeff King	bdbdf42f delta-islands: respect progress flag
Jeff King	0ebbcf70 object: convert lookup_unknown_object() to use object_id
Jeff King	d2bc62b1 pack-bitmap: convert khash_sha1 maps into kh_oid_map
Jeff King	05805d74 pack-bitmap-write: convert some helpers to use object_id
Johannes Schindelin	08e04506 kwset: allow building with GCC 8
Jonathan Nieder	ee70c128 index: offer advice for unknown index extensions
Junio C Hamano	0a66ac47 Merge branch 'cc/test-oidmap' into jch
Matthew DeVore	28438e84 ref-filter: sort detached HEAD lines firstly
Nguyễn Thái Ngọc Duy	183fb44f restore: add --worktree and --staged
Nguyễn Thái Ngọc Duy	fd335a21 cache-tree.c: dump "TREE" extension as json
Nguyễn Thái Ngọc Duy	272b3f2a ls-files: add --json to dump the index
Nguyễn Thái Ngọc Duy	3ec37ad1 switch: add --discard-changes
Nguyễn Thái Ngọc Duy	3b2385cf dir.c: dump "UNTR" extension as json
Nguyễn Thái Ngọc Duy	1f825794 split-index.c: dump "link" extension as json
Nguyễn Thái Ngọc Duy	f0f544da read-cache.c: dump "IEOT" extension as json
Nguyễn Thái Ngọc Duy	5a88583b fetch-pack: print all relevant supported capabilities with -v -v
Nguyễn Thái Ngọc Duy	8eeabe15 read-cache.c: dump common extension info in json
Nguyễn Thái Ngọc Duy	c9c935f6 restore: take tree-ish from --source option instead
Nguyễn Thái Ngọc Duy	0778b293 fetch-pack: move capability names out of i18n strings
Nguyễn Thái Ngọc Duy	d16dc428 switch: allow to switch in the middle of bisect
Phillip Wood	30652115 status: do not report errors in sequencer/todo
Phillip Wood	d559f502 rebase --abort/--quit: cleanup refs/rewritten
Phillip Wood	37e9ee5c sequencer: return errors from sequencer_remove_state()
Rohit Ashiwal	516dc810 sequencer: add advice for revert
Rohit Ashiwal	3fa2e2d8 cherry-pick/revert: add --skip option
SZEDER Gábor	fbe464c0 progress: use term_clear_line()
SZEDER Gábor	cd1096b2 pager: add a helper function to clear the last line in the terminal


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

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

builtin/commit.c

builtin/gc.c
e103f727 691) return 1;

commit-graph.c
ef5b83f2 906) error(_("error opening index for %s"), packname.buf);
ef5b83f2 907) return -1;
4c9efe85 946) continue;
b2c83060 969) display_progress(ctx->progress, ctx->approx_nr_objects);
238def57 1039) error(_("unable to create leading directories of %s"),
238def57 1041) return -1;
e103f727 1158) error(_("the commit graph format cannot write %d commits"), count_distinct);
e103f727 1159) res = -1;
e103f727 1160) goto cleanup;
e103f727 1169) error(_("too many commits to write graph"));
e103f727 1170) res = -1;
e103f727 1171) goto cleanup;

name-hash.c
568a05c5 348) assert(begin >= 0);
568a05c5 350) int mid = begin + ((end - begin) >> 1);

packfile.c
921d49be 1275) COPY_ARRAY(poi_stack, small_poi_stack, poi_stack_nr);
921d49be 1685) COPY_ARRAY(delta_stack, small_delta_stack,

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

sh-i18n--envsubst.c
568a05c5 252)   size_t j = j1 + ((j2 - j1) >> 1);

Commits introducting uncovered code:
Derrick Stolee	e103f727 commit-graph: return with errors during write
Derrick Stolee	ef5b83f2 commit-graph: extract fill_oids_from_packs()
Derrick Stolee	4c9efe85 commit-graph: extract fill_oids_from_commit_hex()
Derrick Stolee	b2c83060 commit-graph: extract fill_oids_from_all_packs()
Derrick Stolee	238def57 commit-graph: extract write_commit_graph_file()
Nickolai Belakovski	2582083f ref-filter: add worktreepath atom
Philip Oakley	1fde99cf doc branch: provide examples for listing remote tracking branches
René Scharfe	568a05c5 cleanup: fix possible overflow errors in binary search, part 2
René Scharfe	921d49be use COPY_ARRAY for copying arrays


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

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/log.c
13cdf780 873) return 0;

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

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);

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 "

list-objects-filter-options.c
5c03bc8b 94) strbuf_addf(errbuf, _("invalid filter-spec '%s'"), arg);

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);

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 King	97387c8b am: read interactive input from stdin
Jeff King	6e7baf24 am: drop tty requirement for --interactive
Jeff King	76a7bc09 cmd_{read,write}_tree: rename "unused" variable that is used
Jeff King	7bd9631b read-cache: drop unused parameter from threaded load
Johannes Schindelin	7877ac3d bisect--helper: verify HEAD could be parsed before continuing
Matthew DeVore	5c03bc8b list-objects-filter-options: error is localizeable
Nguyễn Thái Ngọc Duy	f3f8311e merge: add --quit
Nguyễn Thái Ngọc Duy	1de16aec worktree add: sanitize worktree names

Re: Git Test Coverage Report (Thurs. June 27)

[Derrick Stolee]

Here are some interesting sections I found when examining the test coverage
report. I am only highlighting these sections because they seem to include
non-trivial logic. In some cases, maybe the code isn't needed.

On 6/27/2019 1:05 PM, Derrick Stolee wrote:
> promisor-remote.c
> db27dca5 25) die(_("Remote with no URL"));
> 48de3158 61) warning(_("promisor remote name cannot begin with '/': %s"),
> 48de3158 63) return NULL;
> faf2abf4 93) previous->next = r->next;
> 4ca9474e 108) return git_config_string(&core_partial_clone_filter_default,
> fa3d1b63 139) return 0;
> 9e27beaa 202) static int remove_fetched_oids(struct repository *repo,
> 9e27beaa 206) int i, remaining_nr = 0;
> 9e27beaa 207) int *remaining = xcalloc(oid_nr, sizeof(*remaining));
> 9e27beaa 208) struct object_id *old_oids = *oids;
> 9e27beaa 211) for (i = 0; i < oid_nr; i++)
> 9e27beaa 212) if (oid_object_info_extended(repo, &old_oids[i], NULL,
> 9e27beaa 214) remaining[i] = 1;
> 9e27beaa 215) remaining_nr++;
> 9e27beaa 218) if (remaining_nr) {
> 9e27beaa 219) int j = 0;
> 9e27beaa 220) new_oids = xcalloc(remaining_nr, sizeof(*new_oids));
> 9e27beaa 221) for (i = 0; i < oid_nr; i++)
> 9e27beaa 222) if (remaining[i])
> 9e27beaa 223) oidcpy(&new_oids[j++], &old_oids[i]);
> 9e27beaa 224) *oids = new_oids;
> 9e27beaa 225) if (to_free)
> 9e27beaa 226) free(old_oids);
> 9e27beaa 229) free(remaining);
> 9e27beaa 231) return remaining_nr;
> 9e27beaa 248) if (remaining_nr == 1)
> 9e27beaa 249) continue;
> 9e27beaa 250) remaining_nr = remove_fetched_oids(repo, &remaining_oids,
> 9e27beaa 252) if (remaining_nr) {
> 9e27beaa 253) to_free = 1;
> 9e27beaa 254) continue;
> 9e27beaa 262) free(remaining_oids);

Christian: this section continues to be untested, but I think you were
working on creating tests for this.

> repo-settings.c
> 0a01e977 13) int rate = git_config_int(key, value);
> 0a01e977 14) if (rate >= 3) {
> 0a01e977 15) UPDATE_DEFAULT(rs->core_commit_graph, 1);
> 0a01e977 16) UPDATE_DEFAULT(rs->gc_write_commit_graph, 1);
> c5c84f32 17) UPDATE_DEFAULT(rs->index_version, 4);
> 3172404b 19) if (rate >= 5) {
> 3172404b 20) UPDATE_DEFAULT(rs->pack_use_sparse, 1);
> 0a01e977 22) return 0;

These are mine. Since no one has been complaining about the design
of core.featureAdoptionRate in ds/early-access [1], I'll move forward
to add some tests for this setting. It may come in the form of a
GIT_TEST_ADOPTION_RATE environment variable so it has wider coverage
across the test suite. I may even add explicit tests that demonstrate
the new defaults enabled by core.featureAdoptionRate are overridden by
explicit config settings. index.version is a good candidate.

[1] https://public-inbox.org/git/pull.254.v2.git.gitgitgadget@gmail.com/

> t/helper/test-match-trees.c
> 3fe87a7f 23) shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1);

Duy: here is another example of a conversion to "struct repository *" but
since it is using the_repository here it is definitely safe (no worse than before).

> builtin/fetch.c
> cdbd70c4 88) fetch_show_forced_updates = git_config_bool(k, v);
> cdbd70c4 89) return 0;

Mine again. I should explicitly test the fetch.showForcedUpdates config option,
especially against the '--no-show-forced-updates' argument (to show the argument
wins).

> builtin/pull.c
> 3883c551 556) argv_array_push(&args, "--show-forced-updates");
> 3883c551 558) argv_array_push(&args, "--no-show-forced-updates");

Not sure if this is super-important to test. It's a simple argument pass-through.
But also maybe I change my 'git fetch' tests to be 'git pull' tests and it covers
the 'git fetch' builtin at the same time.

> dir.c
> 3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,
> 3b2385cf 2845) jw_object_bool(jw, "valid", ucd->valid);
> 3b2385cf 2846) jw_object_bool(jw, "check-only", ucd->check_only);
> 3b2385cf 2847) jw_object_stat_data(jw, "stat", &ucd->stat_data);
> 3b2385cf 2848) jw_object_string(jw, "exclude-oid", oid_to_hex(&ucd->exclude_oid));
> 3b2385cf 2849) jw_object_inline_begin_array(jw, "untracked");
> 3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)
> 3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);
> 3b2385cf 2852) jw_end(jw);
> 3b2385cf 2854) jw_object_inline_begin_object(jw, "dirs");
> 3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {
> 3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);
> 3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);
> 3b2385cf 2858) jw_end(jw);
> 3b2385cf 2860) jw_end(jw);
> 3b2385cf 2861) }
> 3b2385cf 2958) jw_object_inline_begin_object(jw, "root");
> 3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);
> 3b2385cf 2960) jw_end(jw);

Duy: I know you were working on some tests for these options. This is specifically
in the "untracked cache" mode, so enabling the cache with at least one entry and
running --debug-json should be sufficient.

> pack-bitmap-write.c
> 05805d74 378) static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)
> d2bc62b1 385) hash_pos = kh_get_oid_map(writer.reused, *oid);
> 05805d74 425) reused_bitmap = find_reused_bitmap(&chosen->object.oid);
> 05805d74 432) reused_bitmap = find_reused_bitmap(&cm->object.oid);

Peff: it is interesting that these portions are not covered previously. (Your change
is clearly mechanical and does not change the correctness.) In particular, lines 425
and 432 are in two blocks of an if/else with one further inside a loop. The loop
should always have at least one run, so this if/else isn't even covered.

> read-cache.c
> 8eeabe15 1752) ret = error(_("index uses %.4s extension, which we do not understand"),
> ee70c128 1754) if (advice_unknown_index_extension) {
> ee70c128 1755) warning(_("ignoring optional %.4s index extension"), ext);
> ee70c128 1756) advise(_("This is likely due to the file having been written by a newer\n"
> 272b3f2a 2026) jw_object_true(jw, "assume_unchanged");
> 272b3f2a 2030) jw_object_true(jw, "skip_worktree");
> 272b3f2a 2032) jw_object_intmax(jw, "stage", ce_stage(ce));
> f0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);
> f0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(
> f0f544da 3673) return do_read_ieot_extension(istate, index, extsize);

Duy: more JSON output cases that could be interesting to cover.

> t/helper/test-example-decorate.c
> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);
> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);
> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);

Peff: again interesting that these lines you refactored were not covered, especially
because they are part of a test helper. Perhaps the tests they were intended for are
now defunct?
 
> t/helper/test-oidmap.c
> 11510dec 52) if (get_oid(p1, &oid)) {
> 11510dec 53) printf("Unknown oid: %s\n", p1);
> 11510dec 54) continue;
> 11510dec 58) FLEX_ALLOC_STR(entry, name, p2);
> 11510dec 59) oidcpy(&entry->entry.oid, &oid);
> 11510dec 62) oidmap_put(&map, entry);

Christian: this block looks like the test-oidmap helper never uses the "add"
subcommand. Is that correct?

> 11510dec 97) if (get_oid(p1, &oid)) {
> 11510dec 98) printf("Unknown oid: %s\n", p1);
> 11510dec 99) continue;
> 11510dec 103) entry = oidmap_remove(&map, &oid);
> 11510dec 106) puts(entry ? entry->name : "NULL");
> 11510dec 107) free(entry);

Similarly, this block means we are not using the "remove" subcommand.


Thanks,
-Stolee

Re: Git Test Coverage Report (Thurs. June 27)

[Jeff King]

On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:

> > pack-bitmap-write.c
> > 05805d74 378) static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)
> > d2bc62b1 385) hash_pos = kh_get_oid_map(writer.reused, *oid);
> > 05805d74 425) reused_bitmap = find_reused_bitmap(&chosen->object.oid);
> > 05805d74 432) reused_bitmap = find_reused_bitmap(&cm->object.oid);
> 
> Peff: it is interesting that these portions are not covered previously. (Your change
> is clearly mechanical and does not change the correctness.) In particular, lines 425
> and 432 are in two blocks of an if/else with one further inside a loop. The loop
> should always have at least one run, so this if/else isn't even covered.

One of 425 or 432 must run if we enter that "for(;;)" loop (in the
latter case, "next" is non-zero, so we enter the inner loop at least
once).

I think that the whole loop starting at line 409 is not exercised by the
test suite, because we hit the early return above it when there are
fewer than 100 commits to index.

I think this would exercise it, at the cost of making the test more
expensive:

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 82d7f7f6a5..8ed6982dcb 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -21,7 +21,7 @@ has_any () {
 }
 
 test_expect_success 'setup repo with moderate-sized history' '
-	for i in $(test_seq 1 10)
+	for i in $(test_seq 1 100)
 	do
 		test_commit $i
 	done &&

It would be nice if we had a "test_commits_bulk" that used fast-import
to create larger numbers of commits.

-Peff

[PATCH 0/6] easy bulk commit creation in tests

[Jeff King]

On Fri, Jun 28, 2019 at 02:41:03AM -0400, Jeff King wrote:

> I think this would exercise it, at the cost of making the test more
> expensive:
> 
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 82d7f7f6a5..8ed6982dcb 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -21,7 +21,7 @@ has_any () {
>  }
>  
>  test_expect_success 'setup repo with moderate-sized history' '
> -	for i in $(test_seq 1 10)
> +	for i in $(test_seq 1 100)
>  	do
>  		test_commit $i
>  	done &&
> 
> It would be nice if we had a "test_commits_bulk" that used fast-import
> to create larger numbers of commits.

So here's a patch to do that. Writing the bulk commit function was a fun
exercise, and I found a couple other places to apply it, too, shaving
off ~7.5 seconds from my test runs. Not ground-breaking, but I think
it's nice to have a solution where we don't have to be afraid to
generate a bunch of commits.

I'm sure there are other spots that could be converted, too (either ones
that are slow loops now, or ones that use fast-import themselves but
could be made more readable by using the helper), but I stopped digging
after finding the low-hanging fruit here.

  [1/6]: test-lib: introduce test_commit_bulk
  [2/6]: t5310: increase the number of bitmapped commits
  [3/6]: t3311: use test_commit_bulk
  [4/6]: t5702: use test_commit_bulk
  [5/6]: t5703: use test_commit_bulk
  [6/6]: t6200: use test_commit_bulk

 t/t3311-notes-merge-fanout.sh      |  10 +--
 t/t5310-pack-bitmaps.sh            |  15 +---
 t/t5702-protocol-v2.sh             |  10 +--
 t/t5703-upload-pack-ref-in-want.sh |   4 +-
 t/t6200-fmt-merge-msg.sh           |   7 +-
 t/test-lib-functions.sh            | 131 +++++++++++++++++++++++++++++
 6 files changed, 144 insertions(+), 33 deletions(-)

-Peff

[PATCH 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

Some tests need to create a string of commits. Doing this with
test_commit is very heavy-weight, as it needs at least one process per
commit (and in fact, uses several).

For bulk creation, we can do much better by using fast-import, but it's
often a pain to generate the input. Let's provide a helper to do so.

We'll use t5310 as a guinea pig, as it has three 10-commit loops. Here
are hyperfine results before and after:

  [before]
  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]
    Range (min … max):    2.250 s …  3.210 s    10 runs

  [after]
  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]
    Range (min … max):    1.999 s …  2.590 s    10 runs

So we're over 20% faster, while making the callers slightly shorter. We
added a lot more lines in test-lib-function.sh, of course, and the
helper is way more featureful than we need here. But my hope is that it
will be flexible enough to use in more places.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5310-pack-bitmaps.sh |  15 +----
 t/test-lib-functions.sh | 131 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 134 insertions(+), 12 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index a26c8ba9a2..3aab7024ca 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -21,15 +21,9 @@ has_any () {
 }
 
 test_expect_success 'setup repo with moderate-sized history' '
-	for i in $(test_seq 1 10)
-	do
-		test_commit $i
-	done &&
+	test_commit_bulk --id=file 10 &&
 	git checkout -b other HEAD~5 &&
-	for i in $(test_seq 1 10)
-	do
-		test_commit side-$i
-	done &&
+	test_commit_bulk --id=side 10 &&
 	git checkout master &&
 	bitmaptip=$(git rev-parse master) &&
 	blob=$(echo tagged-blob | git hash-object -w --stdin) &&
@@ -106,10 +100,7 @@ test_expect_success 'clone from bitmapped repository' '
 '
 
 test_expect_success 'setup further non-bitmapped commits' '
-	for i in $(test_seq 1 10)
-	do
-		test_commit further-$i
-	done
+	test_commit_bulk --id=further 10
 '
 
 rev_list_tests 'partial bitmap'
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0367cec5fd..32a1db81a3 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -233,6 +233,137 @@ test_merge () {
 	git tag "$1"
 }
 
+# Similar to test_commit, but efficiently create <nr> commits, each with a
+# unique number $n (from 1 to <nr> by default) in the commit message.
+#
+# Usage: test_commit_bulk [options] <nr>
+#   -C <dir>:
+#	Run all git commands in directory <dir>
+#   --ref=<n>:
+#	ref on which to create commits (default: HEAD)
+#   --start=<n>:
+#	number commit messages from <n> (default: 1)
+#   --message=<msg>:
+#	use <msg> as the commit mesasge (default: "commit $n")
+#   --filename=<fn>:
+#	modify <fn> in each commit (default: $n.t)
+#   --contents=<string>:
+#	place <string> in each file (default: "content $n")
+#   --id=<string>:
+#	shorthand to use <string> and $n in message, filename, and contents
+#
+# The message, filename, and contents strings are evaluated by the shell inside
+# double-quotes, with $n set to the current commit number. So you can do:
+#
+#   test_commit_bulk --filename=file --contents='modification $n'
+#
+# to have every commit touch the same file, but with unique content. Spaces are
+# OK, but you must escape any metacharacters (like backslashes or
+# double-quotes) you do not want expanded.
+#
+test_commit_bulk () {
+	indir=
+	ref=HEAD
+	n=1
+	message='commit $n'
+	filename='$n.t'
+	contents='content $n'
+	while test $# -gt 0
+	do
+		case "$1" in
+		-C)
+			indir=$2
+			shift
+			;;
+		--ref=*)
+			ref=${1#--*=}
+			;;
+		--start=*)
+			n=${1#--*=}
+			;;
+		--message=*)
+			message=${1#--*=}
+			;;
+		--filename=*)
+			filename=${1#--*=}
+			;;
+		--contents=*)
+			contents=${1#--*=}
+			;;
+		--id=*)
+			message="${1#--*=} \$n"
+			filename="${1#--*=}-\$n.t"
+			contents="${1#--*=} \$n"
+			;;
+		-*)
+			BUG "invalid test_commit_bulk option: $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	total=$1
+
+	in_dir=${indir:+-C "$indir"}
+
+	# Any test_tick calls inside the loop will not affect our outer
+	# timestamp, since it's on the left-hand side of a pipe. So start with
+	# a known value now, increment in the loop, and then do the matching
+	# math here. The final test_tick updates the $GIT_* variables
+	test_tick
+	cur_time=$test_tick
+	test_tick=$((test_tick + total))
+	test_tick
+
+
+	{
+		# A "reset ... from" instructs fastimport to build on an
+		# existing branch tip rather than trying to overwrite.
+		if tip=$(git ${indir:+ -C "$indir"} \
+			 rev-parse --verify "$ref" 2>/dev/null)
+		then
+			echo "reset $ref"
+			echo "from $tip"
+		fi
+
+		while test "$total" -gt 0
+		do
+			echo "commit $ref" &&
+			printf 'author %s <%s> %s\n' \
+				"$GIT_AUTHOR_NAME" \
+				"$GIT_AUTHOR_EMAIL" \
+				"$cur_time -0700" &&
+			printf 'committer %s <%s> %s\n' \
+				"$GIT_COMMITTER_NAME" \
+				"$GIT_COMMITTER_EMAIL" \
+				"$cur_time -0700" &&
+			echo "data <<EOF" &&
+			eval "echo \"$message\"" &&
+			echo "EOF" &&
+			eval "echo \"M 644 inline $filename\"" &&
+			echo "data <<EOF" &&
+			eval "echo \"$contents\"" &&
+			echo "EOF" &&
+			echo &&
+			n=$((n + 1)) &&
+			cur_time=$((cur_time + 1)) &&
+			total=$((total - 1)) ||
+			echo "poison fast-import stream"
+		done
+	} | git ${indir:+ -C "$indir"} \
+		-c fastimport.unpacklimit=0 \
+		fast-import || return 1
+
+	# If we updated HEAD, then be nice and update the index and working
+	# tree, too.
+	if test "$ref" = "HEAD"
+	then
+		git ${indir:+ -C "$indir"} checkout -f HEAD || return 1
+	fi
+}
+
 # This function helps systems where core.filemode=false is set.
 # Use it instead of plain 'chmod +x' to set or unset the executable bit
 # of a file in the working directory and add it to the index.
-- 
2.22.0.768.gd89de1e449

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Derrick Stolee]

On 6/28/2019 5:39 AM, Jeff King wrote:
> Some tests need to create a string of commits. Doing this with
> test_commit is very heavy-weight, as it needs at least one process per
> commit (and in fact, uses several).
> 
> For bulk creation, we can do much better by using fast-import, but it's
> often a pain to generate the input. Let's provide a helper to do so.

What a quick turnaround! I'm happy to nerd-snipe you here, and am glad
the result was so positive.

> We'll use t5310 as a guinea pig, as it has three 10-commit loops. Here
> are hyperfine results before and after:
> 
>   [before]
>   Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
>     Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]
>     Range (min … max):    2.250 s …  3.210 s    10 runs
> 
>   [after]
>   Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
>     Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]
>     Range (min … max):    1.999 s …  2.590 s    10 runs

I ran these tests on my Windows machine, where the process startup time is
a higher cost. The improvement is noticeable from just watching the test lines
pause on the steps creating the commits.

 Before: 30.8-31.2s
  After: 23.5-23.8s

> So we're over 20% faster, while making the callers slightly shorter.

I see about the same relative measurement (~23%). The callers are a bit
cleaner, which is good. They are also slightly less clear of what's
happening, but that's the cost of abstraction. Definitely worth it in
this case!
 
> +# Similar to test_commit, but efficiently create <nr> commits, each with a
> +# unique number $n (from 1 to <nr> by default) in the commit message.
> +#
> +# Usage: test_commit_bulk [options] <nr>
> +#   -C <dir>:
> +#	Run all git commands in directory <dir>
> +#   --ref=<n>:
> +#	ref on which to create commits (default: HEAD)
> +#   --start=<n>:
> +#	number commit messages from <n> (default: 1)
> +#   --message=<msg>:
> +#	use <msg> as the commit mesasge (default: "commit $n")
> +#   --filename=<fn>:
> +#	modify <fn> in each commit (default: $n.t)
> +#   --contents=<string>:
> +#	place <string> in each file (default: "content $n")
> +#   --id=<string>:
> +#	shorthand to use <string> and $n in message, filename, and contents
> +#
> +# The message, filename, and contents strings are evaluated by the shell inside
> +# double-quotes, with $n set to the current commit number. So you can do:
> +#
> +#   test_commit_bulk --filename=file --contents='modification $n'
> +#
> +# to have every commit touch the same file, but with unique content. Spaces are
> +# OK, but you must escape any metacharacters (like backslashes or
> +# double-quotes) you do not want expanded.
> +#

I appreciate all the documentation here!

> +		while test "$total" -gt 0
> +		do
> +			echo "commit $ref" &&
> +			printf 'author %s <%s> %s\n' \
> +				"$GIT_AUTHOR_NAME" \
> +				"$GIT_AUTHOR_EMAIL" \
> +				"$cur_time -0700" &&
> +			printf 'committer %s <%s> %s\n' \
> +				"$GIT_COMMITTER_NAME" \
> +				"$GIT_COMMITTER_EMAIL" \
> +				"$cur_time -0700" &&
> +			echo "data <<EOF" &&
> +			eval "echo \"$message\"" &&
> +			echo "EOF" &&
> +			eval "echo \"M 644 inline $filename\"" &&
> +			echo "data <<EOF" &&
> +			eval "echo \"$contents\"" &&
> +			echo "EOF" &&
> +			echo &&
> +			n=$((n + 1)) &&
> +			cur_time=$((cur_time + 1)) &&
> +			total=$((total - 1)) ||
> +			echo "poison fast-import stream"
> +		done

I am not very good at the nitty-gritty details of our scripts, but
looking at this I wonder if there is a cleaner and possibly faster
way to do this loop. The top thing on my mind are the 'eval "echo X"'
lines. If they start processes, then we can improve the performance.
If not, then it may not be worth it.

In wonder if instead we could create some format string outside the
loop and then pass the values that change between iterations into
that format string.

Thanks,
-Stolee

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> way to do this loop. The top thing on my mind are the 'eval "echo X"'
> lines. If they start processes, then we can improve the performance.
> If not, then it may not be worth it.

Sigh.  

Do you mean 'echo' run inside 'eval' is one extra process?  In most
modern shells, it is a built-in and you need another process.

Do you mean 'eval' running anything is one extra process?  Because
anything done inside eval must be visible to the shell running it,
e.g.

	var=myvar; eval "$var=val"

would evaluate string 'myvar=val' inside that shell itself and it
must be able to update the value of $myvar, whatever it does must
not add any extra process.

The primary reason why the loop in question uses eval is to allow
the callers to pass $n in single-quote to have it interpolated
lazily.

	message='message $n'
	for n in 1 2 3
	do
		echo "$message"
		eval "echo \"$message\""
	done

Each iteration, the first line gives

	message $n

which is the thing that gets passed to 'echo' in the second line, so
you'll see

	message $n
	message 1
	message $n
	message 2
	message $n
	message 3

as the result.

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

On Fri, Jun 28, 2019 at 08:35:28AM -0400, Derrick Stolee wrote:

> > +		while test "$total" -gt 0
> > +		do
> > +			echo "commit $ref" &&
> > +			printf 'author %s <%s> %s\n' \
> > +				"$GIT_AUTHOR_NAME" \
> > +				"$GIT_AUTHOR_EMAIL" \
> > +				"$cur_time -0700" &&
> > +			printf 'committer %s <%s> %s\n' \
> > +				"$GIT_COMMITTER_NAME" \
> > +				"$GIT_COMMITTER_EMAIL" \
> > +				"$cur_time -0700" &&
> > +			echo "data <<EOF" &&
> > +			eval "echo \"$message\"" &&
> > +			echo "EOF" &&
> > +			eval "echo \"M 644 inline $filename\"" &&
> > +			echo "data <<EOF" &&
> > +			eval "echo \"$contents\"" &&
> > +			echo "EOF" &&
> > +			echo &&
> > +			n=$((n + 1)) &&
> > +			cur_time=$((cur_time + 1)) &&
> > +			total=$((total - 1)) ||
> > +			echo "poison fast-import stream"
> > +		done
> 
> I am not very good at the nitty-gritty details of our scripts, but
> looking at this I wonder if there is a cleaner and possibly faster
> way to do this loop. The top thing on my mind are the 'eval "echo X"'
> lines. If they start processes, then we can improve the performance.
> If not, then it may not be worth it.

No, evals by themselves don't require a process.  That whole loop should
all happen as a single process (because it's the left-hand side of the
pipe, it does require a subshell).

We could drop even that process by writing into a temporary file. The
size probably wouldn't be a big deal, and I doubt the latency would even
matter much (and anyway, when you're running the tests in parallel
anyway, CPU time is the most important metric).

It might also make the code a little simpler, since we'd be running in
the main shell and could just use test_tick naturally (rather than the
manual addition hackery).

I'll take a look.

I wasn't super concerned with eliminating processes here as long as the
number of them is constant with respect to the number of commits we're
generating. The big improvement is taking, say, 300 test_commit calls
and turning it into a single bulk call. Replacing a single-commit
test_commit with this would be break-even at best.

> In wonder if instead we could create some format string outside the
> loop and then pass the values that change between iterations into
> that format string.

The evals should be fast. But they are potentially error-prone, since
callers have to pass something like --message='commit $n' with single
quotes to keep the "$" intact. But because all of our test snippets are
inside single-quotes already, you end up with:

  test_bulk_commit --message="commit \$n"

(though in practice most of the callers used the --id shorthand, which
neatly sidesteps this).

Since there's literally only one variable to interpolate, we could swap
this out for using printf formatters, and letting "%s" mean the same as
"$n". It should perform the same but is a bit less magical and a bit
harder to screw up. It would also be easier to handle if
test_commit_bulk eventually became C code. The only downside I can think
of is that you can't mention "%s" twice, but I find it hard to imagine a
caller would want that anyway.

So I'll also take a look at that.

-Peff

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +# The message, filename, and contents strings are evaluated by the shell inside
> +# double-quotes, with $n set to the current commit number. So you can do:
> +#
> +#   test_commit_bulk --filename=file --contents='modification $n'
> +#
> +# to have every commit touch the same file, but with unique content. Spaces are
> +# OK, but you must escape any metacharacters (like backslashes or
> +# double-quotes) you do not want expanded.

Nice.

> +test_commit_bulk () {
> +	indir=
> + ...
> +	while test $# -gt 0
> +	do
> +		case "$1" in
> +		-C)
> +			indir=$2
> +			shift
> +			;;
> + ...
> +		esac
> +		shift
> +	done
> +	total=$1
> +
> +	in_dir=${indir:+-C "$indir"}

I thought that this assignment to $in_dir would be unnecessary if we
parsed -C directly into it, i.e.

		...
		-C)
			in_dir="-C $indir"
			shift
			;;
		...

but you probably could pass -C '' to defeat an $in_dir that was set
earlier by using a separate variable?

Messages and other stuff are made `eval`-safe, but this one does not
care much about quoting, which made me curious.

Reading further, though, I do not seem to see where this variable is
referred to, and that is the answer to my puzzlement.  This must be
a leftover that was written once before but no longer is used.  We
can remove $in_dir while keeping the initialization and assignment
to $indir as-is, I think.

All uses of $indir in the remainder of the function look $IFS-safe,
which is good.

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

On Fri, Jun 28, 2019 at 10:53:41AM -0700, Junio C Hamano wrote:

> > +	in_dir=${indir:+-C "$indir"}
> 
> I thought that this assignment to $in_dir would be unnecessary if we
> parsed -C directly into it, i.e.

Heh, sorry for the confusion. That in_dir is leftover cruft. I was
trying to see if I could then expand it as:

  git $in_dir some-cmd ...

to make the git calls more readable. But that doesn't work if $indir has
whitespace, so I abandoned it (we're relying on whitespace splitting
between "-C" and the argument, but we don't want it split on the
argument).

I _also_ mispelled $indir as $in_dir in that attempt, which meant that
the leftover line did not break anything, and I didn't notice. But it
can just go away.

> 		-C)
> 			in_dir="-C $indir"
> 			shift
> 			;;
> 		...
> 
> but you probably could pass -C '' to defeat an $in_dir that was set
> earlier by using a separate variable?

I don't know if "-C ''" works with Git or not. I had contemplated
defaulting indir to ".", so that we did:

  git -C . command ...

which I think would work (at the minor cost of a useless chdir() inside
the C process).

In the end I just stole the technique that test_commit uses. It's a
little ugly, but there are only 3 calls.

> Reading further, though, I do not seem to see where this variable is
> referred to, and that is the answer to my puzzlement.  This must be
> a leftover that was written once before but no longer is used.  We
> can remove $in_dir while keeping the initialization and assignment
> to $indir as-is, I think.

Yes. :)

> All uses of $indir in the remainder of the function look $IFS-safe,
> which is good.

Yeah, I think it should be (though since most callers pass relative
paths for these kind of one-off -C uses, it's actually pretty rare for
it to matter).

-Peff

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 28 2019, Jeff King wrote:

> Some tests need to create a string of commits. Doing this with
> test_commit is very heavy-weight, as it needs at least one process per
> commit (and in fact, uses several).
>
> For bulk creation, we can do much better by using fast-import, but it's
> often a pain to generate the input. Let's provide a helper to do so.
>
> We'll use t5310 as a guinea pig, as it has three 10-commit loops. Here
> are hyperfine results before and after:
>
>   [before]
>   Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
>     Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]
>     Range (min … max):    2.250 s …  3.210 s    10 runs
>
>   [after]
>   Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
>     Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]
>     Range (min … max):    1.999 s …  2.590 s    10 runs
>
> So we're over 20% faster, while making the callers slightly shorter. We
> added a lot more lines in test-lib-function.sh, of course, and the
> helper is way more featureful than we need here. But my hope is that it
> will be flexible enough to use in more places.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5310-pack-bitmaps.sh |  15 +----
>  t/test-lib-functions.sh | 131 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 134 insertions(+), 12 deletions(-)
>
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index a26c8ba9a2..3aab7024ca 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -21,15 +21,9 @@ has_any () {
>  }
>
>  test_expect_success 'setup repo with moderate-sized history' '
> -	for i in $(test_seq 1 10)
> -	do
> -		test_commit $i
> -	done &&
> +	test_commit_bulk --id=file 10 &&
>  	git checkout -b other HEAD~5 &&
> -	for i in $(test_seq 1 10)
> -	do
> -		test_commit side-$i
> -	done &&
> +	test_commit_bulk --id=side 10 &&
>  	git checkout master &&
>  	bitmaptip=$(git rev-parse master) &&
>  	blob=$(echo tagged-blob | git hash-object -w --stdin) &&
> @@ -106,10 +100,7 @@ test_expect_success 'clone from bitmapped repository' '
>  '
>
>  test_expect_success 'setup further non-bitmapped commits' '
> -	for i in $(test_seq 1 10)
> -	do
> -		test_commit further-$i
> -	done
> +	test_commit_bulk --id=further 10
>  '
>
>  rev_list_tests 'partial bitmap'
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 0367cec5fd..32a1db81a3 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -233,6 +233,137 @@ test_merge () {
>  	git tag "$1"
>  }
>
> +# Similar to test_commit, but efficiently create <nr> commits, each with a
> +# unique number $n (from 1 to <nr> by default) in the commit message.

Is it intentional not to follow test_commit's convention of creating a
tag as well? If so it would be helpful to note that difference here, or
rather, move this documentation to t/README where test_commit and
friends are documented.

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

On Fri, Jun 28, 2019 at 08:44:38PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > +# Similar to test_commit, but efficiently create <nr> commits, each with a
> > +# unique number $n (from 1 to <nr> by default) in the commit message.
> 
> Is it intentional not to follow test_commit's convention of creating a
> tag as well? If so it would be helpful to note that difference here, or

Yes, it was intentional. I have long hated that feature, as there are
many tests have to bend over backwards to deal with the reachability
implications of adding the extra tag (not to mention the waste of a
process). Likewise, I have long hated the implicit-argument-ordering of
test_commit that make it hard to set some optional arguments but not
others (hence the double-dash parameters).

I had planned to add a "--tag" parameter if anybody ever wanted one. But
we can call out that difference explicitly. Or alternatively, stop
saying "like test_commit" and just say "Efficiently create <nr>
commits".

> rather, move this documentation to t/README where test_commit and
> friends are documented.

Ugh. I had no idea that documentation even existed. Because of course
test_commit _is_ documented next to its definition, and that
documentation has been kept up to date, unlike the far-away stale bits
in t/README.

-Peff

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Eric Sunshine]

On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:
> [...]
> For bulk creation, we can do much better by using fast-import, but it's
> often a pain to generate the input. Let's provide a helper to do so.
> [...]
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -233,6 +233,137 @@ test_merge () {
> +# Similar to test_commit, but efficiently create <nr> commits, each with a
> +# unique number $n (from 1 to <nr> by default) in the commit message.
> +#
> +# Usage: test_commit_bulk [options] <nr>
> +#   [...]
> +#
> +# The message, filename, and contents strings are evaluated by the shell inside
> +# double-quotes, with $n set to the current commit number. So you can do:
> +#
> +#   test_commit_bulk --filename=file --contents='modification $n'

Considering that test_commit_bulk() is intended to be used within a
test body, and considering that test bodies are almost always
encapsulated in single quotes, recommending single quoting the value
of --contents= seems contraindicated. Double quotes likely would be
better.

> +# to have every commit touch the same file, but with unique content. Spaces are
> +# OK, but you must escape any metacharacters (like backslashes or
> +# double-quotes) you do not want expanded.
> +#
> +test_commit_bulk () {
> +       [...]
> +       in_dir=${indir:+-C "$indir"}

Doesn't this suffer the problem in which some older/broken
shells[1][2][3][4] incorrectly expand this to:

    "-C <dir>"

rather than the expected:

    -C "<dir>"

? Is this something we still care about?

Same comment applies to other instances of ${indir:+-C "$indir"} below.

[1]: http://public-inbox.org/git/20160517215214.GA16905@sigill.intra.peff.net/
[2]: http://public-inbox.org/git/e3bfc53363b14826d828e1adffbbeea@74d39fa044aa309eaea14b9f57fe79c/
[3]: http://public-inbox.org/git/20160518010609.Horde.sM8QUFek6WMAAwho56DDob8@webmail.informatik.kit.edu/
[4]: http://public-inbox.org/git/1240044459-57227-1-git-send-email-ben@ben.com/

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[SZEDER Gábor]

On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:
> On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:

> > +# to have every commit touch the same file, but with unique content. Spaces are
> > +# OK, but you must escape any metacharacters (like backslashes or
> > +# double-quotes) you do not want expanded.
> > +#
> > +test_commit_bulk () {
> > +       [...]
> > +       in_dir=${indir:+-C "$indir"}
> 
> Doesn't this suffer the problem in which some older/broken
> shells[1][2][3][4] incorrectly expand this to:
> 
>     "-C <dir>"
> 
> rather than the expected:
> 
>     -C "<dir>"
> 
> ? Is this something we still care about?
> 
> Same comment applies to other instances of ${indir:+-C "$indir"} below.

I think we don't need any of those "${indir:+-C "$indir"}" parameter
expansions and could simply use 'git -C "$indir" cmd...' everywhere.
$indir is set to empty right at the start of the function, and 'git -C
"" ...' works and doesn't change the working directory.

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Eric Sunshine]

On Fri, Jun 28, 2019 at 7:04 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:
> > On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:
> > > +       in_dir=${indir:+-C "$indir"}
> >
> > Doesn't this suffer the problem in which some older/broken
> > shells[1][2][3][4] incorrectly [...]
>
> I think we don't need any of those "${indir:+-C "$indir"}" parameter
> expansions and could simply use 'git -C "$indir" cmd...' everywhere.
> $indir is set to empty right at the start of the function, and 'git -C
> "" ...' works and doesn't change the working directory.

I recall the discussion around the meaning of `-C ""` when that
command line option was introduced. The conclusion was that  the
zero-length argument should mean "this directory" since that's how `cd
""` behaves. However, I don't think that behavior ever got documented,
and it's not necessarily obvious. An alternative would be to default
'indir' to ".", which should give the same result and be easily
understood.

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

On Fri, Jun 28, 2019 at 07:46:02PM -0400, Eric Sunshine wrote:

> On Fri, Jun 28, 2019 at 7:04 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:
> > > On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:
> > > > +       in_dir=${indir:+-C "$indir"}
> > >
> > > Doesn't this suffer the problem in which some older/broken
> > > shells[1][2][3][4] incorrectly [...]
> >
> > I think we don't need any of those "${indir:+-C "$indir"}" parameter
> > expansions and could simply use 'git -C "$indir" cmd...' everywhere.
> > $indir is set to empty right at the start of the function, and 'git -C
> > "" ...' works and doesn't change the working directory.
> 
> I recall the discussion around the meaning of `-C ""` when that
> command line option was introduced. The conclusion was that  the
> zero-length argument should mean "this directory" since that's how `cd
> ""` behaves. However, I don't think that behavior ever got documented,
> and it's not necessarily obvious. An alternative would be to default
> 'indir' to ".", which should give the same result and be easily
> understood.

Yeah, I had considered that, too. I had mostly just copied the solution
from test_commit, thinking nobody would nitpick it. ;) But if everybody
likes ".", I think that is a bit more readable.

-Peff

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[SZEDER Gábor]

On Fri, Jun 28, 2019 at 07:46:02PM -0400, Eric Sunshine wrote:
> On Fri, Jun 28, 2019 at 7:04 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:
> > > On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:
> > > > +       in_dir=${indir:+-C "$indir"}
> > >
> > > Doesn't this suffer the problem in which some older/broken
> > > shells[1][2][3][4] incorrectly [...]
> >
> > I think we don't need any of those "${indir:+-C "$indir"}" parameter
> > expansions and could simply use 'git -C "$indir" cmd...' everywhere.
> > $indir is set to empty right at the start of the function, and 'git -C
> > "" ...' works and doesn't change the working directory.
> 
> I recall the discussion around the meaning of `-C ""` when that
> command line option was introduced. The conclusion was that  the
> zero-length argument should mean "this directory" since that's how `cd
> ""` behaves. However, I don't think that behavior ever got documented,

Although it's not documented (but see the patch below), we do
explicitly test it since 6a536e2076 (git: treat "git -C '<path>'" as a
no-op when <path> is empty, 2015-03-06) and e.g. our completion script
relies on this behavior.

> and it's not necessarily obvious. An alternative would be to default
> 'indir' to ".", which should give the same result and be easily
> understood.

That's fine for me as well.


   --- >8 ---

Subject: [PATCH] Document that 'git -C ""' works and doesn't change directory

It's been behaving so since 6a536e2076 (git: treat "git -C '<path>'"
as a no-op when <path> is empty, 2015-03-06).

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index f9b09db89b..a9deca0acb 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -57,7 +57,8 @@ help ...`.
 	Run as if git was started in '<path>' instead of the current working
 	directory.  When multiple `-C` options are given, each subsequent
 	non-absolute `-C <path>` is interpreted relative to the preceding `-C
-	<path>`.
+	<path>`.  If '<path>' is present but empty, e.g. `-C ""`, then the
+	current working directory is left unchanged.
 +
 This option affects options that expect path name like `--git-dir` and
 `--work-tree` in that their interpretations of the path names would be
-- 
2.22.0.589.g5bd7971b91

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> I recall the discussion around the meaning of `-C ""` when that
>> command line option was introduced. The conclusion was that  the
>> zero-length argument should mean "this directory" since that's how `cd
>> ""` behaves. However, I don't think that behavior ever got documented,
>
> Although it's not documented (but see the patch below), we do
> explicitly test it since 6a536e2076 (git: treat "git -C '<path>'" as a
> no-op when <path> is empty, 2015-03-06) and e.g. our completion script
> relies on this behavior.
>
>> and it's not necessarily obvious. An alternative would be to default
>> 'indir' to ".", which should give the same result and be easily
>> understood.
>
> That's fine for me as well.

I find the "an empty string is the same as a dot and means the
current directory" a bit counter-intuitive, but as long as we have
kept Git working that way for this long, we should document it, too.

For the tests in the patch in question, I think "-C ." is a good
thing to use.

Thanks.  Will queue.


>    --- >8 ---
>
> Subject: [PATCH] Document that 'git -C ""' works and doesn't change directory
>
> It's been behaving so since 6a536e2076 (git: treat "git -C '<path>'"
> as a no-op when <path> is empty, 2015-03-06).
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index f9b09db89b..a9deca0acb 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -57,7 +57,8 @@ help ...`.
>  	Run as if git was started in '<path>' instead of the current working
>  	directory.  When multiple `-C` options are given, each subsequent
>  	non-absolute `-C <path>` is interpreted relative to the preceding `-C
> -	<path>`.
> +	<path>`.  If '<path>' is present but empty, e.g. `-C ""`, then the
> +	current working directory is left unchanged.
>  +
>  This option affects options that expect path name like `--git-dir` and
>  `--work-tree` in that their interpretations of the path names would be

Re: [PATCH 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:

> > +# The message, filename, and contents strings are evaluated by the shell inside
> > +# double-quotes, with $n set to the current commit number. So you can do:
> > +#
> > +#   test_commit_bulk --filename=file --contents='modification $n'
> 
> Considering that test_commit_bulk() is intended to be used within a
> test body, and considering that test bodies are almost always
> encapsulated in single quotes, recommending single quoting the value
> of --contents= seems contraindicated. Double quotes likely would be
> better.

I hoped people reading it would realize that they would need to suppress
interpolation of $ one way or another. But I guess many people who touch
the test suite aren't actually prolific shell writers.

Anyway, I'm going to look into changing this to a printf string, which
would make this easier.

> > +       in_dir=${indir:+-C "$indir"}
> 
> Doesn't this suffer the problem in which some older/broken
> shells[1][2][3][4] incorrectly expand this to:

That line is leftover dead code; see my response to Junio.

> Same comment applies to other instances of ${indir:+-C "$indir"} below.

Those ones are fine because of the double-quotes (and whitespace
splitting on the replacement value happens before interpolation). Try
this:

  x=''
  y='with spaces'
  sh -c 'for i in "$@"; do echo arg: $i; done' -- \
    ${x:+-x "$x"} \
    ${y:+-y "$y"}

-Peff

[PATCH 2/6] t5310: increase the number of bitmapped commits

[Jeff King]

The bitmap index we compute in t5310 has only 20 commits in it. This
gives poor coverage of bitmap_writer_select_commits(), which simply
writes a bitmap for everything when there are fewer than 100 commits.

Let's bump the number of commits in the test to cover the more complex
code paths (this does drop coverage of the individual lines of the
trivial path, but the complex path does everything it does and more).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5310-pack-bitmaps.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3aab7024ca..6640329ebf 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -21,7 +21,7 @@ has_any () {
 }
 
 test_expect_success 'setup repo with moderate-sized history' '
-	test_commit_bulk --id=file 10 &&
+	test_commit_bulk --id=file 100 &&
 	git checkout -b other HEAD~5 &&
 	test_commit_bulk --id=side 10 &&
 	git checkout master &&
-- 
2.22.0.768.gd89de1e449

[PATCH 3/6] t3311: use test_commit_bulk

[Jeff King]

One of the tests in t3311 creates 300 commits by running "test_commit"
in a loop. This requires 900 processes. Instead, we can use
test_commit_bulk to do it with only four. This improves the runtime of
the script from:

  Benchmark #1: ./t3311-notes-merge-fanout.sh --root=/var/ram/git-tests
    Time (mean ± σ):      5.821 s ±  0.691 s    [User: 3.146 s, System: 2.782 s]
    Range (min … max):    4.783 s …  6.841 s    10 runs

to:

  Benchmark #1: ./t3311-notes-merge-fanout.sh --root=/var/ram/git-tests
    Time (mean ± σ):      1.743 s ±  0.116 s    [User: 1.144 s, System: 0.691 s]
    Range (min … max):    1.629 s …  1.994 s    10 runs

for an average speedup of over 70%.

Unfortunately we still have to run 300 instances of "git notes add",
since the point is to test the fanout that comes from adding notes one
by one.

Signed-off-by: Jeff King <peff@peff.net>
---
Re-reading the patch again, the notes numbering will be in reverse
order from the original. That's fine for the purposes of the test,
though I could probably fix it if we want to be pedantic.

 t/t3311-notes-merge-fanout.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh
index 93516ef67c..37151a3adc 100755
--- a/t/t3311-notes-merge-fanout.sh
+++ b/t/t3311-notes-merge-fanout.sh
@@ -114,12 +114,12 @@ cp expect_log_x expect_log_y
 test_expect_success 'Add a few hundred commits w/notes to trigger fanout (x -> y)' '
 	git update-ref refs/notes/y refs/notes/x &&
 	git config core.notesRef refs/notes/y &&
-	i=5 &&
-	while test $i -lt $num
+	test_commit_bulk --start=6 --id=commit $((num - 5)) &&
+	i=0 &&
+	while test $i -lt $((num - 5))
 	do
-		i=$(($i + 1)) &&
-		test_commit "commit$i" >/dev/null &&
-		git notes add -m "notes for commit$i" || return 1
+		git notes add -m "notes for commit$i" HEAD~$i || return 1
+		i=$((i + 1))
 	done &&
 	test "$(git rev-parse refs/notes/y)" != "$(git rev-parse refs/notes/x)" &&
 	# Expected number of commits and notes
-- 
2.22.0.768.gd89de1e449

[PATCH 4/6] t5702: use test_commit_bulk

[Jeff King]

There are two loops that create 32 commits each using test_commit. Using
test_commit_bulk speeds this up from:

  Benchmark #1: ./t5702-protocol-v2.sh --root=/var/ram/git-tests
    Time (mean ± σ):      5.409 s ±  0.513 s    [User: 2.382 s, System: 2.466 s]
    Range (min … max):    4.633 s …  5.927 s    10 runs

to:

  Benchmark #1: ./t5702-protocol-v2.sh --root=/var/ram/git-tests
    Time (mean ± σ):      3.956 s ±  0.242 s    [User: 1.775 s, System: 1.627 s]
    Range (min … max):    3.449 s …  4.239 s    10 runs

for an average savings of over 25%.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5702-protocol-v2.sh | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 5b33f625dd..011b81d4fc 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -499,10 +499,7 @@ test_expect_success 'upload-pack respects client shallows' '
 
 	# Add extra commits to the client so that the whole fetch takes more
 	# than 1 request (due to negotiation)
-	for i in $(test_seq 1 32)
-	do
-		test_commit -C client c$i
-	done &&
+	test_commit_bulk -C client --id=c 32 &&
 
 	git -C server checkout -b newbranch base &&
 	test_commit -C server client_wants &&
@@ -711,10 +708,7 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
 	# Create many commits to extend the negotiation phase across multiple
 	# requests, so that the server does not send "ready" in the first
 	# request.
-	for i in $(test_seq 1 32)
-	do
-		test_commit -C http_child c$i
-	done &&
+	test_commit_bulk -C http_child --id=c 32 &&
 
 	# After the acknowledgments section, pretend that a DELIM
 	# (0001) was sent instead of a FLUSH (0000).
-- 
2.22.0.768.gd89de1e449

[PATCH 5/6] t5703: use test_commit_bulk

[Jeff King]

There are two loops that create 33 commits each using test_commit. Using
test_commit_bulk speeds this up from:

  Benchmark #1: ./t5703-upload-pack-ref-in-want.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.142 s ±  0.161 s    [User: 1.136 s, System: 0.974 s]
    Range (min … max):    1.903 s …  2.401 s    10 runs

to:

  Benchmark #1: ./t5703-upload-pack-ref-in-want.sh --root=/var/ram/git-tests
    Time (mean ± σ):      1.440 s ±  0.114 s    [User: 737.7 ms, System: 615.4 ms]
    Range (min … max):    1.230 s …  1.604 s    10 runs

for an average savings of almost 33%.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5703-upload-pack-ref-in-want.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 0951d1bbdc..de4b6106ef 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -176,7 +176,7 @@ test_expect_success 'setup repos for change-while-negotiating test' '
 		git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo" "$LOCAL_PRISTINE" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
-		for i in $(test_seq 1 33); do test_commit s$i; done &&
+		test_commit_bulk --id=s 33 &&
 
 		# Add novel commits to upstream
 		git checkout master &&
@@ -287,7 +287,7 @@ test_expect_success 'setup repos for fetching with ref-in-want tests' '
 		git clone "file://$REPO" "$LOCAL_PRISTINE" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
-		for i in $(test_seq 1 33); do test_commit s$i; done &&
+		test_commit_bulk --id=s 33 &&
 
 		# Add novel commits to upstream
 		git checkout master &&
-- 
2.22.0.768.gd89de1e449

[PATCH 6/6] t6200: use test_commit_bulk

[Jeff King]

There's a loop that creates 30 commits using test_commit. Using
test_commit_bulk speeds this up from:

  Benchmark #1: ./t6200-fmt-merge-msg.sh --root=/var/ram/git-tests
    Time (mean ± σ):      1.926 s ±  0.240 s    [User: 1.055 s, System: 0.963 s]
    Range (min … max):    1.431 s …  2.166 s    10 runs

to:

  Benchmark #1: ./t6200-fmt-merge-msg.sh --root=/var/ram/git-tests
    Time (mean ± σ):      1.343 s ±  0.179 s    [User: 766.5 ms, System: 662.9 ms]
    Range (min … max):    1.032 s …  1.664 s    10 runs

for an average savings of over 30%.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t6200-fmt-merge-msg.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 93f23cfa82..d4e5af4338 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -66,12 +66,7 @@ test_expect_success setup '
 	git commit -a -m "Right #5" &&
 
 	git checkout -b long &&
-	i=0 &&
-	while test $i -lt 30
-	do
-		test_commit $i one &&
-		i=$(($i+1))
-	done &&
+	test_commit_bulk --start=0 --message="\$n" --filename=one 30 &&
 
 	git show-branch &&
 
-- 
2.22.0.768.gd89de1e449

Re: [PATCH 0/6] easy bulk commit creation in tests

[Johannes Schindelin]

Hi Peff,

On Fri, 28 Jun 2019, Jeff King wrote:

> On Fri, Jun 28, 2019 at 02:41:03AM -0400, Jeff King wrote:
>
> > It would be nice if we had a "test_commits_bulk" that used fast-import
> > to create larger numbers of commits.
>
> So here's a patch to do that.

I like the direction, especially because it would make it super easy to go
one step further that would probably make a huge difference on Windows: to
move `test_commit_bulk` to `test-tool commit-bulk`.

Thanks,
Dscho

Re: [PATCH 0/6] easy bulk commit creation in tests

[Jeff King]

On Fri, Jun 28, 2019 at 02:53:26PM +0200, Johannes Schindelin wrote:

> > > It would be nice if we had a "test_commits_bulk" that used fast-import
> > > to create larger numbers of commits.
> >
> > So here's a patch to do that.
> 
> I like the direction, especially because it would make it super easy to go
> one step further that would probably make a huge difference on Windows: to
> move `test_commit_bulk` to `test-tool commit-bulk`.

I actually considered going directly there, but I don't think it would
make a big difference. In the biggest case we dropped 900 processes to
4. If we really want to drop that to 1, we can:

  - use a temp-file to avoid the left-hand-pipe subshell

  - add a feature to fast-import to say "build on top of ref X", instead
    of using to use rev-parse to manually generates a "reset" line
    (maybe this is even possible already; I searched for it, but not
    very hard).

  - add a feature to fast-import to have it check out the result of HEAD
    if it was updated

The first one seems like an easy and obvious win that I'll explore. The
second one would be useful in general, I think, but I don't plan on
digging into it (unless somebody shows up with an easy existing way to
do it).

The third one is a little less elegant to me, because there are a lot of
questions about how to checkout (e.g., with "-f", what happens to
deleted files, etc).

-Peff

Re: [PATCH 0/6] easy bulk commit creation in tests

[Elijah Newren]

On Fri, Jun 28, 2019 at 6:32 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Jun 28, 2019 at 02:53:26PM +0200, Johannes Schindelin wrote:
>
> > > > It would be nice if we had a "test_commits_bulk" that used fast-import
> > > > to create larger numbers of commits.
> > >
> > > So here's a patch to do that.
> >
> > I like the direction, especially because it would make it super easy to go
> > one step further that would probably make a huge difference on Windows: to
> > move `test_commit_bulk` to `test-tool commit-bulk`.
>
> I actually considered going directly there, but I don't think it would
> make a big difference. In the biggest case we dropped 900 processes to
> 4. If we really want to drop that to 1, we can:
>
>   - use a temp-file to avoid the left-hand-pipe subshell
>
>   - add a feature to fast-import to say "build on top of ref X", instead
>     of using to use rev-parse to manually generates a "reset" line
>     (maybe this is even possible already; I searched for it, but not
>     very hard).

It already exists; quoting the fast-import documentation:

"The special case of restarting an incremental import from the
current branch value should be written as:

            from refs/heads/branch^0

The ^0 suffix is necessary as fast-import does not permit a branch
to start from itself, and the branch is created in memory before
the from command is even read from the input. Adding ^0 will force
fast-import to resolve the commit through Git's revision parsing
library, rather than its internal branch table, thereby loading in
the existing value of the branch."

>   - add a feature to fast-import to have it check out the result of HEAD
>     if it was updated

That'd be cool if you could work out the various special cases; it'd
be nice to avoid the 'git reset --hard HEAD' afterwards that I always
do.

> The third one is a little less elegant to me, because there are a lot of
> questions about how to checkout (e.g., with "-f", what happens to
> deleted files, etc).

There's a question with deleted files?  Why wouldn't you just delete
them from the index and working tree?  The more interesting questions
to me in this case is what to do if the index or working tree were
dirty before the import started; that seems like a mess, though maybe
it's just a case where you abort before even importing.  On a similar
note, though, there could have been an untracked file that is in the
way of a now-to-be-tracked file that you might not want to lose.

Elijah

Re: [PATCH 0/6] easy bulk commit creation in tests

[Jeff King]

On Sat, Jun 29, 2019 at 10:38:43AM -0600, Elijah Newren wrote:

> >   - add a feature to fast-import to say "build on top of ref X", instead
> >     of using to use rev-parse to manually generates a "reset" line
> >     (maybe this is even possible already; I searched for it, but not
> >     very hard).
> 
> It already exists; quoting the fast-import documentation:
> 
> "The special case of restarting an incremental import from the
> current branch value should be written as:
> 
>             from refs/heads/branch^0

Thank you! I looked over the documentation several times for this, but I
was looking for an individual command similar to "reset".

Unfortunately, I'm not sure we can use this to save ourselves a process.
What I really want to say is "if it does not exist, start from scratch
and otherwise build on the existing branch".

I couldn't figure out a way to do that without first finding out myself
if the branch exists (incurring a process) and then modifying my
fast-import stream appropriately.

So I don't think it actually shaves off our processes, but as I argued
elsewhere, I think it's probably not that important anyway. I do think
the end result is a bit simpler to read, too, as the while-loop now
generates the input in its entirety (I didn't reindent it yet in the
diff below):

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 9fd0fa2a89..4233f408e8 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -305,14 +305,11 @@ test_commit_bulk () {
 	done
 	total=$1
 
-	{
-		# A "reset ... from" instructs fastimport to build on an
-		# existing branch tip rather than trying to overwrite.
-		if tip=$(git -C "$indir" rev-parse --verify "$ref" 2>/dev/null)
-		then
-			echo "reset $ref"
-			echo "from $tip"
-		fi
+	add_from=
+	if git rev-parse --verify "$ref" >/dev/null 2>&1
+	then
+		add_from=t
+	fi
 
 		while test "$total" -gt 0
 		do
@@ -329,16 +326,16 @@ test_commit_bulk () {
 			echo "data <<EOF"
 			printf "$message\n" $n
 			echo "EOF"
+			test -n "$add_from" && echo "from $ref^0"
 			printf "M 644 inline $filename\n" $n
 			echo "data <<EOF"
 			printf "$contents\n" $n
 			echo "EOF"
 			echo
+			add_from=
 			n=$((n + 1))
 			total=$((total - 1))
-		done
-
-	} >"$tmpfile"
+		done >"$tmpfile"
 
 	git -C "$indir" \
 	    -c fastimport.unpacklimit=0 \

Actually, thinking about it more, avoiding the $() probably does save us
a subshell fork, too.

> > The third one is a little less elegant to me, because there are a lot of
> > questions about how to checkout (e.g., with "-f", what happens to
> > deleted files, etc).
> 
> There's a question with deleted files?  Why wouldn't you just delete
> them from the index and working tree?  The more interesting questions
> to me in this case is what to do if the index or working tree were
> dirty before the import started; that seems like a mess, though maybe
> it's just a case where you abort before even importing.  On a similar
> note, though, there could have been an untracked file that is in the
> way of a now-to-be-tracked file that you might not want to lose.

Sorry, by deleted I meant files that were already deleted in the working
tree or index, not ones our fast-import stream deleted. I.e,. the same
dirty case you're asking about. But modifications have the same problem,
too (I was thinking we'd just overwrite them as if the user had done
"cat >dirty-file" as part of their fast-import, but that only applies to
files they actually touched).

So "dirty" is definitely the right way to think about it.

-Peff

Re: [PATCH 0/6] easy bulk commit creation in tests

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 28 2019, Jeff King wrote:

> On Fri, Jun 28, 2019 at 02:41:03AM -0400, Jeff King wrote:
>
>> I think this would exercise it, at the cost of making the test more
>> expensive:
>>
>> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
>> index 82d7f7f6a5..8ed6982dcb 100755
>> --- a/t/t5310-pack-bitmaps.sh
>> +++ b/t/t5310-pack-bitmaps.sh
>> @@ -21,7 +21,7 @@ has_any () {
>>  }
>>
>>  test_expect_success 'setup repo with moderate-sized history' '
>> -	for i in $(test_seq 1 10)
>> +	for i in $(test_seq 1 100)
>>  	do
>>  		test_commit $i
>>  	done &&
>>
>> It would be nice if we had a "test_commits_bulk" that used fast-import
>> to create larger numbers of commits.
>
> So here's a patch to do that. Writing the bulk commit function was a fun
> exercise, and I found a couple other places to apply it, too, shaving
> off ~7.5 seconds from my test runs. Not ground-breaking, but I think
> it's nice to have a solution where we don't have to be afraid to
> generate a bunch of commits.

Nice.

Just a side-note: I've wondered how much we could speed up the tests in
other places if rather than doing setup all over the place we simply
created a few "template" repository shapes, and the common case for
tests would be to simply cp(1) those over.

I.e. for things like fsck etc. we really do need some specific
repository layout, but a lot of our tests are simply re-doing setup
slightly differently just to get things like "I want a few commits on a
few branches" or "set up a repo like <that> but with some remotes" etc.

Re: [PATCH 0/6] easy bulk commit creation in tests

[Jeff King]

On Fri, Jun 28, 2019 at 08:49:30PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > So here's a patch to do that. Writing the bulk commit function was a fun
> > exercise, and I found a couple other places to apply it, too, shaving
> > off ~7.5 seconds from my test runs. Not ground-breaking, but I think
> > it's nice to have a solution where we don't have to be afraid to
> > generate a bunch of commits.
> 
> Nice.
> 
> Just a side-note: I've wondered how much we could speed up the tests in
> other places if rather than doing setup all over the place we simply
> created a few "template" repository shapes, and the common case for
> tests would be to simply cp(1) those over.

That thought also occurred to me while writing this. I've worked with
test suites that have those kind of "fixtures" before, and I generally
like it less, for two reasons:

  - it's much harder to understand what's important about the fixture,
    because you're seeing the end result of running a bunch of commands.
    Whereas the individual commands that show you _how_ it was derived
    are generally instructive; they give you the steps that the author
    was thinking about.

  - it's more annoying to update them because you don't just change the
    instructions. You have to extract the fixture into a real repo,
    manipulate it, then convert it back into whatever storage format we
    use (which can't just be a real repo, because we don't allow
    embedding repos).

But what I think _would_ be cool is to treat the command instructions as
the source of truth, but allow caching of the on-disk state at certain
points in a test script. I.e., imagine that we annotate some test
snippets to say "I am setup, and it's OK if you don't run me every
time". Like:

  test_expect_success SETUP 'a really slow setup step' '
	for i in $(test_seq 1000)
	do
		test_commit horribly-slow-$i
	done
  '

and then the test harness would recognize the SETUP prereq as magical,
and:

  - look for t/cache/t1234.42.tar; if it exists, then replace the whole
    trash-dir state with it and skip the test

  - otherwise run the snippet and create t1234.42.tar for next time

Bonus points if you could specifically ask to cache t1234.57, even if it
_isn't_ marked as SETUP, and then restart the script from that point,
skipping over the intermediate tests (whether they were cached or not).
That would let you then run subsequent tests from a known point
instantly (e.g., if you're debugging some later test in the script and
want to run it over and over).

The downsides I see are:

  1. It doesn't exercise the setup snippets as much. The idea is that
     this shouldn't matter if it's just setup code, but I'm sure we do
     get some extra coverage from it. But any fixture-based scheme
     suffers from this.

  2. Cache invalidation (isn't it always?). If you changed the setup
     test or even fixed a bug elsewhere in Git, you'd want to re-run the
     setup steps. It's always OK to blow away the cache and get a fresh
     run, but sometimes it's easy to forget to do so (and the results
     can be confusing).

-Peff

[PATCH v2 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

Here's a re-roll of just the first patch of this series, since that was
the one that got all the comments (and the others are textually
independent).

The changes are:

  - drop the leftover in_dir assignment

  - replace ${indir:+} magic with defaulting indir=. (so we always pass
    it to "-C"

  - replace eval formatting magic with "%s" printf formatters (safer and
    gets rid of quoting issues in the callers).

  - use a tempfile to avoid significant logic on the left-hand subshell
    of a pipe. This actually  _doesn't_ save a process because we end up
    having to call "rm" to get rid of the tempfile. But I think it makes
    the logic easier to follow (we can get just call test_tick as normal
    in our loop), and as a bonus it leaves something you can inspect if
    the fast-import fails.

  - I dropped the comparison to test_commit in the documentation, since
    it isn't a direct replacement due to the lack of tag creation. I
    think that makes it clear enough.

    I _didn't_ move the documentation out to t/README. IMHO we should be
    moving in the opposite direction. But either way, I think it's
    something we should handle separately (either consistently moving it
    all into t/README, or moving it closer to the definitions).

I didn't re-run all of the timings, but I spot-checked a few and got
similar improvements (weirdly all of my timings, both before and after,
seem slightly faster today; apparently gremlins were slowing my machine
down yesterday?).

-- >8 --
Subject: test-lib: introduce test_commit_bulk
Some tests need to create a string of commits. Doing this with
test_commit is very heavy-weight, as it needs at least one process per
commit (and in fact, uses several).

For bulk creation, we can do much better by using fast-import, but it's
often a pain to generate the input. Let's provide a helper to do so.

We'll use t5310 as a guinea pig, as it has three 10-commit loops. Here
are hyperfine results before and after:

  [before]
  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]
    Range (min … max):    2.250 s …  3.210 s    10 runs

  [after]
  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]
    Range (min … max):    1.999 s …  2.590 s    10 runs

So we're over 20% faster, while making the callers slightly shorter. We
added a lot more lines in test-lib-function.sh, of course, and the
helper is way more featureful than we need here. But my hope is that it
will be flexible enough to use in more places.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5310-pack-bitmaps.sh |  15 +----
 t/test-lib-functions.sh | 123 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+), 12 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index a26c8ba9a2..3aab7024ca 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -21,15 +21,9 @@ has_any () {
 }
 
 test_expect_success 'setup repo with moderate-sized history' '
-	for i in $(test_seq 1 10)
-	do
-		test_commit $i
-	done &&
+	test_commit_bulk --id=file 10 &&
 	git checkout -b other HEAD~5 &&
-	for i in $(test_seq 1 10)
-	do
-		test_commit side-$i
-	done &&
+	test_commit_bulk --id=side 10 &&
 	git checkout master &&
 	bitmaptip=$(git rev-parse master) &&
 	blob=$(echo tagged-blob | git hash-object -w --stdin) &&
@@ -106,10 +100,7 @@ test_expect_success 'clone from bitmapped repository' '
 '
 
 test_expect_success 'setup further non-bitmapped commits' '
-	for i in $(test_seq 1 10)
-	do
-		test_commit further-$i
-	done
+	test_commit_bulk --id=further 10
 '
 
 rev_list_tests 'partial bitmap'
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0367cec5fd..9fd0fa2a89 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -233,6 +233,129 @@ test_merge () {
 	git tag "$1"
 }
 
+# Efficiently create <nr> commits, each with a unique number (from 1 to <nr>
+# by default) in the commit message.
+#
+# Usage: test_commit_bulk [options] <nr>
+#   -C <dir>:
+#	Run all git commands in directory <dir>
+#   --ref=<n>:
+#	ref on which to create commits (default: HEAD)
+#   --start=<n>:
+#	number commit messages from <n> (default: 1)
+#   --message=<msg>:
+#	use <msg> as the commit mesasge (default: "commit %s")
+#   --filename=<fn>:
+#	modify <fn> in each commit (default: %s.t)
+#   --contents=<string>:
+#	place <string> in each file (default: "content %s")
+#   --id=<string>:
+#	shorthand to use <string> and %s in message, filename, and contents
+#
+# The message, filename, and contents strings are evaluated by printf, with the
+# first "%s" replaced by the current commit number. So you can do:
+#
+#   test_commit_bulk --filename=file --contents="modification %s"
+#
+# to have every commit touch the same file, but with unique content.
+#
+test_commit_bulk () {
+	tmpfile=.bulk-commit.input
+	indir=.
+	ref=HEAD
+	n=1
+	message='commit %s'
+	filename='%s.t'
+	contents='content %s'
+	while test $# -gt 0
+	do
+		case "$1" in
+		-C)
+			indir=$2
+			shift
+			;;
+		--ref=*)
+			ref=${1#--*=}
+			;;
+		--start=*)
+			n=${1#--*=}
+			;;
+		--message=*)
+			message=${1#--*=}
+			;;
+		--filename=*)
+			filename=${1#--*=}
+			;;
+		--contents=*)
+			contents=${1#--*=}
+			;;
+		--id=*)
+			message="${1#--*=} %s"
+			filename="${1#--*=}-%s.t"
+			contents="${1#--*=} %s"
+			;;
+		-*)
+			BUG "invalid test_commit_bulk option: $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	total=$1
+
+	{
+		# A "reset ... from" instructs fastimport to build on an
+		# existing branch tip rather than trying to overwrite.
+		if tip=$(git -C "$indir" rev-parse --verify "$ref" 2>/dev/null)
+		then
+			echo "reset $ref"
+			echo "from $tip"
+		fi
+
+		while test "$total" -gt 0
+		do
+			test_tick &&
+			echo "commit $ref"
+			printf 'author %s <%s> %s\n' \
+				"$GIT_AUTHOR_NAME" \
+				"$GIT_AUTHOR_EMAIL" \
+				"$GIT_AUTHOR_DATE"
+			printf 'committer %s <%s> %s\n' \
+				"$GIT_COMMITTER_NAME" \
+				"$GIT_COMMITTER_EMAIL" \
+				"$GIT_COMMITTER_DATE"
+			echo "data <<EOF"
+			printf "$message\n" $n
+			echo "EOF"
+			printf "M 644 inline $filename\n" $n
+			echo "data <<EOF"
+			printf "$contents\n" $n
+			echo "EOF"
+			echo
+			n=$((n + 1))
+			total=$((total - 1))
+		done
+
+	} >"$tmpfile"
+
+	git -C "$indir" \
+	    -c fastimport.unpacklimit=0 \
+	    fast-import <"$tmpfile" || return 1
+
+	# This will be left in place on failure, which may aid debugging.
+	rm -f "$tmpfile"
+
+	# If we updated HEAD, then be nice and update the index and working
+	# tree, too.
+	if test "$ref" = "HEAD"
+	then
+		git -C "$indir" checkout -f HEAD || return 1
+	fi
+
+}
+
 # This function helps systems where core.filemode=false is set.
 # Use it instead of plain 'chmod +x' to set or unset the executable bit
 # of a file in the working directory and add it to the index.
-- 
2.22.0.775.g4ba9815492

Re: [PATCH v2 1/6] test-lib: introduce test_commit_bulk

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's a re-roll of just the first patch of this series, since that was
> the one that got all the comments (and the others are textually
> independent).

OK, will replace and then queue an adjustment for 6200 which used to
use \$n but now must use %s instead.  Let's see if people spot things
worth pointing out in the remainder of the series (or this one, of
course, but I found this step quite sensible).

Thanks.

Re: [PATCH v2 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

On Mon, Jul 01, 2019 at 03:24:35PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Here's a re-roll of just the first patch of this series, since that was
> > the one that got all the comments (and the others are textually
> > independent).
> 
> OK, will replace and then queue an adjustment for 6200 which used to
> use \$n but now must use %s instead.  Let's see if people spot things
> worth pointing out in the remainder of the series (or this one, of
> course, but I found this step quite sensible).

Urgh, I forgot I did have to tweak that later test. Thanks for noticing.

I do have one more update based on the comments from Elijah: using
"from" in the initial commit lets us simplify a few things (I posted the
incremental earlier in the thread, but here it is as a complete
replacement for patch 1).

-- >8 --
Subject: [PATCH v3] test-lib: introduce test_commit_bulk

Some tests need to create a string of commits. Doing this with
test_commit is very heavy-weight, as it needs at least one process per
commit (and in fact, uses several).

For bulk creation, we can do much better by using fast-import, but it's
often a pain to generate the input. Let's provide a helper to do so.

We'll use t5310 as a guinea pig, as it has three 10-commit loops. Here
are hyperfine results before and after:

  [before]
  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]
    Range (min … max):    2.250 s …  3.210 s    10 runs

  [after]
  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]
    Range (min … max):    1.999 s …  2.590 s    10 runs

So we're over 20% faster, while making the callers slightly shorter. We
added a lot more lines in test-lib-function.sh, of course, and the
helper is way more featureful than we need here. But my hope is that it
will be flexible enough to use in more places.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5310-pack-bitmaps.sh |  15 +----
 t/test-lib-functions.sh | 123 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+), 12 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index a26c8ba9a2..3aab7024ca 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -21,15 +21,9 @@ has_any () {
 }
 
 test_expect_success 'setup repo with moderate-sized history' '
-	for i in $(test_seq 1 10)
-	do
-		test_commit $i
-	done &&
+	test_commit_bulk --id=file 10 &&
 	git checkout -b other HEAD~5 &&
-	for i in $(test_seq 1 10)
-	do
-		test_commit side-$i
-	done &&
+	test_commit_bulk --id=side 10 &&
 	git checkout master &&
 	bitmaptip=$(git rev-parse master) &&
 	blob=$(echo tagged-blob | git hash-object -w --stdin) &&
@@ -106,10 +100,7 @@ test_expect_success 'clone from bitmapped repository' '
 '
 
 test_expect_success 'setup further non-bitmapped commits' '
-	for i in $(test_seq 1 10)
-	do
-		test_commit further-$i
-	done
+	test_commit_bulk --id=further 10
 '
 
 rev_list_tests 'partial bitmap'
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0367cec5fd..6083cf483a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -233,6 +233,129 @@ test_merge () {
 	git tag "$1"
 }
 
+# Efficiently create <nr> commits, each with a unique number (from 1 to <nr>
+# by default) in the commit message.
+#
+# Usage: test_commit_bulk [options] <nr>
+#   -C <dir>:
+#	Run all git commands in directory <dir>
+#   --ref=<n>:
+#	ref on which to create commits (default: HEAD)
+#   --start=<n>:
+#	number commit messages from <n> (default: 1)
+#   --message=<msg>:
+#	use <msg> as the commit mesasge (default: "commit %s")
+#   --filename=<fn>:
+#	modify <fn> in each commit (default: %s.t)
+#   --contents=<string>:
+#	place <string> in each file (default: "content %s")
+#   --id=<string>:
+#	shorthand to use <string> and %s in message, filename, and contents
+#
+# The message, filename, and contents strings are evaluated by printf, with the
+# first "%s" replaced by the current commit number. So you can do:
+#
+#   test_commit_bulk --filename=file --contents="modification %s"
+#
+# to have every commit touch the same file, but with unique content.
+#
+test_commit_bulk () {
+	tmpfile=.bulk-commit.input
+	indir=.
+	ref=HEAD
+	n=1
+	message='commit %s'
+	filename='%s.t'
+	contents='content %s'
+	while test $# -gt 0
+	do
+		case "$1" in
+		-C)
+			indir=$2
+			shift
+			;;
+		--ref=*)
+			ref=${1#--*=}
+			;;
+		--start=*)
+			n=${1#--*=}
+			;;
+		--message=*)
+			message=${1#--*=}
+			;;
+		--filename=*)
+			filename=${1#--*=}
+			;;
+		--contents=*)
+			contents=${1#--*=}
+			;;
+		--id=*)
+			message="${1#--*=} %s"
+			filename="${1#--*=}-%s.t"
+			contents="${1#--*=} %s"
+			;;
+		-*)
+			BUG "invalid test_commit_bulk option: $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	total=$1
+
+	add_from=
+	if git -C "$indir" rev-parse --verify "$ref"
+	then
+		add_from=t
+	fi
+
+	while test "$total" -gt 0
+	do
+		test_tick &&
+		echo "commit $ref"
+		printf 'author %s <%s> %s\n' \
+			"$GIT_AUTHOR_NAME" \
+			"$GIT_AUTHOR_EMAIL" \
+			"$GIT_AUTHOR_DATE"
+		printf 'committer %s <%s> %s\n' \
+			"$GIT_COMMITTER_NAME" \
+			"$GIT_COMMITTER_EMAIL" \
+			"$GIT_COMMITTER_DATE"
+		echo "data <<EOF"
+		printf "$message\n" $n
+		echo "EOF"
+		if test -n "$add_from"
+		then
+			echo "from $ref^0"
+			add_from=
+		fi
+		printf "M 644 inline $filename\n" $n
+		echo "data <<EOF"
+		printf "$contents\n" $n
+		echo "EOF"
+		echo
+		n=$((n + 1))
+		total=$((total - 1))
+	done >"$tmpfile"
+
+	git -C "$indir" \
+	    -c fastimport.unpacklimit=0 \
+	    fast-import <"$tmpfile" || return 1
+
+	# This will be left in place on failure, which may aid debugging.
+	rm -f "$tmpfile"
+
+	# If we updated HEAD, then be nice and update the index and working
+	# tree, too.
+	if test "$ref" = "HEAD"
+	then
+		git -C "$indir" checkout -f HEAD || return 1
+	fi
+
+}
+
 # This function helps systems where core.filemode=false is set.
 # Use it instead of plain 'chmod +x' to set or unset the executable bit
 # of a file in the working directory and add it to the index.
-- 
2.22.0.776.g16867c022c

Re: [PATCH v2 1/6] test-lib: introduce test_commit_bulk

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   - replace eval formatting magic with "%s" printf formatters (safer and
>     gets rid of quoting issues in the callers).

This one actually made me think twice about safety, as we'd be using
end-user supplied formatting string without any inspection.  I think
it is fine as it is merely a test helper.  

If somebody is later making it into a test-tool function, I expect
that our interpolation engine, not the bare sprintf(), would be used
there, and it would hopefully also be safe?

Thanks.

Re: [PATCH v2 1/6] test-lib: introduce test_commit_bulk

[Jeff King]

On Mon, Jul 01, 2019 at 03:28:45PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >   - replace eval formatting magic with "%s" printf formatters (safer and
> >     gets rid of quoting issues in the callers).
> 
> This one actually made me think twice about safety, as we'd be using
> end-user supplied formatting string without any inspection.  I think
> it is fine as it is merely a test helper.

Yeah, and most shells do something sensible with nonsense formats.
E.g., "%s %s" will yield an empty string for the second one in both dash
and bash (and that's what POSIX says, though I'd be happy with any
implementation that avoids segfaulting).

> If somebody is later making it into a test-tool function, I expect
> that our interpolation engine, not the bare sprintf(), would be used
> there, and it would hopefully also be safe?

Yes, that was exactly my plan. It would also let you mention the number
more than once in the format, though I doubt any callers would care
about that feature.

I also think more potential callers could be converted if the refname
was formatted, too (e.g., some of them seem to write to branch-1,
branch-2, etc). I drew the line there, but anybody is welcome to explore
it further.

-Peff

Re: Git Test Coverage Report (Thurs. June 27)

[Jeff King]

On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:

> > t/helper/test-example-decorate.c
> > 0ebbcf70 29) one = lookup_unknown_object(&one_oid);
> > 0ebbcf70 30) two = lookup_unknown_object(&two_oid);
> > 0ebbcf70 59) three = lookup_unknown_object(&three_oid);
> 
> Peff: again interesting that these lines you refactored were not covered, especially
> because they are part of a test helper. Perhaps the tests they were intended for are
> now defunct?

They should be run by t9004 (and if I replace them with a `die`, they
clearly are). Are you sure your coverage script is not mistaken?

-Peff

Re: Git Test Coverage Report (Thurs. June 27)

[Derrick Stolee]

On 6/28/2019 2:45 AM, Jeff King wrote:
> On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:
> 
>>> t/helper/test-example-decorate.c
>>> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);
>>> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);
>>> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);
>>
>> Peff: again interesting that these lines you refactored were not covered, especially
>> because they are part of a test helper. Perhaps the tests they were intended for are
>> now defunct?
> 
> They should be run by t9004 (and if I replace them with a `die`, they
> clearly are). Are you sure your coverage script is not mistaken?

It looks like I'm missing the 9000+ tests. The following line was in the script
I adapted from another CI job:

	rm -f t/t9*.sh

This was probably because the job I adapted from needed to run quickly, but for
this coverage report we should do the hard work of running whatever t9*.sh tests
we can.

Sorry for the noise here, and thanks for checking!

-Stolee

Re: Git Test Coverage Report (Thurs. June 27)

[Jeff King]

On Fri, Jun 28, 2019 at 08:23:49AM -0400, Derrick Stolee wrote:

> On 6/28/2019 2:45 AM, Jeff King wrote:
> > On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:
> > 
> >>> t/helper/test-example-decorate.c
> >>> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);
> >>> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);
> >>> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);
> >>
> >> Peff: again interesting that these lines you refactored were not covered, especially
> >> because they are part of a test helper. Perhaps the tests they were intended for are
> >> now defunct?
> > 
> > They should be run by t9004 (and if I replace them with a `die`, they
> > clearly are). Are you sure your coverage script is not mistaken?
> 
> It looks like I'm missing the 9000+ tests. The following line was in the script
> I adapted from another CI job:
> 
> 	rm -f t/t9*.sh
> 
> This was probably because the job I adapted from needed to run quickly, but for
> this coverage report we should do the hard work of running whatever t9*.sh tests
> we can.

I suspect most of those _are_ low-value. The git-p4 tests, for instance,
are mostly exercising the p4 script and not our C code, and the same
with git-svn. However I wouldn't be surprised if there are a few dusty
corners they manage to hit that aren't covered elsewhere.

Still, if it's not too painful to add them in time-wise, it probably
makes sense for the coverage tests to be as exhaustive as possible.

-Peff

Re: Git Test Coverage Report (Thurs. June 27)

[Derrick Stolee]

On 6/28/2019 7:59 PM, Jeff King wrote:
> On Fri, Jun 28, 2019 at 08:23:49AM -0400, Derrick Stolee wrote:
> 
>> On 6/28/2019 2:45 AM, Jeff King wrote:
>>> On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:
>>>
>>>>> t/helper/test-example-decorate.c
>>>>> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);
>>>>> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);
>>>>> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);
>>>>
>>>> Peff: again interesting that these lines you refactored were not covered, especially
>>>> because they are part of a test helper. Perhaps the tests they were intended for are
>>>> now defunct?
>>>
>>> They should be run by t9004 (and if I replace them with a `die`, they
>>> clearly are). Are you sure your coverage script is not mistaken?
>>
>> It looks like I'm missing the 9000+ tests. The following line was in the script
>> I adapted from another CI job:
>>
>> 	rm -f t/t9*.sh
>>
>> This was probably because the job I adapted from needed to run quickly, but for
>> this coverage report we should do the hard work of running whatever t9*.sh tests
>> we can.
> 
> I suspect most of those _are_ low-value. The git-p4 tests, for instance,
> are mostly exercising the p4 script and not our C code, and the same
> with git-svn. However I wouldn't be surprised if there are a few dusty
> corners they manage to hit that aren't covered elsewhere.
> 
> Still, if it's not too painful to add them in time-wise, it probably
> makes sense for the coverage tests to be as exhaustive as possible.

Unfortunately, even running the t9*.sh tests once (among the two runs:
first with default options and then with several GIT_TEST_* options)
causes the build to go beyond the three hour limit, and the builds time
out.

I'll just need to keep this in mind and do some more diligence myself
to check if things are covered in the 9000 tests before bugging people
about coverage.

Thanks,
-Stolee
 

Re: Git Test Coverage Report (Thurs. June 27)

[Jeff King]

On Fri, Jun 28, 2019 at 09:36:14PM -0400, Derrick Stolee wrote:

> > Still, if it's not too painful to add them in time-wise, it probably
> > makes sense for the coverage tests to be as exhaustive as possible.
> 
> Unfortunately, even running the t9*.sh tests once (among the two runs:
> first with default options and then with several GIT_TEST_* options)
> causes the build to go beyond the three hour limit, and the builds time
> out.

Is that because you're running the tests sequentially, due to the
corruption of the gcov files?

I think something like this would work to get per-script profiles:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4b346467df..81841191d2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -369,6 +369,9 @@ TZ=UTC
 export LANG LC_ALL PAGER TZ
 EDITOR=:
 
+GCOV_PREFIX=$TEST_RESULTS_BASE.gcov
+export GCOV_PREFIX
+
 # GIT_TEST_GETTEXT_POISON should not influence git commands executed
 # during initialization of test-lib and the test repo. Back it up,
 # unset and then restore after initialization is finished.


And then you can reassemble that with something like this (gcov-tool
comes with gcc):

  for i in t/test-results/t*.gcov; do
    echo >&2 "Merging $i..."
    gcov-tool merge -o . . "$i/$PWD"
  done

The merge is pretty slow, though (and necessarily serial). I wonder if
you'd do better to dump gcov output from each directory and then collate
it as text. I've heard lcov also has better support for handling
multiple runs like this.

-Peff

Re: Git Test Coverage Report (Thurs. June 27)

[Duy Nguyen]

On Fri, Jun 28, 2019 at 12:35 AM Derrick Stolee <stolee@gmail.com> wrote:
> > dir.c
> > 3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,
> > 3b2385cf 2845) jw_object_bool(jw, "valid", ucd->valid);
> > 3b2385cf 2846) jw_object_bool(jw, "check-only", ucd->check_only);
> > 3b2385cf 2847) jw_object_stat_data(jw, "stat", &ucd->stat_data);
> > 3b2385cf 2848) jw_object_string(jw, "exclude-oid", oid_to_hex(&ucd->exclude_oid));
> > 3b2385cf 2849) jw_object_inline_begin_array(jw, "untracked");
> > 3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)
> > 3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);
> > 3b2385cf 2852) jw_end(jw);
> > 3b2385cf 2854) jw_object_inline_begin_object(jw, "dirs");
> > 3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {
> > 3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);
> > 3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);
> > 3b2385cf 2858) jw_end(jw);
> > 3b2385cf 2860) jw_end(jw);
> > 3b2385cf 2861) }
> > 3b2385cf 2958) jw_object_inline_begin_object(jw, "root");
> > 3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);
> > 3b2385cf 2960) jw_end(jw);
>
> Duy: I know you were working on some tests for these options. This is specifically
> in the "untracked cache" mode, so enabling the cache with at least one entry and
> running --debug-json should be sufficient.

It's a bit more complicated than that, but I see your point. I
initially looked at the output and saw "something" and moved on. I
should have examined the json output more carefully.

> > read-cache.c
> > 8eeabe15 1752) ret = error(_("index uses %.4s extension, which we do not understand"),
> > ee70c128 1754) if (advice_unknown_index_extension) {
> > ee70c128 1755) warning(_("ignoring optional %.4s index extension"), ext);
> > ee70c128 1756) advise(_("This is likely due to the file having been written by a newer\n"
> > 272b3f2a 2026) jw_object_true(jw, "assume_unchanged");
> > 272b3f2a 2030) jw_object_true(jw, "skip_worktree");
> > 272b3f2a 2032) jw_object_intmax(jw, "stage", ce_stage(ce));
> > f0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);
> > f0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(
> > f0f544da 3673) return do_read_ieot_extension(istate, index, extsize);
>
> Duy: more JSON output cases that could be interesting to cover.

This is because I changed the function signature, I think. Both IEOT
and EOIE extensions, if I'm not mistaken, are never tested in the test
suite. You need to set GIT_TEST_INDEX_THREADS, then the last three
lines should be covered.
-- 
Duy

Re: Git Test Coverage Report (Thurs. June 27)

[Derrick Stolee]

On 6/28/2019 5:47 AM, Duy Nguyen wrote:
> On Fri, Jun 28, 2019 at 12:35 AM Derrick Stolee <stolee@gmail.com> wrote:
>>> dir.c
>>> 3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,
>>> 3b2385cf 2845) jw_object_bool(jw, "valid", ucd->valid);
>>> 3b2385cf 2846) jw_object_bool(jw, "check-only", ucd->check_only);
>>> 3b2385cf 2847) jw_object_stat_data(jw, "stat", &ucd->stat_data);
>>> 3b2385cf 2848) jw_object_string(jw, "exclude-oid", oid_to_hex(&ucd->exclude_oid));
>>> 3b2385cf 2849) jw_object_inline_begin_array(jw, "untracked");
>>> 3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)
>>> 3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);
>>> 3b2385cf 2852) jw_end(jw);
>>> 3b2385cf 2854) jw_object_inline_begin_object(jw, "dirs");
>>> 3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {
>>> 3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);
>>> 3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);
>>> 3b2385cf 2858) jw_end(jw);
>>> 3b2385cf 2860) jw_end(jw);
>>> 3b2385cf 2861) }
>>> 3b2385cf 2958) jw_object_inline_begin_object(jw, "root");
>>> 3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);
>>> 3b2385cf 2960) jw_end(jw);
>>
>> Duy: I know you were working on some tests for these options. This is specifically
>> in the "untracked cache" mode, so enabling the cache with at least one entry and
>> running --debug-json should be sufficient.
> 
> It's a bit more complicated than that, but I see your point.

It usually is. I don't mean to underestimate the effort here.

> I initially looked at the output and saw "something" and moved on. I
> should have examined the json output more carefully.

Thanks for taking a second look!

>>> read-cache.c
>>> 8eeabe15 1752) ret = error(_("index uses %.4s extension, which we do not understand"),
>>> ee70c128 1754) if (advice_unknown_index_extension) {
>>> ee70c128 1755) warning(_("ignoring optional %.4s index extension"), ext);
>>> ee70c128 1756) advise(_("This is likely due to the file having been written by a newer\n"
>>> 272b3f2a 2026) jw_object_true(jw, "assume_unchanged");
>>> 272b3f2a 2030) jw_object_true(jw, "skip_worktree");
>>> 272b3f2a 2032) jw_object_intmax(jw, "stage", ce_stage(ce));
>>> f0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);
>>> f0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(
>>> f0f544da 3673) return do_read_ieot_extension(istate, index, extsize);
>>
>> Duy: more JSON output cases that could be interesting to cover.
> 
> This is because I changed the function signature, I think. Both IEOT
> and EOIE extensions, if I'm not mistaken, are never tested in the test
> suite. You need to set GIT_TEST_INDEX_THREADS, then the last three
> lines should be covered.

Thanks! Unfortunately, the threading is removed at compile-time in
order to prevent race conditions with the gcov output. This means
the report will never report the threading code as covered. :(

Does that same reasoning apply to the assume_unchanged, skip_worktree,
and "stage" lines?

-Stolee

Re: Git Test Coverage Report (Thurs. June 27)

[Christian Couder]

On Thu, Jun 27, 2019 at 7:35 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> Here are some interesting sections I found when examining the test coverage
> report. I am only highlighting these sections because they seem to include
> non-trivial logic. In some cases, maybe the code isn't needed.
>
> On 6/27/2019 1:05 PM, Derrick Stolee wrote:
> > promisor-remote.c
> > db27dca5 25) die(_("Remote with no URL"));
> > 48de3158 61) warning(_("promisor remote name cannot begin with '/': %s"),
> > 48de3158 63) return NULL;
> > faf2abf4 93) previous->next = r->next;
> > 4ca9474e 108) return git_config_string(&core_partial_clone_filter_default,
> > fa3d1b63 139) return 0;
> > 9e27beaa 202) static int remove_fetched_oids(struct repository *repo,
> > 9e27beaa 206) int i, remaining_nr = 0;
> > 9e27beaa 207) int *remaining = xcalloc(oid_nr, sizeof(*remaining));
> > 9e27beaa 208) struct object_id *old_oids = *oids;
> > 9e27beaa 211) for (i = 0; i < oid_nr; i++)
> > 9e27beaa 212) if (oid_object_info_extended(repo, &old_oids[i], NULL,
> > 9e27beaa 214) remaining[i] = 1;
> > 9e27beaa 215) remaining_nr++;
> > 9e27beaa 218) if (remaining_nr) {
> > 9e27beaa 219) int j = 0;
> > 9e27beaa 220) new_oids = xcalloc(remaining_nr, sizeof(*new_oids));
> > 9e27beaa 221) for (i = 0; i < oid_nr; i++)
> > 9e27beaa 222) if (remaining[i])
> > 9e27beaa 223) oidcpy(&new_oids[j++], &old_oids[i]);
> > 9e27beaa 224) *oids = new_oids;
> > 9e27beaa 225) if (to_free)
> > 9e27beaa 226) free(old_oids);
> > 9e27beaa 229) free(remaining);
> > 9e27beaa 231) return remaining_nr;
> > 9e27beaa 248) if (remaining_nr == 1)
> > 9e27beaa 249) continue;
> > 9e27beaa 250) remaining_nr = remove_fetched_oids(repo, &remaining_oids,
> > 9e27beaa 252) if (remaining_nr) {
> > 9e27beaa 253) to_free = 1;
> > 9e27beaa 254) continue;
> > 9e27beaa 262) free(remaining_oids);
>
> Christian: this section continues to be untested, but I think you were
> working on creating tests for this.

Yeah, I am planning to work on this soon.

> > t/helper/test-oidmap.c
> > 11510dec 52) if (get_oid(p1, &oid)) {
> > 11510dec 53) printf("Unknown oid: %s\n", p1);
> > 11510dec 54) continue;
> > 11510dec 58) FLEX_ALLOC_STR(entry, name, p2);
> > 11510dec 59) oidcpy(&entry->entry.oid, &oid);
> > 11510dec 62) oidmap_put(&map, entry);
>
> Christian: this block looks like the test-oidmap helper never uses the "add"
> subcommand. Is that correct?

Yeah, I initially copied it from hashmap, but then I realized that it
was nearly identical as the "put" subcommand, so not worth testing
separately. I should have removed it and will do it soon.

> > 11510dec 97) if (get_oid(p1, &oid)) {
> > 11510dec 98) printf("Unknown oid: %s\n", p1);
> > 11510dec 99) continue;
> > 11510dec 103) entry = oidmap_remove(&map, &oid);
> > 11510dec 106) puts(entry ? entry->name : "NULL");
> > 11510dec 107) free(entry);
>
> Similarly, this block means we are not using the "remove" subcommand.

Yeah, it looks like I forgot to implement a test for that subcommand.
Will add it soon.

Thanks,
Christian.