{"thread":{"id":"51396","subject":"Git Test Coverage Report (Thurs. June 27)","startedAt":"2019-06-27T17:05:37Z","lastAt":"2019-07-02T05:22:47Z","messageCount":43,"participants":["Derrick Stolee","Jeff King","Duy Nguyen","Johannes Schindelin","Christian Couder","Junio C Hamano","Ævar Arnfjörð Bjarmason","Eric Sunshine","SZEDER Gábor","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"378170","messageId":"49d98293-9f0b-44e9-cb07-d6b7ac791eb6@gmail.com","threadId":"51396","inReplyTo":null,"subject":"Git Test Coverage Report (Thurs. June 27)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-27T17:05:33Z","receivedAt":"2019-06-27T17:05:37Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Here is today's test coverage report.\n\nThanks,\n-Stolee\n\n[1] https://derrickstolee.github.io/git-test-coverage/reports/2019-06-27.htm\n[2] https://derrickstolee.github.io/git-test-coverage/reports/2019-06-27.txt\n\n---\n\npu\tf8df19e612605e564ac8006e61428c8b95391624\njch\te1093b448f0c8fd5069bcff6cd20af2e1ab1bcef\nnext\tee2e6308ae95ce7ba2fabedcc43f5f741e71c152\nmaster\t8dca754b1e874719a732bc9ab7b0e14b21b1bc10\nmaster@{1}\torigin/maint\n\n\nUncovered code in 'pu' not in 'jch'\n--------------------------------------------------------\n\narchive.c\n47f956bd 421) err = get_tree_entry(ar_args->repo,\n47f956bd 422)      &tree->object.oid,\n\nbuiltin/cat-file.c\nb14ed5ad 529) warning(\"This repository uses promisor remotes. Some objects may not be loaded.\");\n\nbuiltin/clone.c\nfbb4a33c 407) die_errno(_(\"failed to create directory '%s'\"), pathname);\nfbb4a33c 409) die_errno(_(\"failed to stat '%s'\"), pathname);\nd95432d7 428) die_errno(_(\"failed to start iterator over '%s'\"), src->buf);\nd95432d7 466) strbuf_setlen(src, src_len);\nd95432d7 467) die(_(\"failed to iterate over '%s'\"), src->buf);\n\nbuiltin/env--helper.c\nb4f207f3 23) die(_(\"unrecognized --type argument, %s\"), arg);\nb4f207f3 67) default_int = 0;\nb4f207f3 82) default_ulong = 0;\nb4f207f3 90) BUG(\"unknown <type> value\");\n\nbuiltin/fetch-pack.c\n\nbuiltin/pack-objects.c\n820a5361 859) BUG(\"configured exclusion wasn't configured\");\n820a5361 2794) die(_(\"value of uploadpack.blobpackfileuri must be \"\n820a5361 2797) die(_(\"object already configured in another \"\n\nbuiltin/remote.c\nf39a9c65 1551) die(_(\"--save-to-push cannot be used with other options\"));\nf39a9c65 1575) die(_(\"--save-to-push can only be used when only one url is defined\"));\n\nbuiltin/rev-list.c\n9b93d269 476) die(\n\nconfig.c\n2e43cd4c 998)     value, name, cf->name, _(error_type));\n2e43cd4c 1007)     value, name, cf->name, _(error_type));\n2e43cd4c 1010)     value, name, cf->name, _(error_type));\n2e43cd4c 1013)     value, name, cf->name, _(error_type));\n\ndir-iterator.c\n50e85c40 92) warning_errno(\"error closing directory '%s'\",\n655af733 126) warning_errno(\"failed to stat '%s'\", iter->base.path.buf);\n655af733 159) goto error_out;\n50e85c40 174) warning_errno(\"error reading directory '%s'\",\n655af733 176) if (iter->flags & DIR_ITERATOR_PEDANTIC)\n655af733 177) goto error_out;\n655af733 188) if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n655af733 189) goto error_out;\nc40f077a 216) int saved_errno = errno;\nc40f077a 218) errno = saved_errno;\nc40f077a 219) warning_errno(\"error closing directory '%s'\",\n\nfast-import.c\n35d7cdbe 2565) char *buf = read_object_with_reference(the_repository,\n35d7cdbe 2566)        &n->oid,\n\nfetch-pack.c\n820a5361 1397) die(\"expected '<hash> <uri>', got: %s\\n\", reader->line);\n820a5361 1402) die(\"expected DELIM\");\n820a5361 1529) die(\"fetch-pack: unable to spawn http-fetch\");\n820a5361 1533) die(\"fetch-pack: expected keep then TAB at start of http-fetch output\");\n820a5361 1538) die(\"fetch-pack: expected hash then LF at end of http-fetch output\");\n820a5361 1545) die(\"fetch-pack: unable to finish http-fetch\");\n820a5361 1549) die(\"fetch-pack: pack downloaded from %s does not match expected hash %.*s\",\n820a5361 1550)     uri, (int) the_hash_algo->hexsz,\n820a5361 1551)     packfile_uris.items[i].string);\n\nhttp-fetch.c\n\nhttp.c\n3d908bb8 2304) target ? hash_to_hex(target->hash) : base_url,\n\nlist-objects-filter-options.c\nd3d10e56 44) BUG(\"filter_options already populated\");\nd3d10e56 236) die(_(\"multiple filter-specs cannot be combined\"));\n9b93d269 286) BUG(\"no filter_spec available for this filter\");\n1e43301f 321) return;\n\nlist-objects-filter.c\n1e43301f 583) BUG(\"expected oidset to be cleared already\");\n\nlist-objects.c\naa36553a 210) ctx->show_object(obj, base->buf, ctx->show_data);\n\nmatch-trees.c\n3fe87a7f 294) if (get_tree_entry(r, hash2, del_prefix, shifted, &mode))\n\npromisor-remote.c\ndb27dca5 25) die(_(\"Remote with no URL\"));\n48de3158 61) warning(_(\"promisor remote name cannot begin with '/': %s\"),\n48de3158 63) return NULL;\nfaf2abf4 93) previous->next = r->next;\n4ca9474e 108) return git_config_string(&core_partial_clone_filter_default,\nfa3d1b63 139) return 0;\n9e27beaa 202) static int remove_fetched_oids(struct repository *repo,\n9e27beaa 206) int i, remaining_nr = 0;\n9e27beaa 207) int *remaining = xcalloc(oid_nr, sizeof(*remaining));\n9e27beaa 208) struct object_id *old_oids = *oids;\n9e27beaa 211) for (i = 0; i < oid_nr; i++)\n9e27beaa 212) if (oid_object_info_extended(repo, &old_oids[i], NULL,\n9e27beaa 214) remaining[i] = 1;\n9e27beaa 215) remaining_nr++;\n9e27beaa 218) if (remaining_nr) {\n9e27beaa 219) int j = 0;\n9e27beaa 220) new_oids = xcalloc(remaining_nr, sizeof(*new_oids));\n9e27beaa 221) for (i = 0; i < oid_nr; i++)\n9e27beaa 222) if (remaining[i])\n9e27beaa 223) oidcpy(&new_oids[j++], &old_oids[i]);\n9e27beaa 224) *oids = new_oids;\n9e27beaa 225) if (to_free)\n9e27beaa 226) free(old_oids);\n9e27beaa 229) free(remaining);\n9e27beaa 231) return remaining_nr;\n9e27beaa 248) if (remaining_nr == 1)\n9e27beaa 249) continue;\n9e27beaa 250) remaining_nr = remove_fetched_oids(repo, &remaining_oids,\n9e27beaa 252) if (remaining_nr) {\n9e27beaa 253) to_free = 1;\n9e27beaa 254) continue;\n9e27beaa 262) free(remaining_oids);\n\nprotocol.c\n\nremote-curl.c\n\nrepo-settings.c\n0a01e977 13) int rate = git_config_int(key, value);\n0a01e977 14) if (rate >= 3) {\n0a01e977 15) UPDATE_DEFAULT(rs->core_commit_graph, 1);\n0a01e977 16) UPDATE_DEFAULT(rs->gc_write_commit_graph, 1);\nc5c84f32 17) UPDATE_DEFAULT(rs->index_version, 4);\n3172404b 19) if (rate >= 5) {\n3172404b 20) UPDATE_DEFAULT(rs->pack_use_sparse, 1);\n0a01e977 22) return 0;\n\nstrbuf.c\nfb819691 818) strbuf_addf(buf, \"%u.%2.2u \",\nfb819691 822) strbuf_addstr(buf, _(\"GiB\"));\n\nt/helper/test-dir-iterator.c\n655af733 24) die(\"invalid option '%s'\", *argv);\n655af733 28) die(\"dir-iterator needs exactly one non-option argument\");\n9bd70db7 46) printf(\"[?] \");\n\nt/helper/test-match-trees.c\n3fe87a7f 23) shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1);\n\nupload-pack.c\na8d662e3 130) return readsz;\n820a5361 149) BUG(\"packfile_uris requires sideband-all\");\n9b93d269 221) sq_quote_buf(&buf, spec);\na8d662e3 354) send_client_data(1, output_state.buffer, output_state.used);\n820a5361 1387) string_list_clear(&data->uri_protocols, 0);\n\nwrapper.c\n\nCommits introducting uncovered code:\nÆvar Arnfjörð Bjarmason\tb4f207f3 env--helper: new undocumented builtin wrapping git_env_*()\nÆvar Arnfjörð Bjarmason\t2e43cd4c config.c: refactor die_bad_number() to not call gettext() early\nChristian Couder\tfa3d1b63 promisor-remote: parse remote.*.partialclonefilter\nChristian Couder\tfaf2abf4 promisor-remote: use repository_format_partial_clone\nChristian Couder\t4ca9474e Move core_partial_clone_filter_default to promisor-remote.c\nChristian Couder\tdb27dca5 Remove fetch-object.{c,h} in favor of promisor-remote.{c,h}\nChristian Couder\t48de3158 Add initial support for many promisor remotes\nChristian Couder\t9e27beaa promisor-remote: implement promisor_remote_get_direct()\nChristian Couder\tb14ed5ad Use promisor_remote_get_direct() and has_promisor_remote()\nDaniel Ferreira\t9bd70db7 dir-iterator: add tests for dir-iterator API\nDenton Liu\tf39a9c65 remote: add --save-to-push option to git remote set-url\nDerrick Stolee\t0a01e977 repo-settings: create core.featureAdoptionRate setting\nDerrick Stolee\tc5c84f32 repo-settings: use index.version=4 by default\nDerrick Stolee\t3172404b repo-settings: pack.useSparse=true\nDimitriy Ryazantcev\tfb819691 l10n: localizable upload progress messages\nJonathan Tan\t820a5361 upload-pack: send part of packfile response as uri\nJonathan Tan\ta8d662e3 upload-pack: refactor reading of pack-objects out\nJunio C Hamano\t3d908bb8 Merge branch 'jt/fetch-cdn-offload' into pu\nMatheus Tavares\t655af733 dir-iterator: add flags parameter to dir_iterator_begin\nMatheus Tavares\t50e85c40 dir-iterator: refactor state machine model\nMatheus Tavares\td95432d7 clone: use dir-iterator to avoid explicit dir traversal\nMatheus Tavares\tfbb4a33c clone: extract function from copy_or_link_directory\nMatheus Tavares\tc40f077a dir-iterator: use warning_errno when possible\nMatthew DeVore\taa36553a list-objects-filter: make API easier to use\nMatthew DeVore\t1e43301f list-objects-filter: implement composite filters\nMatthew DeVore\td3d10e56 list-objects-filter-options: move error check up\nMatthew DeVore\t9b93d269 list-objects-filter-options: make filter_spec a string_list\nNguyễn Thái Ngọc Duy\t35d7cdbe sha1-file.c: remove the_repo from read_object_with_reference()\nNguyễn Thái Ngọc Duy\t3fe87a7f match-trees.c: remove the_repo from shift_tree*()\nNguyễn Thái Ngọc Duy\t47f956bd tree-walk.c: remove the_repo from get_tree_entry()\n\n\nUncovered code in 'jch' not in 'next'\n--------------------------------------------------------\n\nblame.c\n1fc73384 990) return;\na07a9776 1599) continue;\nae3f36de 2417) continue;\n\nbuiltin/blame.c\n\nbuiltin/checkout.c\nd16dc428 1345) warning(_(\"you are switching branch while bisecting\"));\n3ec37ad1 1370) die(_(\"'%s' cannot be used with '%s'\"), \"--discard-changes\", \"--merge\");\nc9c935f6 1508) BUG(\"make up your mind, you need to take _something_\");\n183fb44f 1540) opts->checkout_index = 0;\n183fb44f 1550) BUG(\"these flags should be non-negative by now\");\nc9c935f6 1611) die(_(\"could not resolve %s\"), opts->from_treeish);\n\nbuiltin/commit-graph.c\nc2bc6e6a 203) return 1;\n\nbuiltin/fetch.c\ncdbd70c4 88) fetch_show_forced_updates = git_config_bool(k, v);\ncdbd70c4 89) return 0;\n377444b4 1011) warning(_(\"It took %.2f seconds to check forced updates. You can use '--no-show-forced-updates'\\n\"),\n377444b4 1013) warning(_(\"or run 'git config fetch.showForcedUpdates false' to avoid this check.\\n\"));\n\nbuiltin/ls-files.c\n272b3f2a 672) die(_(\"--debug-json cannot be used with other file selection options\"));\n272b3f2a 674) die(_(\"--debug-json cannot be used with %s\"), \"--resolve-undo\");\n272b3f2a 676) die(_(\"--debug-json cannot be used with %s\"), \"--with-tree\");\n272b3f2a 678) die(_(\"--debug-json cannot be used with %s\"), \"--debug\");\n272b3f2a 701) die(\"index file corrupt\");\n\nbuiltin/multi-pack-index.c\n\nbuiltin/pull.c\n3883c551 556) argv_array_push(&args, \"--show-forced-updates\");\n3883c551 558) argv_array_push(&args, \"--no-show-forced-updates\");\n\nbuiltin/rebase.c\nd559f502 759) ret = error(_(\"could not remove '%s'\"),\n526c03b5 1272) goto done;\n10572de1 1288) goto done;\nd559f502 1675) error(_(\"could not remove '%s'\"),\n\ncache-tree.c\nfd335a21 605) ret = NULL; /* not the whole tree */\n\ncommit-graph.c\n118bd570 277) chunk_repeated = 1;\n118bd570 347) warning(_(\"commit-graph has no base graphs chunk\"));\n118bd570 348) return 0;\n5c84b339 401) break;\nd4f4d60f 550) BUG(\"NULL commit-graph\");\nd4f4d60f 553) die(_(\"invalid commit position. commit-graph is likely corrupt\"));\nd4f4d60f 613) die(_(\"invalid commit position. commit-graph is likely corrupt\"));\n6c622f9f 1057) continue;\n6c622f9f 1331) error(_(\"failed to write correct number of base graph ids\"));\n6c622f9f 1332) return -1;\n6c622f9f 1376) error(_(\"unable to create '%s'\"), ctx->graph_name);\n6c622f9f 1377) return -1;\n6c622f9f 1451) return -1;\n6c622f9f 1477) error(_(\"unable to open commit-graph chain file\"));\n6c622f9f 1478) return -1;\n135a7123 1489) error(_(\"failed to rename base commit-graph file\"));\n6c622f9f 1509) error(_(\"failed to rename temporary commit-graph file\"));\n6c622f9f 1510) return -1;\nc523035c 1539) break;\nc523035c 1554) ctx->num_commit_graphs_after = 1;\nc523035c 1555) ctx->new_base_graph = NULL;\n1771be90 1631) die(_(\"unexpected duplicate commit id %s\"),\n1771be90 1632)     oid_to_hex(&ctx->commits.list[i]->object.oid));\nc2bc6e6a 1806) ctx->oids.alloc = split_opts->max_commits;\n\nconfig.c\n07b2c0ea 283) return 0;\n\ndelta-islands.c\nbdbdf42f 467) fprintf(stderr, _(\"Marked %d islands, done.\\n\"), island_counter);\n\ndir.c\n3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,\n3b2385cf 2845) jw_object_bool(jw, \"valid\", ucd->valid);\n3b2385cf 2846) jw_object_bool(jw, \"check-only\", ucd->check_only);\n3b2385cf 2847) jw_object_stat_data(jw, \"stat\", &ucd->stat_data);\n3b2385cf 2848) jw_object_string(jw, \"exclude-oid\", oid_to_hex(&ucd->exclude_oid));\n3b2385cf 2849) jw_object_inline_begin_array(jw, \"untracked\");\n3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)\n3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);\n3b2385cf 2852) jw_end(jw);\n3b2385cf 2854) jw_object_inline_begin_object(jw, \"dirs\");\n3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {\n3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);\n3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);\n3b2385cf 2858) jw_end(jw);\n3b2385cf 2860) jw_end(jw);\n3b2385cf 2861) }\n3b2385cf 2958) jw_object_inline_begin_object(jw, \"root\");\n3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);\n3b2385cf 2960) jw_end(jw);\n\nfetch-pack.c\n5a88583b 914) else if (args->depth > 0 || is_repository_shallow(the_repository))\n0778b293 928) print_verbose(args, _(\"Server supports %s\"), \"multi_ack\");\n0778b293 936) print_verbose(args, _(\"Server supports %s\"), \"side-band\");\n5a88583b 974) } else if (args->deepen_since)\n5a88583b 979) } else if (args->deepen_not)\n5a88583b 983) else if (args->deepen_relative)\n\nkwset.c\n08e04506 45) BUG(\"Cannot allocate a negative amount: %ld\", size);\n\nmidx.c\nd01bf2e6 478) close_pack(packs->info[packs->nr].p);\nd01bf2e6 479) FREE_AND_NULL(packs->info[packs->nr].p);\n19575c7c 738) BUG(\"object %s is in an expired pack with int-id %d\",\n19575c7c 865) error(_(\"did not see pack-file %s to drop\"),\n19575c7c 867) drop_index++;\n19575c7c 868) missing_drops++;\n19575c7c 869) i--;\n19575c7c 876) result = 1;\n19575c7c 877) goto cleanup;\n19575c7c 1194) return 0;\n19575c7c 1209) continue;\nce1e4a10 1248) return 0;\nce1e4a10 1275) continue;\nce1e4a10 1295) continue;\nce1e4a10 1297) continue;\nce1e4a10 1329) return 0;\nce1e4a10 1350) error(_(\"could not start pack-objects\"));\nce1e4a10 1351) result = 1;\nce1e4a10 1352) goto cleanup;\nce1e4a10 1369) error(_(\"could not finish pack-objects\"));\nce1e4a10 1370) result = 1;\nce1e4a10 1371) goto cleanup;\n\noidmap.c\n0a66ac47 42) hashmap_entry_init(&entry, oidhash(key));\n\noidset.c\n\npack-bitmap-write.c\n05805d74 378) static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)\nd2bc62b1 385) hash_pos = kh_get_oid_map(writer.reused, *oid);\n05805d74 425) reused_bitmap = find_reused_bitmap(&chosen->object.oid);\n05805d74 432) reused_bitmap = find_reused_bitmap(&cm->object.oid);\n\npackfile.c\n8434e85d 372) strbuf_release(&buf);\n8434e85d 373) return;\n\npager.c\ncd1096b2 197) fputs(\"\\r\\033[K\", stderr);\n\nprogress.c\nfbe464c0 121) fprintf(stderr, \"  %s%s\", counters_sb->buf,\nfbe464c0 127) fprintf(stderr, \"%s:\\n  %s%s\",\n\nread-cache.c\n8eeabe15 1752) ret = error(_(\"index uses %.4s extension, which we do not understand\"),\nee70c128 1754) if (advice_unknown_index_extension) {\nee70c128 1755) warning(_(\"ignoring optional %.4s index extension\"), ext);\nee70c128 1756) advise(_(\"This is likely due to the file having been written by a newer\\n\"\n272b3f2a 2026) jw_object_true(jw, \"assume_unchanged\");\n272b3f2a 2030) jw_object_true(jw, \"skip_worktree\");\n272b3f2a 2032) jw_object_intmax(jw, \"stage\", ce_stage(ce));\nf0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);\nf0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(\nf0f544da 3673) return do_read_ieot_extension(istate, index, extsize);\n\nref-filter.c\n28438e84 1500) strbuf_addstr(&desc, _(\"no branch\"));\n\nsequencer.c\n37e9ee5c 293) ret = -1;\n37e9ee5c 311) ret = error(_(\"could not remove '%s'\"), buf.buf);\n30652115 2198)  (*bol == ' ' || *bol == '\\t'))\n30652115 2201) ret = -1;\n516dc810 2668) in_progress_error = _(\"revert is already in progress\");\n516dc810 2669) in_progress_advice =\n516dc810 2671) break;\n516dc810 2678) BUG(\"unexpected action in create_seq_dir\");\n3fa2e2d8 2777) return error(_(\"cannot resolve HEAD\"));\n3fa2e2d8 2860) if (!rollback_is_safe())\n3fa2e2d8 2861) goto give_advice;\n3fa2e2d8 2873) BUG(\"unexpected action in sequencer_skip\");\n3fa2e2d8 2877) return error(_(\"failed to skip the commit\"));\n\nsplit-index.c\n1f825794 29) goto done;\n\nt/helper/test-example-decorate.c\n0ebbcf70 29) one = lookup_unknown_object(&one_oid);\n0ebbcf70 30) two = lookup_unknown_object(&two_oid);\n0ebbcf70 59) three = lookup_unknown_object(&three_oid);\n\nt/helper/test-oidmap.c\n11510dec 52) if (get_oid(p1, &oid)) {\n11510dec 53) printf(\"Unknown oid: %s\\n\", p1);\n11510dec 54) continue;\n11510dec 58) FLEX_ALLOC_STR(entry, name, p2);\n11510dec 59) oidcpy(&entry->entry.oid, &oid);\n11510dec 62) oidmap_put(&map, entry);\n11510dec 97) if (get_oid(p1, &oid)) {\n11510dec 98) printf(\"Unknown oid: %s\\n\", p1);\n11510dec 99) continue;\n11510dec 103) entry = oidmap_remove(&map, &oid);\n11510dec 106) puts(entry ? entry->name : \"NULL\");\n11510dec 107) free(entry);\n\nupload-pack.c\n62b89d43 534) if (parse_oid_hex(namebuf, &oid, &p) || *p != '\\n')\nd0229abd 537) o = lookup_object(the_repository, &oid);\n\nwt-status.c\n0a53561a 1114) strbuf_addf(&sb, _(\"\\n\"\n\nCommits introducting uncovered code:\nBarret Rhoden\t1fc73384 blame: optionally track line fingerprints during fill_blame_origin()\nBarret Rhoden\tae3f36de blame: add the ability to ignore commits and their changes\nBarret Rhoden\ta07a9776 blame: use the fingerprint heuristic to match ignored lines\nChristian Couder\t11510dec t/helper: add test-oidmap.c\nDenton Liu\t07b2c0ea config: learn the \"onbranch:\" includeIf condition\nDenton Liu\t526c03b5 rebase: refactor can_fast_forward into goto tower\nDenton Liu\t10572de1 rebase: fast-forward --onto in more cases\nDerrick Stolee\tce1e4a10 midx: implement midx_repack()\nDerrick Stolee\t8434e85d repack: refactor pack deletion for future use\nDerrick Stolee\td01bf2e6 midx: refactor permutation logic and pack sorting\nDerrick Stolee\t1771be90 commit-graph: merge commit-graph chains\nDerrick Stolee\tc523035c commit-graph: allow cross-alternate chains\nDerrick Stolee\t135a7123 commit-graph: add --split option to builtin\nDerrick Stolee\t6c622f9f commit-graph: write commit-graph chains\nDerrick Stolee\td4f4d60f commit-graph: prepare for commit-graph chains\nDerrick Stolee\t19575c7c multi-pack-index: implement 'expire' subcommand\nDerrick Stolee\t118bd570 commit-graph: add base graphs chunk\nDerrick Stolee\t3883c551 pull: add --[no-]show-forced-updates passthrough\nDerrick Stolee\t377444b4 fetch: warn about forced updates in branch listing\nDerrick Stolee\t5c84b339 commit-graph: load commit-graph chains\nDerrick Stolee\tcdbd70c4 fetch: add --[no-]show-forced-updates argument\nDerrick Stolee\tc2bc6e6a commit-graph: create options for split files\nJeff Hostetler\t0a53561a status: warn when a/b calculation takes too long\nJeff King\td0229abd object: convert lookup_object() to use object_id\nJeff King\t62b89d43 upload-pack: rename a \"sha1\" variable to \"oid\"\nJeff King\tbdbdf42f delta-islands: respect progress flag\nJeff King\t0ebbcf70 object: convert lookup_unknown_object() to use object_id\nJeff King\td2bc62b1 pack-bitmap: convert khash_sha1 maps into kh_oid_map\nJeff King\t05805d74 pack-bitmap-write: convert some helpers to use object_id\nJohannes Schindelin\t08e04506 kwset: allow building with GCC 8\nJonathan Nieder\tee70c128 index: offer advice for unknown index extensions\nJunio C Hamano\t0a66ac47 Merge branch 'cc/test-oidmap' into jch\nMatthew DeVore\t28438e84 ref-filter: sort detached HEAD lines firstly\nNguyễn Thái Ngọc Duy\t183fb44f restore: add --worktree and --staged\nNguyễn Thái Ngọc Duy\tfd335a21 cache-tree.c: dump \"TREE\" extension as json\nNguyễn Thái Ngọc Duy\t272b3f2a ls-files: add --json to dump the index\nNguyễn Thái Ngọc Duy\t3ec37ad1 switch: add --discard-changes\nNguyễn Thái Ngọc Duy\t3b2385cf dir.c: dump \"UNTR\" extension as json\nNguyễn Thái Ngọc Duy\t1f825794 split-index.c: dump \"link\" extension as json\nNguyễn Thái Ngọc Duy\tf0f544da read-cache.c: dump \"IEOT\" extension as json\nNguyễn Thái Ngọc Duy\t5a88583b fetch-pack: print all relevant supported capabilities with -v -v\nNguyễn Thái Ngọc Duy\t8eeabe15 read-cache.c: dump common extension info in json\nNguyễn Thái Ngọc Duy\tc9c935f6 restore: take tree-ish from --source option instead\nNguyễn Thái Ngọc Duy\t0778b293 fetch-pack: move capability names out of i18n strings\nNguyễn Thái Ngọc Duy\td16dc428 switch: allow to switch in the middle of bisect\nPhillip Wood\t30652115 status: do not report errors in sequencer/todo\nPhillip Wood\td559f502 rebase --abort/--quit: cleanup refs/rewritten\nPhillip Wood\t37e9ee5c sequencer: return errors from sequencer_remove_state()\nRohit Ashiwal\t516dc810 sequencer: add advice for revert\nRohit Ashiwal\t3fa2e2d8 cherry-pick/revert: add --skip option\nSZEDER Gábor\tfbe464c0 progress: use term_clear_line()\nSZEDER Gábor\tcd1096b2 pager: add a helper function to clear the last line in the terminal\n\n\nUncovered code in 'next' not in 'master'\n--------------------------------------------------------\n\nbuiltin/branch.c\n1fde99cf 841) die(_(\"The -a, and -r, options to 'git branch' do not take a branch name.\\n\"\n\nbuiltin/commit.c\n\nbuiltin/gc.c\ne103f727 691) return 1;\n\ncommit-graph.c\nef5b83f2 906) error(_(\"error opening index for %s\"), packname.buf);\nef5b83f2 907) return -1;\n4c9efe85 946) continue;\nb2c83060 969) display_progress(ctx->progress, ctx->approx_nr_objects);\n238def57 1039) error(_(\"unable to create leading directories of %s\"),\n238def57 1041) return -1;\ne103f727 1158) error(_(\"the commit graph format cannot write %d commits\"), count_distinct);\ne103f727 1159) res = -1;\ne103f727 1160) goto cleanup;\ne103f727 1169) error(_(\"too many commits to write graph\"));\ne103f727 1170) res = -1;\ne103f727 1171) goto cleanup;\n\nname-hash.c\n568a05c5 348) assert(begin >= 0);\n568a05c5 350) int mid = begin + ((end - begin) >> 1);\n\npackfile.c\n921d49be 1275) COPY_ARRAY(poi_stack, small_poi_stack, poi_stack_nr);\n921d49be 1685) COPY_ARRAY(delta_stack, small_delta_stack,\n\nref-filter.c\n2582083f 93) keydata_aka_refname ? keydata_aka_refname : k->wt->head_ref);\n\nsh-i18n--envsubst.c\n568a05c5 252)   size_t j = j1 + ((j2 - j1) >> 1);\n\nCommits introducting uncovered code:\nDerrick Stolee\te103f727 commit-graph: return with errors during write\nDerrick Stolee\tef5b83f2 commit-graph: extract fill_oids_from_packs()\nDerrick Stolee\t4c9efe85 commit-graph: extract fill_oids_from_commit_hex()\nDerrick Stolee\tb2c83060 commit-graph: extract fill_oids_from_all_packs()\nDerrick Stolee\t238def57 commit-graph: extract write_commit_graph_file()\nNickolai Belakovski\t2582083f ref-filter: add worktreepath atom\nPhilip Oakley\t1fde99cf doc branch: provide examples for listing remote tracking branches\nRené Scharfe\t568a05c5 cleanup: fix possible overflow errors in binary search, part 2\nRené Scharfe\t921d49be use COPY_ARRAY for copying arrays\n\n\nUncovered code in 'master' not in 'master@{1}'\n--------------------------------------------------------\n\nbuiltin/am.c\n97387c8b 1662) die(\"unable to read from stdin; aborting\");\n6e7baf24 2336) die(_(\"interactive mode requires patches on the command line\"));\n\nbuiltin/bisect--helper.c\n7877ac3d 574) retval = error(_(\"invalid ref: '%s'\"), start_head.buf);\n7877ac3d 575) goto finish;\n\nbuiltin/fast-export.c\ne80001f8 81) static int parse_opt_reencode_mode(const struct option *opt,\ne80001f8 84) if (unset) {\ne80001f8 85) reencode_mode = REENCODE_ABORT;\ne80001f8 86) return 0;\ne80001f8 89) switch (git_parse_maybe_bool(arg)) {\ne80001f8 91) reencode_mode = REENCODE_NO;\ne80001f8 92) break;\ne80001f8 94) reencode_mode = REENCODE_YES;\ne80001f8 95) break;\ne80001f8 97) if (!strcasecmp(arg, \"abort\"))\ne80001f8 98) reencode_mode = REENCODE_ABORT;\ne80001f8 100) return error(\"Unknown reencoding mode: %s\", arg);\ne80001f8 103) return 0;\ne80001f8 665) switch(reencode_mode) {\ne80001f8 667) reencoded = reencode_string(message, \"UTF-8\", encoding);\ne80001f8 668) break;\ne80001f8 670) break;\ne80001f8 672) die(\"Encountered commit-specific encoding %s in commit \"\ne80001f8 674)     encoding, oid_to_hex(&commit->object.oid));\nccbfc96d 686) printf(\"encoding %s\\n\", encoding);\n\nbuiltin/log.c\n13cdf780 873) return 0;\n\nbuiltin/merge.c\nf3f8311e 1290) usage_msg_opt(_(\"--quit expects no arguments\"),\n\nbuiltin/worktree.c\n1de16aec 297) BUG(\"How come '%s' becomes empty after sanitization?\", sb.buf);\n\nbuiltin/write-tree.c\n76a7bc09 53) die(\"%s: prefix %s not found\", me, tree_prefix);\n\nfast-import.c\n3edfcc65 2612) read_next_command();\n3edfcc65 2679) strbuf_addf(&new_data,\n\ngrep.c\nde99eb0c 1784) BUG(\"grep call which could print a name requires \"\n\nlist-objects-filter-options.c\n5c03bc8b 94) strbuf_addf(errbuf, _(\"invalid filter-spec '%s'\"), arg);\n\nread-cache.c\n7bd9631b 2201) src_offset += load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);\n\nrefs.c\n1de16aec 111) sanitized->buf[sanitized->len-1] = '-';\n1de16aec 170) if (sanitized)\n1de16aec 171) strbuf_addch(sanitized, '-');\n1de16aec 173) return -1;\n1de16aec 178) strbuf_complete(sanitized, '/');\n1de16aec 215) BUG(\"sanitizing refname '%s' check returned error\", refname);\n\nserver-info.c\nf4f476b6 110) ret = -1;\nf4f476b6 111) goto out;\nf4f476b6 123) goto out;\nf4f476b6 125) goto out;\nf4f476b6 134) if (uic.cur_fp)\nf4f476b6 135) fclose(uic.cur_fp);\n\nCommits introducting uncovered code:\nDenton Liu\t13cdf780 format-patch: teach format.notes config option\nElijah Newren\te80001f8 fast-export: do automatic reencoding of commit messages only if requested\nElijah Newren\tccbfc96d fast-export: avoid stripping encoding header if we cannot reencode\nElijah Newren\t3edfcc65 fast-import: support 'encoding' commit header\nEmily Shaffer\tde99eb0c grep: fail if call could output and name is null\nEric Wong\tf4f476b6 update-server-info: avoid needless overwrites\nJeff King\t97387c8b am: read interactive input from stdin\nJeff King\t6e7baf24 am: drop tty requirement for --interactive\nJeff King\t76a7bc09 cmd_{read,write}_tree: rename \"unused\" variable that is used\nJeff King\t7bd9631b read-cache: drop unused parameter from threaded load\nJohannes Schindelin\t7877ac3d bisect--helper: verify HEAD could be parsed before continuing\nMatthew DeVore\t5c03bc8b list-objects-filter-options: error is localizeable\nNguyễn Thái Ngọc Duy\tf3f8311e merge: add --quit\nNguyễn Thái Ngọc Duy\t1de16aec worktree add: sanitize worktree names\n\n"},{"id":"378178","messageId":"14689d27-eecd-2e0a-715d-796b20d573e5@gmail.com","threadId":"51396","inReplyTo":"49d98293-9f0b-44e9-cb07-d6b7ac791eb6@gmail.com","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-27T17:35:17Z","receivedAt":"2019-06-27T17:35:20Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Here are some interesting sections I found when examining the test coverage\nreport. I am only highlighting these sections because they seem to include\nnon-trivial logic. In some cases, maybe the code isn't needed.\n\nOn 6/27/2019 1:05 PM, Derrick Stolee wrote:\n> promisor-remote.c\n> db27dca5 25) die(_(\"Remote with no URL\"));\n> 48de3158 61) warning(_(\"promisor remote name cannot begin with '/': %s\"),\n> 48de3158 63) return NULL;\n> faf2abf4 93) previous->next = r->next;\n> 4ca9474e 108) return git_config_string(&core_partial_clone_filter_default,\n> fa3d1b63 139) return 0;\n> 9e27beaa 202) static int remove_fetched_oids(struct repository *repo,\n> 9e27beaa 206) int i, remaining_nr = 0;\n> 9e27beaa 207) int *remaining = xcalloc(oid_nr, sizeof(*remaining));\n> 9e27beaa 208) struct object_id *old_oids = *oids;\n> 9e27beaa 211) for (i = 0; i < oid_nr; i++)\n> 9e27beaa 212) if (oid_object_info_extended(repo, &old_oids[i], NULL,\n> 9e27beaa 214) remaining[i] = 1;\n> 9e27beaa 215) remaining_nr++;\n> 9e27beaa 218) if (remaining_nr) {\n> 9e27beaa 219) int j = 0;\n> 9e27beaa 220) new_oids = xcalloc(remaining_nr, sizeof(*new_oids));\n> 9e27beaa 221) for (i = 0; i < oid_nr; i++)\n> 9e27beaa 222) if (remaining[i])\n> 9e27beaa 223) oidcpy(&new_oids[j++], &old_oids[i]);\n> 9e27beaa 224) *oids = new_oids;\n> 9e27beaa 225) if (to_free)\n> 9e27beaa 226) free(old_oids);\n> 9e27beaa 229) free(remaining);\n> 9e27beaa 231) return remaining_nr;\n> 9e27beaa 248) if (remaining_nr == 1)\n> 9e27beaa 249) continue;\n> 9e27beaa 250) remaining_nr = remove_fetched_oids(repo, &remaining_oids,\n> 9e27beaa 252) if (remaining_nr) {\n> 9e27beaa 253) to_free = 1;\n> 9e27beaa 254) continue;\n> 9e27beaa 262) free(remaining_oids);\n\nChristian: this section continues to be untested, but I think you were\nworking on creating tests for this.\n\n> repo-settings.c\n> 0a01e977 13) int rate = git_config_int(key, value);\n> 0a01e977 14) if (rate >= 3) {\n> 0a01e977 15) UPDATE_DEFAULT(rs->core_commit_graph, 1);\n> 0a01e977 16) UPDATE_DEFAULT(rs->gc_write_commit_graph, 1);\n> c5c84f32 17) UPDATE_DEFAULT(rs->index_version, 4);\n> 3172404b 19) if (rate >= 5) {\n> 3172404b 20) UPDATE_DEFAULT(rs->pack_use_sparse, 1);\n> 0a01e977 22) return 0;\n\nThese are mine. Since no one has been complaining about the design\nof core.featureAdoptionRate in ds/early-access [1], I'll move forward\nto add some tests for this setting. It may come in the form of a\nGIT_TEST_ADOPTION_RATE environment variable so it has wider coverage\nacross the test suite. I may even add explicit tests that demonstrate\nthe new defaults enabled by core.featureAdoptionRate are overridden by\nexplicit config settings. index.version is a good candidate.\n\n[1] https://public-inbox.org/git/pull.254.v2.git.gitgitgadget@gmail.com/\n\n> t/helper/test-match-trees.c\n> 3fe87a7f 23) shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1);\n\nDuy: here is another example of a conversion to \"struct repository *\" but\nsince it is using the_repository here it is definitely safe (no worse than before).\n\n> builtin/fetch.c\n> cdbd70c4 88) fetch_show_forced_updates = git_config_bool(k, v);\n> cdbd70c4 89) return 0;\n\nMine again. I should explicitly test the fetch.showForcedUpdates config option,\nespecially against the '--no-show-forced-updates' argument (to show the argument\nwins).\n\n> builtin/pull.c\n> 3883c551 556) argv_array_push(&args, \"--show-forced-updates\");\n> 3883c551 558) argv_array_push(&args, \"--no-show-forced-updates\");\n\nNot sure if this is super-important to test. It's a simple argument pass-through.\nBut also maybe I change my 'git fetch' tests to be 'git pull' tests and it covers\nthe 'git fetch' builtin at the same time.\n\n> dir.c\n> 3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,\n> 3b2385cf 2845) jw_object_bool(jw, \"valid\", ucd->valid);\n> 3b2385cf 2846) jw_object_bool(jw, \"check-only\", ucd->check_only);\n> 3b2385cf 2847) jw_object_stat_data(jw, \"stat\", &ucd->stat_data);\n> 3b2385cf 2848) jw_object_string(jw, \"exclude-oid\", oid_to_hex(&ucd->exclude_oid));\n> 3b2385cf 2849) jw_object_inline_begin_array(jw, \"untracked\");\n> 3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)\n> 3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);\n> 3b2385cf 2852) jw_end(jw);\n> 3b2385cf 2854) jw_object_inline_begin_object(jw, \"dirs\");\n> 3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {\n> 3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);\n> 3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);\n> 3b2385cf 2858) jw_end(jw);\n> 3b2385cf 2860) jw_end(jw);\n> 3b2385cf 2861) }\n> 3b2385cf 2958) jw_object_inline_begin_object(jw, \"root\");\n> 3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);\n> 3b2385cf 2960) jw_end(jw);\n\nDuy: I know you were working on some tests for these options. This is specifically\nin the \"untracked cache\" mode, so enabling the cache with at least one entry and\nrunning --debug-json should be sufficient.\n\n> pack-bitmap-write.c\n> 05805d74 378) static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)\n> d2bc62b1 385) hash_pos = kh_get_oid_map(writer.reused, *oid);\n> 05805d74 425) reused_bitmap = find_reused_bitmap(&chosen->object.oid);\n> 05805d74 432) reused_bitmap = find_reused_bitmap(&cm->object.oid);\n\nPeff: it is interesting that these portions are not covered previously. (Your change\nis clearly mechanical and does not change the correctness.) In particular, lines 425\nand 432 are in two blocks of an if/else with one further inside a loop. The loop\nshould always have at least one run, so this if/else isn't even covered.\n\n> read-cache.c\n> 8eeabe15 1752) ret = error(_(\"index uses %.4s extension, which we do not understand\"),\n> ee70c128 1754) if (advice_unknown_index_extension) {\n> ee70c128 1755) warning(_(\"ignoring optional %.4s index extension\"), ext);\n> ee70c128 1756) advise(_(\"This is likely due to the file having been written by a newer\\n\"\n> 272b3f2a 2026) jw_object_true(jw, \"assume_unchanged\");\n> 272b3f2a 2030) jw_object_true(jw, \"skip_worktree\");\n> 272b3f2a 2032) jw_object_intmax(jw, \"stage\", ce_stage(ce));\n> f0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);\n> f0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(\n> f0f544da 3673) return do_read_ieot_extension(istate, index, extsize);\n\nDuy: more JSON output cases that could be interesting to cover.\n\n> t/helper/test-example-decorate.c\n> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);\n> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);\n> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);\n\nPeff: again interesting that these lines you refactored were not covered, especially\nbecause they are part of a test helper. Perhaps the tests they were intended for are\nnow defunct?\n \n> t/helper/test-oidmap.c\n> 11510dec 52) if (get_oid(p1, &oid)) {\n> 11510dec 53) printf(\"Unknown oid: %s\\n\", p1);\n> 11510dec 54) continue;\n> 11510dec 58) FLEX_ALLOC_STR(entry, name, p2);\n> 11510dec 59) oidcpy(&entry->entry.oid, &oid);\n> 11510dec 62) oidmap_put(&map, entry);\n\nChristian: this block looks like the test-oidmap helper never uses the \"add\"\nsubcommand. Is that correct?\n\n> 11510dec 97) if (get_oid(p1, &oid)) {\n> 11510dec 98) printf(\"Unknown oid: %s\\n\", p1);\n> 11510dec 99) continue;\n> 11510dec 103) entry = oidmap_remove(&map, &oid);\n> 11510dec 106) puts(entry ? entry->name : \"NULL\");\n> 11510dec 107) free(entry);\n\nSimilarly, this block means we are not using the \"remove\" subcommand.\n\n\nThanks,\n-Stolee\n"},{"id":"378238","messageId":"20190628064103.GA19777@sigill.intra.peff.net","threadId":"51396","inReplyTo":"14689d27-eecd-2e0a-715d-796b20d573e5@gmail.com","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T06:41:03Z","receivedAt":"2019-06-28T06:41:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:\n\n> > pack-bitmap-write.c\n> > 05805d74 378) static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)\n> > d2bc62b1 385) hash_pos = kh_get_oid_map(writer.reused, *oid);\n> > 05805d74 425) reused_bitmap = find_reused_bitmap(&chosen->object.oid);\n> > 05805d74 432) reused_bitmap = find_reused_bitmap(&cm->object.oid);\n> \n> Peff: it is interesting that these portions are not covered previously. (Your change\n> is clearly mechanical and does not change the correctness.) In particular, lines 425\n> and 432 are in two blocks of an if/else with one further inside a loop. The loop\n> should always have at least one run, so this if/else isn't even covered.\n\nOne of 425 or 432 must run if we enter that \"for(;;)\" loop (in the\nlatter case, \"next\" is non-zero, so we enter the inner loop at least\nonce).\n\nI think that the whole loop starting at line 409 is not exercised by the\ntest suite, because we hit the early return above it when there are\nfewer than 100 commits to index.\n\nI think this would exercise it, at the cost of making the test more\nexpensive:\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 82d7f7f6a5..8ed6982dcb 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -21,7 +21,7 @@ has_any () {\n }\n \n test_expect_success 'setup repo with moderate-sized history' '\n-\tfor i in $(test_seq 1 10)\n+\tfor i in $(test_seq 1 100)\n \tdo\n \t\ttest_commit $i\n \tdone &&\n\nIt would be nice if we had a \"test_commits_bulk\" that used fast-import\nto create larger numbers of commits.\n\n-Peff\n"},{"id":"378239","messageId":"20190628064520.GB19777@sigill.intra.peff.net","threadId":"51396","inReplyTo":"14689d27-eecd-2e0a-715d-796b20d573e5@gmail.com","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T06:45:21Z","receivedAt":"2019-06-28T06:45:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:\n\n> > t/helper/test-example-decorate.c\n> > 0ebbcf70 29) one = lookup_unknown_object(&one_oid);\n> > 0ebbcf70 30) two = lookup_unknown_object(&two_oid);\n> > 0ebbcf70 59) three = lookup_unknown_object(&three_oid);\n> \n> Peff: again interesting that these lines you refactored were not covered, especially\n> because they are part of a test helper. Perhaps the tests they were intended for are\n> now defunct?\n\nThey should be run by t9004 (and if I replace them with a `die`, they\nclearly are). Are you sure your coverage script is not mistaken?\n\n-Peff\n"},{"id":"378244","messageId":"20190628093751.GA3569@sigill.intra.peff.net","threadId":"51396","inReplyTo":"20190628064103.GA19777@sigill.intra.peff.net","subject":"[PATCH 0/6] easy bulk commit creation in tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T09:37:52Z","receivedAt":"2019-06-28T09:37:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 02:41:03AM -0400, Jeff King wrote:\n\n> I think this would exercise it, at the cost of making the test more\n> expensive:\n> \n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index 82d7f7f6a5..8ed6982dcb 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -21,7 +21,7 @@ has_any () {\n>  }\n>  \n>  test_expect_success 'setup repo with moderate-sized history' '\n> -\tfor i in $(test_seq 1 10)\n> +\tfor i in $(test_seq 1 100)\n>  \tdo\n>  \t\ttest_commit $i\n>  \tdone &&\n> \n> It would be nice if we had a \"test_commits_bulk\" that used fast-import\n> to create larger numbers of commits.\n\nSo here's a patch to do that. Writing the bulk commit function was a fun\nexercise, and I found a couple other places to apply it, too, shaving\noff ~7.5 seconds from my test runs. Not ground-breaking, but I think\nit's nice to have a solution where we don't have to be afraid to\ngenerate a bunch of commits.\n\nI'm sure there are other spots that could be converted, too (either ones\nthat are slow loops now, or ones that use fast-import themselves but\ncould be made more readable by using the helper), but I stopped digging\nafter finding the low-hanging fruit here.\n\n  [1/6]: test-lib: introduce test_commit_bulk\n  [2/6]: t5310: increase the number of bitmapped commits\n  [3/6]: t3311: use test_commit_bulk\n  [4/6]: t5702: use test_commit_bulk\n  [5/6]: t5703: use test_commit_bulk\n  [6/6]: t6200: use test_commit_bulk\n\n t/t3311-notes-merge-fanout.sh      |  10 +--\n t/t5310-pack-bitmaps.sh            |  15 +---\n t/t5702-protocol-v2.sh             |  10 +--\n t/t5703-upload-pack-ref-in-want.sh |   4 +-\n t/t6200-fmt-merge-msg.sh           |   7 +-\n t/test-lib-functions.sh            | 131 +++++++++++++++++++++++++++++\n 6 files changed, 144 insertions(+), 33 deletions(-)\n\n-Peff\n"},{"id":"378245","messageId":"20190628093911.GA27329@sigill.intra.peff.net","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"[PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T09:39:11Z","receivedAt":"2019-06-28T09:39:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some tests need to create a string of commits. Doing this with\ntest_commit is very heavy-weight, as it needs at least one process per\ncommit (and in fact, uses several).\n\nFor bulk creation, we can do much better by using fast-import, but it's\noften a pain to generate the input. Let's provide a helper to do so.\n\nWe'll use t5310 as a guinea pig, as it has three 10-commit loops. Here\nare hyperfine results before and after:\n\n  [before]\n  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]\n    Range (min … max):    2.250 s …  3.210 s    10 runs\n\n  [after]\n  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]\n    Range (min … max):    1.999 s …  2.590 s    10 runs\n\nSo we're over 20% faster, while making the callers slightly shorter. We\nadded a lot more lines in test-lib-function.sh, of course, and the\nhelper is way more featureful than we need here. But my hope is that it\nwill be flexible enough to use in more places.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5310-pack-bitmaps.sh |  15 +----\n t/test-lib-functions.sh | 131 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 134 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a26c8ba9a2..3aab7024ca 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -21,15 +21,9 @@ has_any () {\n }\n \n test_expect_success 'setup repo with moderate-sized history' '\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit $i\n-\tdone &&\n+\ttest_commit_bulk --id=file 10 &&\n \tgit checkout -b other HEAD~5 &&\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit side-$i\n-\tdone &&\n+\ttest_commit_bulk --id=side 10 &&\n \tgit checkout master &&\n \tbitmaptip=$(git rev-parse master) &&\n \tblob=$(echo tagged-blob | git hash-object -w --stdin) &&\n@@ -106,10 +100,7 @@ test_expect_success 'clone from bitmapped repository' '\n '\n \n test_expect_success 'setup further non-bitmapped commits' '\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit further-$i\n-\tdone\n+\ttest_commit_bulk --id=further 10\n '\n \n rev_list_tests 'partial bitmap'\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0367cec5fd..32a1db81a3 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -233,6 +233,137 @@ test_merge () {\n \tgit tag \"$1\"\n }\n \n+# Similar to test_commit, but efficiently create <nr> commits, each with a\n+# unique number $n (from 1 to <nr> by default) in the commit message.\n+#\n+# Usage: test_commit_bulk [options] <nr>\n+#   -C <dir>:\n+#\tRun all git commands in directory <dir>\n+#   --ref=<n>:\n+#\tref on which to create commits (default: HEAD)\n+#   --start=<n>:\n+#\tnumber commit messages from <n> (default: 1)\n+#   --message=<msg>:\n+#\tuse <msg> as the commit mesasge (default: \"commit $n\")\n+#   --filename=<fn>:\n+#\tmodify <fn> in each commit (default: $n.t)\n+#   --contents=<string>:\n+#\tplace <string> in each file (default: \"content $n\")\n+#   --id=<string>:\n+#\tshorthand to use <string> and $n in message, filename, and contents\n+#\n+# The message, filename, and contents strings are evaluated by the shell inside\n+# double-quotes, with $n set to the current commit number. So you can do:\n+#\n+#   test_commit_bulk --filename=file --contents='modification $n'\n+#\n+# to have every commit touch the same file, but with unique content. Spaces are\n+# OK, but you must escape any metacharacters (like backslashes or\n+# double-quotes) you do not want expanded.\n+#\n+test_commit_bulk () {\n+\tindir=\n+\tref=HEAD\n+\tn=1\n+\tmessage='commit $n'\n+\tfilename='$n.t'\n+\tcontents='content $n'\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-C)\n+\t\t\tindir=$2\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--ref=*)\n+\t\t\tref=${1#--*=}\n+\t\t\t;;\n+\t\t--start=*)\n+\t\t\tn=${1#--*=}\n+\t\t\t;;\n+\t\t--message=*)\n+\t\t\tmessage=${1#--*=}\n+\t\t\t;;\n+\t\t--filename=*)\n+\t\t\tfilename=${1#--*=}\n+\t\t\t;;\n+\t\t--contents=*)\n+\t\t\tcontents=${1#--*=}\n+\t\t\t;;\n+\t\t--id=*)\n+\t\t\tmessage=\"${1#--*=} \\$n\"\n+\t\t\tfilename=\"${1#--*=}-\\$n.t\"\n+\t\t\tcontents=\"${1#--*=} \\$n\"\n+\t\t\t;;\n+\t\t-*)\n+\t\t\tBUG \"invalid test_commit_bulk option: $1\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\ttotal=$1\n+\n+\tin_dir=${indir:+-C \"$indir\"}\n+\n+\t# Any test_tick calls inside the loop will not affect our outer\n+\t# timestamp, since it's on the left-hand side of a pipe. So start with\n+\t# a known value now, increment in the loop, and then do the matching\n+\t# math here. The final test_tick updates the $GIT_* variables\n+\ttest_tick\n+\tcur_time=$test_tick\n+\ttest_tick=$((test_tick + total))\n+\ttest_tick\n+\n+\n+\t{\n+\t\t# A \"reset ... from\" instructs fastimport to build on an\n+\t\t# existing branch tip rather than trying to overwrite.\n+\t\tif tip=$(git ${indir:+ -C \"$indir\"} \\\n+\t\t\t rev-parse --verify \"$ref\" 2>/dev/null)\n+\t\tthen\n+\t\t\techo \"reset $ref\"\n+\t\t\techo \"from $tip\"\n+\t\tfi\n+\n+\t\twhile test \"$total\" -gt 0\n+\t\tdo\n+\t\t\techo \"commit $ref\" &&\n+\t\t\tprintf 'author %s <%s> %s\\n' \\\n+\t\t\t\t\"$GIT_AUTHOR_NAME\" \\\n+\t\t\t\t\"$GIT_AUTHOR_EMAIL\" \\\n+\t\t\t\t\"$cur_time -0700\" &&\n+\t\t\tprintf 'committer %s <%s> %s\\n' \\\n+\t\t\t\t\"$GIT_COMMITTER_NAME\" \\\n+\t\t\t\t\"$GIT_COMMITTER_EMAIL\" \\\n+\t\t\t\t\"$cur_time -0700\" &&\n+\t\t\techo \"data <<EOF\" &&\n+\t\t\teval \"echo \\\"$message\\\"\" &&\n+\t\t\techo \"EOF\" &&\n+\t\t\teval \"echo \\\"M 644 inline $filename\\\"\" &&\n+\t\t\techo \"data <<EOF\" &&\n+\t\t\teval \"echo \\\"$contents\\\"\" &&\n+\t\t\techo \"EOF\" &&\n+\t\t\techo &&\n+\t\t\tn=$((n + 1)) &&\n+\t\t\tcur_time=$((cur_time + 1)) &&\n+\t\t\ttotal=$((total - 1)) ||\n+\t\t\techo \"poison fast-import stream\"\n+\t\tdone\n+\t} | git ${indir:+ -C \"$indir\"} \\\n+\t\t-c fastimport.unpacklimit=0 \\\n+\t\tfast-import || return 1\n+\n+\t# If we updated HEAD, then be nice and update the index and working\n+\t# tree, too.\n+\tif test \"$ref\" = \"HEAD\"\n+\tthen\n+\t\tgit ${indir:+ -C \"$indir\"} checkout -f HEAD || return 1\n+\tfi\n+}\n+\n # This function helps systems where core.filemode=false is set.\n # Use it instead of plain 'chmod +x' to set or unset the executable bit\n # of a file in the working directory and add it to the index.\n-- \n2.22.0.768.gd89de1e449\n\n"},{"id":"378246","messageId":"20190628093942.GB27329@sigill.intra.peff.net","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"[PATCH 2/6] t5310: increase the number of bitmapped commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T09:39:42Z","receivedAt":"2019-06-28T09:39:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The bitmap index we compute in t5310 has only 20 commits in it. This\ngives poor coverage of bitmap_writer_select_commits(), which simply\nwrites a bitmap for everything when there are fewer than 100 commits.\n\nLet's bump the number of commits in the test to cover the more complex\ncode paths (this does drop coverage of the individual lines of the\ntrivial path, but the complex path does everything it does and more).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5310-pack-bitmaps.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 3aab7024ca..6640329ebf 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -21,7 +21,7 @@ has_any () {\n }\n \n test_expect_success 'setup repo with moderate-sized history' '\n-\ttest_commit_bulk --id=file 10 &&\n+\ttest_commit_bulk --id=file 100 &&\n \tgit checkout -b other HEAD~5 &&\n \ttest_commit_bulk --id=side 10 &&\n \tgit checkout master &&\n-- \n2.22.0.768.gd89de1e449\n\n"},{"id":"378247","messageId":"20190628094135.GC27329@sigill.intra.peff.net","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"[PATCH 3/6] t3311: use test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T09:41:35Z","receivedAt":"2019-06-28T09:41:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of the tests in t3311 creates 300 commits by running \"test_commit\"\nin a loop. This requires 900 processes. Instead, we can use\ntest_commit_bulk to do it with only four. This improves the runtime of\nthe script from:\n\n  Benchmark #1: ./t3311-notes-merge-fanout.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      5.821 s ±  0.691 s    [User: 3.146 s, System: 2.782 s]\n    Range (min … max):    4.783 s …  6.841 s    10 runs\n\nto:\n\n  Benchmark #1: ./t3311-notes-merge-fanout.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      1.743 s ±  0.116 s    [User: 1.144 s, System: 0.691 s]\n    Range (min … max):    1.629 s …  1.994 s    10 runs\n\nfor an average speedup of over 70%.\n\nUnfortunately we still have to run 300 instances of \"git notes add\",\nsince the point is to test the fanout that comes from adding notes one\nby one.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nRe-reading the patch again, the notes numbering will be in reverse\norder from the original. That's fine for the purposes of the test,\nthough I could probably fix it if we want to be pedantic.\n\n t/t3311-notes-merge-fanout.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh\nindex 93516ef67c..37151a3adc 100755\n--- a/t/t3311-notes-merge-fanout.sh\n+++ b/t/t3311-notes-merge-fanout.sh\n@@ -114,12 +114,12 @@ cp expect_log_x expect_log_y\n test_expect_success 'Add a few hundred commits w/notes to trigger fanout (x -> y)' '\n \tgit update-ref refs/notes/y refs/notes/x &&\n \tgit config core.notesRef refs/notes/y &&\n-\ti=5 &&\n-\twhile test $i -lt $num\n+\ttest_commit_bulk --start=6 --id=commit $((num - 5)) &&\n+\ti=0 &&\n+\twhile test $i -lt $((num - 5))\n \tdo\n-\t\ti=$(($i + 1)) &&\n-\t\ttest_commit \"commit$i\" >/dev/null &&\n-\t\tgit notes add -m \"notes for commit$i\" || return 1\n+\t\tgit notes add -m \"notes for commit$i\" HEAD~$i || return 1\n+\t\ti=$((i + 1))\n \tdone &&\n \ttest \"$(git rev-parse refs/notes/y)\" != \"$(git rev-parse refs/notes/x)\" &&\n \t# Expected number of commits and notes\n-- \n2.22.0.768.gd89de1e449\n\n"},{"id":"378248","messageId":"20190628094153.GD27329@sigill.intra.peff.net","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"[PATCH 4/6] t5702: use test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T09:41:54Z","receivedAt":"2019-06-28T09:41:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are two loops that create 32 commits each using test_commit. Using\ntest_commit_bulk speeds this up from:\n\n  Benchmark #1: ./t5702-protocol-v2.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      5.409 s ±  0.513 s    [User: 2.382 s, System: 2.466 s]\n    Range (min … max):    4.633 s …  5.927 s    10 runs\n\nto:\n\n  Benchmark #1: ./t5702-protocol-v2.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      3.956 s ±  0.242 s    [User: 1.775 s, System: 1.627 s]\n    Range (min … max):    3.449 s …  4.239 s    10 runs\n\nfor an average savings of over 25%.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5702-protocol-v2.sh | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5b33f625dd..011b81d4fc 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -499,10 +499,7 @@ test_expect_success 'upload-pack respects client shallows' '\n \n \t# Add extra commits to the client so that the whole fetch takes more\n \t# than 1 request (due to negotiation)\n-\tfor i in $(test_seq 1 32)\n-\tdo\n-\t\ttest_commit -C client c$i\n-\tdone &&\n+\ttest_commit_bulk -C client --id=c 32 &&\n \n \tgit -C server checkout -b newbranch base &&\n \ttest_commit -C server client_wants &&\n@@ -711,10 +708,7 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n \t# Create many commits to extend the negotiation phase across multiple\n \t# requests, so that the server does not send \"ready\" in the first\n \t# request.\n-\tfor i in $(test_seq 1 32)\n-\tdo\n-\t\ttest_commit -C http_child c$i\n-\tdone &&\n+\ttest_commit_bulk -C http_child --id=c 32 &&\n \n \t# After the acknowledgments section, pretend that a DELIM\n \t# (0001) was sent instead of a FLUSH (0000).\n-- \n2.22.0.768.gd89de1e449\n\n"},{"id":"378249","messageId":"20190628094207.GE27329@sigill.intra.peff.net","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"[PATCH 5/6] t5703: use test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T09:42:07Z","receivedAt":"2019-06-28T09:42:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are two loops that create 33 commits each using test_commit. Using\ntest_commit_bulk speeds this up from:\n\n  Benchmark #1: ./t5703-upload-pack-ref-in-want.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      2.142 s ±  0.161 s    [User: 1.136 s, System: 0.974 s]\n    Range (min … max):    1.903 s …  2.401 s    10 runs\n\nto:\n\n  Benchmark #1: ./t5703-upload-pack-ref-in-want.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      1.440 s ±  0.114 s    [User: 737.7 ms, System: 615.4 ms]\n    Range (min … max):    1.230 s …  1.604 s    10 runs\n\nfor an average savings of almost 33%.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5703-upload-pack-ref-in-want.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex 0951d1bbdc..de4b6106ef 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -176,7 +176,7 @@ test_expect_success 'setup repos for change-while-negotiating test' '\n \t\tgit clone \"http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo\" \"$LOCAL_PRISTINE\" &&\n \t\tcd \"$LOCAL_PRISTINE\" &&\n \t\tgit checkout -b side &&\n-\t\tfor i in $(test_seq 1 33); do test_commit s$i; done &&\n+\t\ttest_commit_bulk --id=s 33 &&\n \n \t\t# Add novel commits to upstream\n \t\tgit checkout master &&\n@@ -287,7 +287,7 @@ test_expect_success 'setup repos for fetching with ref-in-want tests' '\n \t\tgit clone \"file://$REPO\" \"$LOCAL_PRISTINE\" &&\n \t\tcd \"$LOCAL_PRISTINE\" &&\n \t\tgit checkout -b side &&\n-\t\tfor i in $(test_seq 1 33); do test_commit s$i; done &&\n+\t\ttest_commit_bulk --id=s 33 &&\n \n \t\t# Add novel commits to upstream\n \t\tgit checkout master &&\n-- \n2.22.0.768.gd89de1e449\n\n"},{"id":"378250","messageId":"20190628094212.GF27329@sigill.intra.peff.net","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"[PATCH 6/6] t6200: use test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T09:42:12Z","receivedAt":"2019-06-28T09:42:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a loop that creates 30 commits using test_commit. Using\ntest_commit_bulk speeds this up from:\n\n  Benchmark #1: ./t6200-fmt-merge-msg.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      1.926 s ±  0.240 s    [User: 1.055 s, System: 0.963 s]\n    Range (min … max):    1.431 s …  2.166 s    10 runs\n\nto:\n\n  Benchmark #1: ./t6200-fmt-merge-msg.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      1.343 s ±  0.179 s    [User: 766.5 ms, System: 662.9 ms]\n    Range (min … max):    1.032 s …  1.664 s    10 runs\n\nfor an average savings of over 30%.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6200-fmt-merge-msg.sh | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh\nindex 93f23cfa82..d4e5af4338 100755\n--- a/t/t6200-fmt-merge-msg.sh\n+++ b/t/t6200-fmt-merge-msg.sh\n@@ -66,12 +66,7 @@ test_expect_success setup '\n \tgit commit -a -m \"Right #5\" &&\n \n \tgit checkout -b long &&\n-\ti=0 &&\n-\twhile test $i -lt 30\n-\tdo\n-\t\ttest_commit $i one &&\n-\t\ti=$(($i+1))\n-\tdone &&\n+\ttest_commit_bulk --start=0 --message=\"\\$n\" --filename=one 30 &&\n \n \tgit show-branch &&\n \n-- \n2.22.0.768.gd89de1e449\n"},{"id":"378251","messageId":"CACsJy8C7Qr3JauRqUkB957ZDzVVZ0+wofzs7N2NzCbu+e00JnQ@mail.gmail.com","threadId":"51396","inReplyTo":"14689d27-eecd-2e0a-715d-796b20d573e5@gmail.com","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-28T09:47:20Z","receivedAt":"2019-06-28T09:47:48Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jun 28, 2019 at 12:35 AM Derrick Stolee <stolee@gmail.com> wrote:\n> > dir.c\n> > 3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,\n> > 3b2385cf 2845) jw_object_bool(jw, \"valid\", ucd->valid);\n> > 3b2385cf 2846) jw_object_bool(jw, \"check-only\", ucd->check_only);\n> > 3b2385cf 2847) jw_object_stat_data(jw, \"stat\", &ucd->stat_data);\n> > 3b2385cf 2848) jw_object_string(jw, \"exclude-oid\", oid_to_hex(&ucd->exclude_oid));\n> > 3b2385cf 2849) jw_object_inline_begin_array(jw, \"untracked\");\n> > 3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)\n> > 3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);\n> > 3b2385cf 2852) jw_end(jw);\n> > 3b2385cf 2854) jw_object_inline_begin_object(jw, \"dirs\");\n> > 3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {\n> > 3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);\n> > 3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);\n> > 3b2385cf 2858) jw_end(jw);\n> > 3b2385cf 2860) jw_end(jw);\n> > 3b2385cf 2861) }\n> > 3b2385cf 2958) jw_object_inline_begin_object(jw, \"root\");\n> > 3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);\n> > 3b2385cf 2960) jw_end(jw);\n>\n> Duy: I know you were working on some tests for these options. This is specifically\n> in the \"untracked cache\" mode, so enabling the cache with at least one entry and\n> running --debug-json should be sufficient.\n\nIt's a bit more complicated than that, but I see your point. I\ninitially looked at the output and saw \"something\" and moved on. I\nshould have examined the json output more carefully.\n\n> > read-cache.c\n> > 8eeabe15 1752) ret = error(_(\"index uses %.4s extension, which we do not understand\"),\n> > ee70c128 1754) if (advice_unknown_index_extension) {\n> > ee70c128 1755) warning(_(\"ignoring optional %.4s index extension\"), ext);\n> > ee70c128 1756) advise(_(\"This is likely due to the file having been written by a newer\\n\"\n> > 272b3f2a 2026) jw_object_true(jw, \"assume_unchanged\");\n> > 272b3f2a 2030) jw_object_true(jw, \"skip_worktree\");\n> > 272b3f2a 2032) jw_object_intmax(jw, \"stage\", ce_stage(ce));\n> > f0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);\n> > f0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(\n> > f0f544da 3673) return do_read_ieot_extension(istate, index, extsize);\n>\n> Duy: more JSON output cases that could be interesting to cover.\n\nThis is because I changed the function signature, I think. Both IEOT\nand EOIE extensions, if I'm not mistaken, are never tested in the test\nsuite. You need to set GIT_TEST_INDEX_THREADS, then the last three\nlines should be covered.\n-- \nDuy\n"},{"id":"378261","messageId":"ddf13a6b-42f4-57ae-49f1-8cc758b1dc4b@gmail.com","threadId":"51396","inReplyTo":"20190628064520.GB19777@sigill.intra.peff.net","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-28T12:23:49Z","receivedAt":"2019-06-28T12:24:04Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/28/2019 2:45 AM, Jeff King wrote:\n> On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:\n> \n>>> t/helper/test-example-decorate.c\n>>> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);\n>>> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);\n>>> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);\n>>\n>> Peff: again interesting that these lines you refactored were not covered, especially\n>> because they are part of a test helper. Perhaps the tests they were intended for are\n>> now defunct?\n> \n> They should be run by t9004 (and if I replace them with a `die`, they\n> clearly are). Are you sure your coverage script is not mistaken?\n\nIt looks like I'm missing the 9000+ tests. The following line was in the script\nI adapted from another CI job:\n\n\trm -f t/t9*.sh\n\nThis was probably because the job I adapted from needed to run quickly, but for\nthis coverage report we should do the hard work of running whatever t9*.sh tests\nwe can.\n\nSorry for the noise here, and thanks for checking!\n\n-Stolee\n"},{"id":"378262","messageId":"2d4410a9-fd3e-8b9f-00b5-f8eba4d51b42@gmail.com","threadId":"51396","inReplyTo":"20190628093911.GA27329@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-28T12:35:28Z","receivedAt":"2019-06-28T12:35:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/28/2019 5:39 AM, Jeff King wrote:\n> Some tests need to create a string of commits. Doing this with\n> test_commit is very heavy-weight, as it needs at least one process per\n> commit (and in fact, uses several).\n> \n> For bulk creation, we can do much better by using fast-import, but it's\n> often a pain to generate the input. Let's provide a helper to do so.\n\nWhat a quick turnaround! I'm happy to nerd-snipe you here, and am glad\nthe result was so positive.\n\n> We'll use t5310 as a guinea pig, as it has three 10-commit loops. Here\n> are hyperfine results before and after:\n> \n>   [before]\n>   Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n>     Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]\n>     Range (min … max):    2.250 s …  3.210 s    10 runs\n> \n>   [after]\n>   Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n>     Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]\n>     Range (min … max):    1.999 s …  2.590 s    10 runs\n\nI ran these tests on my Windows machine, where the process startup time is\na higher cost. The improvement is noticeable from just watching the test lines\npause on the steps creating the commits.\n\n Before: 30.8-31.2s\n  After: 23.5-23.8s\n\n> So we're over 20% faster, while making the callers slightly shorter.\n\nI see about the same relative measurement (~23%). The callers are a bit\ncleaner, which is good. They are also slightly less clear of what's\nhappening, but that's the cost of abstraction. Definitely worth it in\nthis case!\n \n> +# Similar to test_commit, but efficiently create <nr> commits, each with a\n> +# unique number $n (from 1 to <nr> by default) in the commit message.\n> +#\n> +# Usage: test_commit_bulk [options] <nr>\n> +#   -C <dir>:\n> +#\tRun all git commands in directory <dir>\n> +#   --ref=<n>:\n> +#\tref on which to create commits (default: HEAD)\n> +#   --start=<n>:\n> +#\tnumber commit messages from <n> (default: 1)\n> +#   --message=<msg>:\n> +#\tuse <msg> as the commit mesasge (default: \"commit $n\")\n> +#   --filename=<fn>:\n> +#\tmodify <fn> in each commit (default: $n.t)\n> +#   --contents=<string>:\n> +#\tplace <string> in each file (default: \"content $n\")\n> +#   --id=<string>:\n> +#\tshorthand to use <string> and $n in message, filename, and contents\n> +#\n> +# The message, filename, and contents strings are evaluated by the shell inside\n> +# double-quotes, with $n set to the current commit number. So you can do:\n> +#\n> +#   test_commit_bulk --filename=file --contents='modification $n'\n> +#\n> +# to have every commit touch the same file, but with unique content. Spaces are\n> +# OK, but you must escape any metacharacters (like backslashes or\n> +# double-quotes) you do not want expanded.\n> +#\n\nI appreciate all the documentation here!\n\n> +\t\twhile test \"$total\" -gt 0\n> +\t\tdo\n> +\t\t\techo \"commit $ref\" &&\n> +\t\t\tprintf 'author %s <%s> %s\\n' \\\n> +\t\t\t\t\"$GIT_AUTHOR_NAME\" \\\n> +\t\t\t\t\"$GIT_AUTHOR_EMAIL\" \\\n> +\t\t\t\t\"$cur_time -0700\" &&\n> +\t\t\tprintf 'committer %s <%s> %s\\n' \\\n> +\t\t\t\t\"$GIT_COMMITTER_NAME\" \\\n> +\t\t\t\t\"$GIT_COMMITTER_EMAIL\" \\\n> +\t\t\t\t\"$cur_time -0700\" &&\n> +\t\t\techo \"data <<EOF\" &&\n> +\t\t\teval \"echo \\\"$message\\\"\" &&\n> +\t\t\techo \"EOF\" &&\n> +\t\t\teval \"echo \\\"M 644 inline $filename\\\"\" &&\n> +\t\t\techo \"data <<EOF\" &&\n> +\t\t\teval \"echo \\\"$contents\\\"\" &&\n> +\t\t\techo \"EOF\" &&\n> +\t\t\techo &&\n> +\t\t\tn=$((n + 1)) &&\n> +\t\t\tcur_time=$((cur_time + 1)) &&\n> +\t\t\ttotal=$((total - 1)) ||\n> +\t\t\techo \"poison fast-import stream\"\n> +\t\tdone\n\nI am not very good at the nitty-gritty details of our scripts, but\nlooking at this I wonder if there is a cleaner and possibly faster\nway to do this loop. The top thing on my mind are the 'eval \"echo X\"'\nlines. If they start processes, then we can improve the performance.\nIf not, then it may not be worth it.\n\nIn wonder if instead we could create some format string outside the\nloop and then pass the values that change between iterations into\nthat format string.\n\nThanks,\n-Stolee\n\n"},{"id":"378263","messageId":"e9c14b58-2704-4750-9c37-7a7b15e596dc@gmail.com","threadId":"51396","inReplyTo":"CACsJy8C7Qr3JauRqUkB957ZDzVVZ0+wofzs7N2NzCbu+e00JnQ@mail.gmail.com","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-28T12:39:48Z","receivedAt":"2019-06-28T12:39:51Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/28/2019 5:47 AM, Duy Nguyen wrote:\n> On Fri, Jun 28, 2019 at 12:35 AM Derrick Stolee <stolee@gmail.com> wrote:\n>>> dir.c\n>>> 3b2385cf 2840) static void jw_object_untracked_cache_dir(struct json_writer *jw,\n>>> 3b2385cf 2845) jw_object_bool(jw, \"valid\", ucd->valid);\n>>> 3b2385cf 2846) jw_object_bool(jw, \"check-only\", ucd->check_only);\n>>> 3b2385cf 2847) jw_object_stat_data(jw, \"stat\", &ucd->stat_data);\n>>> 3b2385cf 2848) jw_object_string(jw, \"exclude-oid\", oid_to_hex(&ucd->exclude_oid));\n>>> 3b2385cf 2849) jw_object_inline_begin_array(jw, \"untracked\");\n>>> 3b2385cf 2850) for (i = 0; i < ucd->untracked_nr; i++)\n>>> 3b2385cf 2851) jw_array_string(jw, ucd->untracked[i]);\n>>> 3b2385cf 2852) jw_end(jw);\n>>> 3b2385cf 2854) jw_object_inline_begin_object(jw, \"dirs\");\n>>> 3b2385cf 2855) for (i = 0; i < ucd->dirs_nr; i++) {\n>>> 3b2385cf 2856) jw_object_inline_begin_object(jw, ucd->dirs[i]->name);\n>>> 3b2385cf 2857) jw_object_untracked_cache_dir(jw, ucd->dirs[i]);\n>>> 3b2385cf 2858) jw_end(jw);\n>>> 3b2385cf 2860) jw_end(jw);\n>>> 3b2385cf 2861) }\n>>> 3b2385cf 2958) jw_object_inline_begin_object(jw, \"root\");\n>>> 3b2385cf 2959) jw_object_untracked_cache_dir(jw, uc->root);\n>>> 3b2385cf 2960) jw_end(jw);\n>>\n>> Duy: I know you were working on some tests for these options. This is specifically\n>> in the \"untracked cache\" mode, so enabling the cache with at least one entry and\n>> running --debug-json should be sufficient.\n> \n> It's a bit more complicated than that, but I see your point.\n\nIt usually is. I don't mean to underestimate the effort here.\n\n> I initially looked at the output and saw \"something\" and moved on. I\n> should have examined the json output more carefully.\n\nThanks for taking a second look!\n\n>>> read-cache.c\n>>> 8eeabe15 1752) ret = error(_(\"index uses %.4s extension, which we do not understand\"),\n>>> ee70c128 1754) if (advice_unknown_index_extension) {\n>>> ee70c128 1755) warning(_(\"ignoring optional %.4s index extension\"), ext);\n>>> ee70c128 1756) advise(_(\"This is likely due to the file having been written by a newer\\n\"\n>>> 272b3f2a 2026) jw_object_true(jw, \"assume_unchanged\");\n>>> 272b3f2a 2030) jw_object_true(jw, \"skip_worktree\");\n>>> 272b3f2a 2032) jw_object_intmax(jw, \"stage\", ce_stage(ce));\n>>> f0f544da 2309) ieot = read_ieot_extension(istate, mmap, mmap_size, extension_offset);\n>>> f0f544da 3651) static struct index_entry_offset_table *read_ieot_extension(\n>>> f0f544da 3673) return do_read_ieot_extension(istate, index, extsize);\n>>\n>> Duy: more JSON output cases that could be interesting to cover.\n> \n> This is because I changed the function signature, I think. Both IEOT\n> and EOIE extensions, if I'm not mistaken, are never tested in the test\n> suite. You need to set GIT_TEST_INDEX_THREADS, then the last three\n> lines should be covered.\n\nThanks! Unfortunately, the threading is removed at compile-time in\norder to prevent race conditions with the gcov output. This means\nthe report will never report the threading code as covered. :(\n\nDoes that same reasoning apply to the assume_unchanged, skip_worktree,\nand \"stage\" lines?\n\n-Stolee\n\n"},{"id":"378267","messageId":"nycvar.QRO.7.76.6.1906281452251.44@tvgsbejvaqbjf.bet","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] easy bulk commit creation in tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-28T12:53:26Z","receivedAt":"2019-06-28T12:53:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 28 Jun 2019, Jeff King wrote:\n\n> On Fri, Jun 28, 2019 at 02:41:03AM -0400, Jeff King wrote:\n>\n> > It would be nice if we had a \"test_commits_bulk\" that used fast-import\n> > to create larger numbers of commits.\n>\n> So here's a patch to do that.\n\nI like the direction, especially because it would make it super easy to go\none step further that would probably make a huge difference on Windows: to\nmove `test_commit_bulk` to `test-tool commit-bulk`.\n\nThanks,\nDscho\n"},{"id":"378268","messageId":"CAP8UFD3VFdCUwDBTb9en22FO7HnWc4vgQ4h0hhariCB=om4b8A@mail.gmail.com","threadId":"51396","inReplyTo":"14689d27-eecd-2e0a-715d-796b20d573e5@gmail.com","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-28T13:39:03Z","receivedAt":"2019-06-28T13:39:17Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jun 27, 2019 at 7:35 PM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> Here are some interesting sections I found when examining the test coverage\n> report. I am only highlighting these sections because they seem to include\n> non-trivial logic. In some cases, maybe the code isn't needed.\n>\n> On 6/27/2019 1:05 PM, Derrick Stolee wrote:\n> > promisor-remote.c\n> > db27dca5 25) die(_(\"Remote with no URL\"));\n> > 48de3158 61) warning(_(\"promisor remote name cannot begin with '/': %s\"),\n> > 48de3158 63) return NULL;\n> > faf2abf4 93) previous->next = r->next;\n> > 4ca9474e 108) return git_config_string(&core_partial_clone_filter_default,\n> > fa3d1b63 139) return 0;\n> > 9e27beaa 202) static int remove_fetched_oids(struct repository *repo,\n> > 9e27beaa 206) int i, remaining_nr = 0;\n> > 9e27beaa 207) int *remaining = xcalloc(oid_nr, sizeof(*remaining));\n> > 9e27beaa 208) struct object_id *old_oids = *oids;\n> > 9e27beaa 211) for (i = 0; i < oid_nr; i++)\n> > 9e27beaa 212) if (oid_object_info_extended(repo, &old_oids[i], NULL,\n> > 9e27beaa 214) remaining[i] = 1;\n> > 9e27beaa 215) remaining_nr++;\n> > 9e27beaa 218) if (remaining_nr) {\n> > 9e27beaa 219) int j = 0;\n> > 9e27beaa 220) new_oids = xcalloc(remaining_nr, sizeof(*new_oids));\n> > 9e27beaa 221) for (i = 0; i < oid_nr; i++)\n> > 9e27beaa 222) if (remaining[i])\n> > 9e27beaa 223) oidcpy(&new_oids[j++], &old_oids[i]);\n> > 9e27beaa 224) *oids = new_oids;\n> > 9e27beaa 225) if (to_free)\n> > 9e27beaa 226) free(old_oids);\n> > 9e27beaa 229) free(remaining);\n> > 9e27beaa 231) return remaining_nr;\n> > 9e27beaa 248) if (remaining_nr == 1)\n> > 9e27beaa 249) continue;\n> > 9e27beaa 250) remaining_nr = remove_fetched_oids(repo, &remaining_oids,\n> > 9e27beaa 252) if (remaining_nr) {\n> > 9e27beaa 253) to_free = 1;\n> > 9e27beaa 254) continue;\n> > 9e27beaa 262) free(remaining_oids);\n>\n> Christian: this section continues to be untested, but I think you were\n> working on creating tests for this.\n\nYeah, I am planning to work on this soon.\n\n> > t/helper/test-oidmap.c\n> > 11510dec 52) if (get_oid(p1, &oid)) {\n> > 11510dec 53) printf(\"Unknown oid: %s\\n\", p1);\n> > 11510dec 54) continue;\n> > 11510dec 58) FLEX_ALLOC_STR(entry, name, p2);\n> > 11510dec 59) oidcpy(&entry->entry.oid, &oid);\n> > 11510dec 62) oidmap_put(&map, entry);\n>\n> Christian: this block looks like the test-oidmap helper never uses the \"add\"\n> subcommand. Is that correct?\n\nYeah, I initially copied it from hashmap, but then I realized that it\nwas nearly identical as the \"put\" subcommand, so not worth testing\nseparately. I should have removed it and will do it soon.\n\n> > 11510dec 97) if (get_oid(p1, &oid)) {\n> > 11510dec 98) printf(\"Unknown oid: %s\\n\", p1);\n> > 11510dec 99) continue;\n> > 11510dec 103) entry = oidmap_remove(&map, &oid);\n> > 11510dec 106) puts(entry ? entry->name : \"NULL\");\n> > 11510dec 107) free(entry);\n>\n> Similarly, this block means we are not using the \"remove\" subcommand.\n\nYeah, it looks like I forgot to implement a test for that subcommand.\nWill add it soon.\n\nThanks,\nChristian.\n"},{"id":"378278","messageId":"xmqqlfxl8ul6.fsf@gitster-ct.c.googlers.com","threadId":"51396","inReplyTo":"20190628093911.GA27329@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-28T17:53:41Z","receivedAt":"2019-06-28T17:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +# The message, filename, and contents strings are evaluated by the shell inside\n> +# double-quotes, with $n set to the current commit number. So you can do:\n> +#\n> +#   test_commit_bulk --filename=file --contents='modification $n'\n> +#\n> +# to have every commit touch the same file, but with unique content. Spaces are\n> +# OK, but you must escape any metacharacters (like backslashes or\n> +# double-quotes) you do not want expanded.\n\nNice.\n\n> +test_commit_bulk () {\n> +\tindir=\n> + ...\n> +\twhile test $# -gt 0\n> +\tdo\n> +\t\tcase \"$1\" in\n> +\t\t-C)\n> +\t\t\tindir=$2\n> +\t\t\tshift\n> +\t\t\t;;\n> + ...\n> +\t\tesac\n> +\t\tshift\n> +\tdone\n> +\ttotal=$1\n> +\n> +\tin_dir=${indir:+-C \"$indir\"}\n\nI thought that this assignment to $in_dir would be unnecessary if we\nparsed -C directly into it, i.e.\n\n\t\t...\n\t\t-C)\n\t\t\tin_dir=\"-C $indir\"\n\t\t\tshift\n\t\t\t;;\n\t\t...\n\nbut you probably could pass -C '' to defeat an $in_dir that was set\nearlier by using a separate variable?\n\nMessages and other stuff are made `eval`-safe, but this one does not\ncare much about quoting, which made me curious.\n\nReading further, though, I do not seem to see where this variable is\nreferred to, and that is the answer to my puzzlement.  This must be\na leftover that was written once before but no longer is used.  We\ncan remove $in_dir while keeping the initialization and assignment\nto $indir as-is, I think.\n\nAll uses of $indir in the remainder of the function look $IFS-safe,\nwhich is good.\n"},{"id":"378279","messageId":"xmqqh8898u28.fsf@gitster-ct.c.googlers.com","threadId":"51396","inReplyTo":"2d4410a9-fd3e-8b9f-00b5-f8eba4d51b42@gmail.com","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-28T18:05:03Z","receivedAt":"2019-06-28T18:05:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> way to do this loop. The top thing on my mind are the 'eval \"echo X\"'\n> lines. If they start processes, then we can improve the performance.\n> If not, then it may not be worth it.\n\nSigh.  \n\nDo you mean 'echo' run inside 'eval' is one extra process?  In most\nmodern shells, it is a built-in and you need another process.\n\nDo you mean 'eval' running anything is one extra process?  Because\nanything done inside eval must be visible to the shell running it,\ne.g.\n\n\tvar=myvar; eval \"$var=val\"\n\nwould evaluate string 'myvar=val' inside that shell itself and it\nmust be able to update the value of $myvar, whatever it does must\nnot add any extra process.\n\nThe primary reason why the loop in question uses eval is to allow\nthe callers to pass $n in single-quote to have it interpolated\nlazily.\n\n\tmessage='message $n'\n\tfor n in 1 2 3\n\tdo\n\t\techo \"$message\"\n\t\teval \"echo \\\"$message\\\"\"\n\tdone\n\nEach iteration, the first line gives\n\n\tmessage $n\n\nwhich is the thing that gets passed to 'echo' in the second line, so\nyou'll see\n\n\tmessage $n\n\tmessage 1\n\tmessage $n\n\tmessage 2\n\tmessage $n\n\tmessage 3\n\nas the result.\n"},{"id":"378280","messageId":"87woh57dnt.fsf@evledraar.gmail.com","threadId":"51396","inReplyTo":"20190628093911.GA27329@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-06-28T18:44:38Z","receivedAt":"2019-06-28T18:44:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 28 2019, Jeff King wrote:\n\n> Some tests need to create a string of commits. Doing this with\n> test_commit is very heavy-weight, as it needs at least one process per\n> commit (and in fact, uses several).\n>\n> For bulk creation, we can do much better by using fast-import, but it's\n> often a pain to generate the input. Let's provide a helper to do so.\n>\n> We'll use t5310 as a guinea pig, as it has three 10-commit loops. Here\n> are hyperfine results before and after:\n>\n>   [before]\n>   Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n>     Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]\n>     Range (min … max):    2.250 s …  3.210 s    10 runs\n>\n>   [after]\n>   Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n>     Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]\n>     Range (min … max):    1.999 s …  2.590 s    10 runs\n>\n> So we're over 20% faster, while making the callers slightly shorter. We\n> added a lot more lines in test-lib-function.sh, of course, and the\n> helper is way more featureful than we need here. But my hope is that it\n> will be flexible enough to use in more places.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5310-pack-bitmaps.sh |  15 +----\n>  t/test-lib-functions.sh | 131 ++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 134 insertions(+), 12 deletions(-)\n>\n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index a26c8ba9a2..3aab7024ca 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -21,15 +21,9 @@ has_any () {\n>  }\n>\n>  test_expect_success 'setup repo with moderate-sized history' '\n> -\tfor i in $(test_seq 1 10)\n> -\tdo\n> -\t\ttest_commit $i\n> -\tdone &&\n> +\ttest_commit_bulk --id=file 10 &&\n>  \tgit checkout -b other HEAD~5 &&\n> -\tfor i in $(test_seq 1 10)\n> -\tdo\n> -\t\ttest_commit side-$i\n> -\tdone &&\n> +\ttest_commit_bulk --id=side 10 &&\n>  \tgit checkout master &&\n>  \tbitmaptip=$(git rev-parse master) &&\n>  \tblob=$(echo tagged-blob | git hash-object -w --stdin) &&\n> @@ -106,10 +100,7 @@ test_expect_success 'clone from bitmapped repository' '\n>  '\n>\n>  test_expect_success 'setup further non-bitmapped commits' '\n> -\tfor i in $(test_seq 1 10)\n> -\tdo\n> -\t\ttest_commit further-$i\n> -\tdone\n> +\ttest_commit_bulk --id=further 10\n>  '\n>\n>  rev_list_tests 'partial bitmap'\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 0367cec5fd..32a1db81a3 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -233,6 +233,137 @@ test_merge () {\n>  \tgit tag \"$1\"\n>  }\n>\n> +# Similar to test_commit, but efficiently create <nr> commits, each with a\n> +# unique number $n (from 1 to <nr> by default) in the commit message.\n\nIs it intentional not to follow test_commit's convention of creating a\ntag as well? If so it would be helpful to note that difference here, or\nrather, move this documentation to t/README where test_commit and\nfriends are documented.\n"},{"id":"378281","messageId":"87v9wp7dfp.fsf@evledraar.gmail.com","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] easy bulk commit creation in tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-06-28T18:49:30Z","receivedAt":"2019-06-28T18:49:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 28 2019, Jeff King wrote:\n\n> On Fri, Jun 28, 2019 at 02:41:03AM -0400, Jeff King wrote:\n>\n>> I think this would exercise it, at the cost of making the test more\n>> expensive:\n>>\n>> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n>> index 82d7f7f6a5..8ed6982dcb 100755\n>> --- a/t/t5310-pack-bitmaps.sh\n>> +++ b/t/t5310-pack-bitmaps.sh\n>> @@ -21,7 +21,7 @@ has_any () {\n>>  }\n>>\n>>  test_expect_success 'setup repo with moderate-sized history' '\n>> -\tfor i in $(test_seq 1 10)\n>> +\tfor i in $(test_seq 1 100)\n>>  \tdo\n>>  \t\ttest_commit $i\n>>  \tdone &&\n>>\n>> It would be nice if we had a \"test_commits_bulk\" that used fast-import\n>> to create larger numbers of commits.\n>\n> So here's a patch to do that. Writing the bulk commit function was a fun\n> exercise, and I found a couple other places to apply it, too, shaving\n> off ~7.5 seconds from my test runs. Not ground-breaking, but I think\n> it's nice to have a solution where we don't have to be afraid to\n> generate a bunch of commits.\n\nNice.\n\nJust a side-note: I've wondered how much we could speed up the tests in\nother places if rather than doing setup all over the place we simply\ncreated a few \"template\" repository shapes, and the common case for\ntests would be to simply cp(1) those over.\n\nI.e. for things like fsck etc. we really do need some specific\nrepository layout, but a lot of our tests are simply re-doing setup\nslightly differently just to get things like \"I want a few commits on a\nfew branches\" or \"set up a repo like <that> but with some remotes\" etc.\n"},{"id":"378286","messageId":"CAPig+cQAKVOqYH-CcqzViP_zuKwg5fWO2_i8=Z5t=BVf9uoAXQ@mail.gmail.com","threadId":"51396","inReplyTo":"20190628093911.GA27329@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-06-28T21:32:35Z","receivedAt":"2019-06-28T21:32:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:\n> [...]\n> For bulk creation, we can do much better by using fast-import, but it's\n> often a pain to generate the input. Let's provide a helper to do so.\n> [...]\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> @@ -233,6 +233,137 @@ test_merge () {\n> +# Similar to test_commit, but efficiently create <nr> commits, each with a\n> +# unique number $n (from 1 to <nr> by default) in the commit message.\n> +#\n> +# Usage: test_commit_bulk [options] <nr>\n> +#   [...]\n> +#\n> +# The message, filename, and contents strings are evaluated by the shell inside\n> +# double-quotes, with $n set to the current commit number. So you can do:\n> +#\n> +#   test_commit_bulk --filename=file --contents='modification $n'\n\nConsidering that test_commit_bulk() is intended to be used within a\ntest body, and considering that test bodies are almost always\nencapsulated in single quotes, recommending single quoting the value\nof --contents= seems contraindicated. Double quotes likely would be\nbetter.\n\n> +# to have every commit touch the same file, but with unique content. Spaces are\n> +# OK, but you must escape any metacharacters (like backslashes or\n> +# double-quotes) you do not want expanded.\n> +#\n> +test_commit_bulk () {\n> +       [...]\n> +       in_dir=${indir:+-C \"$indir\"}\n\nDoesn't this suffer the problem in which some older/broken\nshells[1][2][3][4] incorrectly expand this to:\n\n    \"-C <dir>\"\n\nrather than the expected:\n\n    -C \"<dir>\"\n\n? Is this something we still care about?\n\nSame comment applies to other instances of ${indir:+-C \"$indir\"} below.\n\n[1]: http://public-inbox.org/git/20160517215214.GA16905@sigill.intra.peff.net/\n[2]: http://public-inbox.org/git/e3bfc53363b14826d828e1adffbbeea@74d39fa044aa309eaea14b9f57fe79c/\n[3]: http://public-inbox.org/git/20160518010609.Horde.sM8QUFek6WMAAwho56DDob8@webmail.informatik.kit.edu/\n[4]: http://public-inbox.org/git/1240044459-57227-1-git-send-email-ben@ben.com/\n"},{"id":"378301","messageId":"20190628230417.GC21574@szeder.dev","threadId":"51396","inReplyTo":"CAPig+cQAKVOqYH-CcqzViP_zuKwg5fWO2_i8=Z5t=BVf9uoAXQ@mail.gmail.com","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-28T23:04:17Z","receivedAt":"2019-06-28T23:04:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:\n> On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:\n\n> > +# to have every commit touch the same file, but with unique content. Spaces are\n> > +# OK, but you must escape any metacharacters (like backslashes or\n> > +# double-quotes) you do not want expanded.\n> > +#\n> > +test_commit_bulk () {\n> > +       [...]\n> > +       in_dir=${indir:+-C \"$indir\"}\n> \n> Doesn't this suffer the problem in which some older/broken\n> shells[1][2][3][4] incorrectly expand this to:\n> \n>     \"-C <dir>\"\n> \n> rather than the expected:\n> \n>     -C \"<dir>\"\n> \n> ? Is this something we still care about?\n> \n> Same comment applies to other instances of ${indir:+-C \"$indir\"} below.\n\nI think we don't need any of those \"${indir:+-C \"$indir\"}\" parameter\nexpansions and could simply use 'git -C \"$indir\" cmd...' everywhere.\n$indir is set to empty right at the start of the function, and 'git -C\n\"\" ...' works and doesn't change the working directory.\n\n"},{"id":"378302","messageId":"CAPig+cQnvotvJakCp1ic_WP3AMojNmfZw-PoinR=VOODmtNBtw@mail.gmail.com","threadId":"51396","inReplyTo":"20190628230417.GC21574@szeder.dev","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-06-28T23:46:02Z","receivedAt":"2019-06-28T23:47:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 28, 2019 at 7:04 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:\n> > On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:\n> > > +       in_dir=${indir:+-C \"$indir\"}\n> >\n> > Doesn't this suffer the problem in which some older/broken\n> > shells[1][2][3][4] incorrectly [...]\n>\n> I think we don't need any of those \"${indir:+-C \"$indir\"}\" parameter\n> expansions and could simply use 'git -C \"$indir\" cmd...' everywhere.\n> $indir is set to empty right at the start of the function, and 'git -C\n> \"\" ...' works and doesn't change the working directory.\n\nI recall the discussion around the meaning of `-C \"\"` when that\ncommand line option was introduced. The conclusion was that  the\nzero-length argument should mean \"this directory\" since that's how `cd\n\"\"` behaves. However, I don't think that behavior ever got documented,\nand it's not necessarily obvious. An alternative would be to default\n'indir' to \".\", which should give the same result and be easily\nunderstood.\n"},{"id":"378305","messageId":"20190628235903.GB2625@sigill.intra.peff.net","threadId":"51396","inReplyTo":"ddf13a6b-42f4-57ae-49f1-8cc758b1dc4b@gmail.com","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T23:59:03Z","receivedAt":"2019-06-28T23:59:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 08:23:49AM -0400, Derrick Stolee wrote:\n\n> On 6/28/2019 2:45 AM, Jeff King wrote:\n> > On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:\n> > \n> >>> t/helper/test-example-decorate.c\n> >>> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);\n> >>> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);\n> >>> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);\n> >>\n> >> Peff: again interesting that these lines you refactored were not covered, especially\n> >> because they are part of a test helper. Perhaps the tests they were intended for are\n> >> now defunct?\n> > \n> > They should be run by t9004 (and if I replace them with a `die`, they\n> > clearly are). Are you sure your coverage script is not mistaken?\n> \n> It looks like I'm missing the 9000+ tests. The following line was in the script\n> I adapted from another CI job:\n> \n> \trm -f t/t9*.sh\n> \n> This was probably because the job I adapted from needed to run quickly, but for\n> this coverage report we should do the hard work of running whatever t9*.sh tests\n> we can.\n\nI suspect most of those _are_ low-value. The git-p4 tests, for instance,\nare mostly exercising the p4 script and not our C code, and the same\nwith git-svn. However I wouldn't be surprised if there are a few dusty\ncorners they manage to hit that aren't covered elsewhere.\n\nStill, if it's not too painful to add them in time-wise, it probably\nmakes sense for the coverage tests to be as exhaustive as possible.\n\n-Peff\n"},{"id":"378306","messageId":"20190629000942.GC2625@sigill.intra.peff.net","threadId":"51396","inReplyTo":"2d4410a9-fd3e-8b9f-00b5-f8eba4d51b42@gmail.com","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T00:09:43Z","receivedAt":"2019-06-29T00:09:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 08:35:28AM -0400, Derrick Stolee wrote:\n\n> > +\t\twhile test \"$total\" -gt 0\n> > +\t\tdo\n> > +\t\t\techo \"commit $ref\" &&\n> > +\t\t\tprintf 'author %s <%s> %s\\n' \\\n> > +\t\t\t\t\"$GIT_AUTHOR_NAME\" \\\n> > +\t\t\t\t\"$GIT_AUTHOR_EMAIL\" \\\n> > +\t\t\t\t\"$cur_time -0700\" &&\n> > +\t\t\tprintf 'committer %s <%s> %s\\n' \\\n> > +\t\t\t\t\"$GIT_COMMITTER_NAME\" \\\n> > +\t\t\t\t\"$GIT_COMMITTER_EMAIL\" \\\n> > +\t\t\t\t\"$cur_time -0700\" &&\n> > +\t\t\techo \"data <<EOF\" &&\n> > +\t\t\teval \"echo \\\"$message\\\"\" &&\n> > +\t\t\techo \"EOF\" &&\n> > +\t\t\teval \"echo \\\"M 644 inline $filename\\\"\" &&\n> > +\t\t\techo \"data <<EOF\" &&\n> > +\t\t\teval \"echo \\\"$contents\\\"\" &&\n> > +\t\t\techo \"EOF\" &&\n> > +\t\t\techo &&\n> > +\t\t\tn=$((n + 1)) &&\n> > +\t\t\tcur_time=$((cur_time + 1)) &&\n> > +\t\t\ttotal=$((total - 1)) ||\n> > +\t\t\techo \"poison fast-import stream\"\n> > +\t\tdone\n> \n> I am not very good at the nitty-gritty details of our scripts, but\n> looking at this I wonder if there is a cleaner and possibly faster\n> way to do this loop. The top thing on my mind are the 'eval \"echo X\"'\n> lines. If they start processes, then we can improve the performance.\n> If not, then it may not be worth it.\n\nNo, evals by themselves don't require a process.  That whole loop should\nall happen as a single process (because it's the left-hand side of the\npipe, it does require a subshell).\n\nWe could drop even that process by writing into a temporary file. The\nsize probably wouldn't be a big deal, and I doubt the latency would even\nmatter much (and anyway, when you're running the tests in parallel\nanyway, CPU time is the most important metric).\n\nIt might also make the code a little simpler, since we'd be running in\nthe main shell and could just use test_tick naturally (rather than the\nmanual addition hackery).\n\nI'll take a look.\n\nI wasn't super concerned with eliminating processes here as long as the\nnumber of them is constant with respect to the number of commits we're\ngenerating. The big improvement is taking, say, 300 test_commit calls\nand turning it into a single bulk call. Replacing a single-commit\ntest_commit with this would be break-even at best.\n\n> In wonder if instead we could create some format string outside the\n> loop and then pass the values that change between iterations into\n> that format string.\n\nThe evals should be fast. But they are potentially error-prone, since\ncallers have to pass something like --message='commit $n' with single\nquotes to keep the \"$\" intact. But because all of our test snippets are\ninside single-quotes already, you end up with:\n\n  test_bulk_commit --message=\"commit \\$n\"\n\n(though in practice most of the callers used the --id shorthand, which\nneatly sidesteps this).\n\nSince there's literally only one variable to interpolate, we could swap\nthis out for using printf formatters, and letting \"%s\" mean the same as\n\"$n\". It should perform the same but is a bit less magical and a bit\nharder to screw up. It would also be easier to handle if\ntest_commit_bulk eventually became C code. The only downside I can think\nof is that you can't mention \"%s\" twice, but I find it hard to imagine a\ncaller would want that anyway.\n\nSo I'll also take a look at that.\n\n-Peff\n"},{"id":"378307","messageId":"20190629001457.GD2625@sigill.intra.peff.net","threadId":"51396","inReplyTo":"xmqqlfxl8ul6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T00:14:57Z","receivedAt":"2019-06-29T00:15:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 10:53:41AM -0700, Junio C Hamano wrote:\n\n> > +\tin_dir=${indir:+-C \"$indir\"}\n> \n> I thought that this assignment to $in_dir would be unnecessary if we\n> parsed -C directly into it, i.e.\n\nHeh, sorry for the confusion. That in_dir is leftover cruft. I was\ntrying to see if I could then expand it as:\n\n  git $in_dir some-cmd ...\n\nto make the git calls more readable. But that doesn't work if $indir has\nwhitespace, so I abandoned it (we're relying on whitespace splitting\nbetween \"-C\" and the argument, but we don't want it split on the\nargument).\n\nI _also_ mispelled $indir as $in_dir in that attempt, which meant that\nthe leftover line did not break anything, and I didn't notice. But it\ncan just go away.\n\n> \t\t-C)\n> \t\t\tin_dir=\"-C $indir\"\n> \t\t\tshift\n> \t\t\t;;\n> \t\t...\n> \n> but you probably could pass -C '' to defeat an $in_dir that was set\n> earlier by using a separate variable?\n\nI don't know if \"-C ''\" works with Git or not. I had contemplated\ndefaulting indir to \".\", so that we did:\n\n  git -C . command ...\n\nwhich I think would work (at the minor cost of a useless chdir() inside\nthe C process).\n\nIn the end I just stole the technique that test_commit uses. It's a\nlittle ugly, but there are only 3 calls.\n\n> Reading further, though, I do not seem to see where this variable is\n> referred to, and that is the answer to my puzzlement.  This must be\n> a leftover that was written once before but no longer is used.  We\n> can remove $in_dir while keeping the initialization and assignment\n> to $indir as-is, I think.\n\nYes. :)\n\n> All uses of $indir in the remainder of the function look $IFS-safe,\n> which is good.\n\nYeah, I think it should be (though since most callers pass relative\npaths for these kind of one-off -C uses, it's actually pretty rare for\nit to matter).\n\n-Peff\n"},{"id":"378308","messageId":"20190629001935.GE2625@sigill.intra.peff.net","threadId":"51396","inReplyTo":"87woh57dnt.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T00:19:35Z","receivedAt":"2019-06-29T00:19:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 08:44:38PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > +# Similar to test_commit, but efficiently create <nr> commits, each with a\n> > +# unique number $n (from 1 to <nr> by default) in the commit message.\n> \n> Is it intentional not to follow test_commit's convention of creating a\n> tag as well? If so it would be helpful to note that difference here, or\n\nYes, it was intentional. I have long hated that feature, as there are\nmany tests have to bend over backwards to deal with the reachability\nimplications of adding the extra tag (not to mention the waste of a\nprocess). Likewise, I have long hated the implicit-argument-ordering of\ntest_commit that make it hard to set some optional arguments but not\nothers (hence the double-dash parameters).\n\nI had planned to add a \"--tag\" parameter if anybody ever wanted one. But\nwe can call out that difference explicitly. Or alternatively, stop\nsaying \"like test_commit\" and just say \"Efficiently create <nr>\ncommits\".\n\n> rather, move this documentation to t/README where test_commit and\n> friends are documented.\n\nUgh. I had no idea that documentation even existed. Because of course\ntest_commit _is_ documented next to its definition, and that\ndocumentation has been kept up to date, unlike the far-away stale bits\nin t/README.\n\n-Peff\n"},{"id":"378309","messageId":"20190629002542.GF2625@sigill.intra.peff.net","threadId":"51396","inReplyTo":"CAPig+cQAKVOqYH-CcqzViP_zuKwg5fWO2_i8=Z5t=BVf9uoAXQ@mail.gmail.com","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T00:25:42Z","receivedAt":"2019-06-29T00:25:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:\n\n> > +# The message, filename, and contents strings are evaluated by the shell inside\n> > +# double-quotes, with $n set to the current commit number. So you can do:\n> > +#\n> > +#   test_commit_bulk --filename=file --contents='modification $n'\n> \n> Considering that test_commit_bulk() is intended to be used within a\n> test body, and considering that test bodies are almost always\n> encapsulated in single quotes, recommending single quoting the value\n> of --contents= seems contraindicated. Double quotes likely would be\n> better.\n\nI hoped people reading it would realize that they would need to suppress\ninterpolation of $ one way or another. But I guess many people who touch\nthe test suite aren't actually prolific shell writers.\n\nAnyway, I'm going to look into changing this to a printf string, which\nwould make this easier.\n\n> > +       in_dir=${indir:+-C \"$indir\"}\n> \n> Doesn't this suffer the problem in which some older/broken\n> shells[1][2][3][4] incorrectly expand this to:\n\nThat line is leftover dead code; see my response to Junio.\n\n> Same comment applies to other instances of ${indir:+-C \"$indir\"} below.\n\nThose ones are fine because of the double-quotes (and whitespace\nsplitting on the replacement value happens before interpolation). Try\nthis:\n\n  x=''\n  y='with spaces'\n  sh -c 'for i in \"$@\"; do echo arg: $i; done' -- \\\n    ${x:+-x \"$x\"} \\\n    ${y:+-y \"$y\"}\n\n-Peff\n"},{"id":"378310","messageId":"20190629002617.GA3094@sigill.intra.peff.net","threadId":"51396","inReplyTo":"CAPig+cQnvotvJakCp1ic_WP3AMojNmfZw-PoinR=VOODmtNBtw@mail.gmail.com","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T00:26:17Z","receivedAt":"2019-06-29T00:26:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 07:46:02PM -0400, Eric Sunshine wrote:\n\n> On Fri, Jun 28, 2019 at 7:04 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> > On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:\n> > > On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:\n> > > > +       in_dir=${indir:+-C \"$indir\"}\n> > >\n> > > Doesn't this suffer the problem in which some older/broken\n> > > shells[1][2][3][4] incorrectly [...]\n> >\n> > I think we don't need any of those \"${indir:+-C \"$indir\"}\" parameter\n> > expansions and could simply use 'git -C \"$indir\" cmd...' everywhere.\n> > $indir is set to empty right at the start of the function, and 'git -C\n> > \"\" ...' works and doesn't change the working directory.\n> \n> I recall the discussion around the meaning of `-C \"\"` when that\n> command line option was introduced. The conclusion was that  the\n> zero-length argument should mean \"this directory\" since that's how `cd\n> \"\"` behaves. However, I don't think that behavior ever got documented,\n> and it's not necessarily obvious. An alternative would be to default\n> 'indir' to \".\", which should give the same result and be easily\n> understood.\n\nYeah, I had considered that, too. I had mostly just copied the solution\nfrom test_commit, thinking nobody would nitpick it. ;) But if everybody\nlikes \".\", I think that is a bit more readable.\n\n-Peff\n"},{"id":"378311","messageId":"20190629003057.GB3094@sigill.intra.peff.net","threadId":"51396","inReplyTo":"nycvar.QRO.7.76.6.1906281452251.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/6] easy bulk commit creation in tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T00:30:57Z","receivedAt":"2019-06-29T00:31:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 02:53:26PM +0200, Johannes Schindelin wrote:\n\n> > > It would be nice if we had a \"test_commits_bulk\" that used fast-import\n> > > to create larger numbers of commits.\n> >\n> > So here's a patch to do that.\n> \n> I like the direction, especially because it would make it super easy to go\n> one step further that would probably make a huge difference on Windows: to\n> move `test_commit_bulk` to `test-tool commit-bulk`.\n\nI actually considered going directly there, but I don't think it would\nmake a big difference. In the biggest case we dropped 900 processes to\n4. If we really want to drop that to 1, we can:\n\n  - use a temp-file to avoid the left-hand-pipe subshell\n\n  - add a feature to fast-import to say \"build on top of ref X\", instead\n    of using to use rev-parse to manually generates a \"reset\" line\n    (maybe this is even possible already; I searched for it, but not\n    very hard).\n\n  - add a feature to fast-import to have it check out the result of HEAD\n    if it was updated\n\nThe first one seems like an easy and obvious win that I'll explore. The\nsecond one would be useful in general, I think, but I don't plan on\ndigging into it (unless somebody shows up with an easy existing way to\ndo it).\n\nThe third one is a little less elegant to me, because there are a lot of\nquestions about how to checkout (e.g., with \"-f\", what happens to\ndeleted files, etc).\n\n-Peff\n"},{"id":"378312","messageId":"20190629004559.GC3094@sigill.intra.peff.net","threadId":"51396","inReplyTo":"87v9wp7dfp.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/6] easy bulk commit creation in tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T00:45:59Z","receivedAt":"2019-06-29T00:46:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 08:49:30PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > So here's a patch to do that. Writing the bulk commit function was a fun\n> > exercise, and I found a couple other places to apply it, too, shaving\n> > off ~7.5 seconds from my test runs. Not ground-breaking, but I think\n> > it's nice to have a solution where we don't have to be afraid to\n> > generate a bunch of commits.\n> \n> Nice.\n> \n> Just a side-note: I've wondered how much we could speed up the tests in\n> other places if rather than doing setup all over the place we simply\n> created a few \"template\" repository shapes, and the common case for\n> tests would be to simply cp(1) those over.\n\nThat thought also occurred to me while writing this. I've worked with\ntest suites that have those kind of \"fixtures\" before, and I generally\nlike it less, for two reasons:\n\n  - it's much harder to understand what's important about the fixture,\n    because you're seeing the end result of running a bunch of commands.\n    Whereas the individual commands that show you _how_ it was derived\n    are generally instructive; they give you the steps that the author\n    was thinking about.\n\n  - it's more annoying to update them because you don't just change the\n    instructions. You have to extract the fixture into a real repo,\n    manipulate it, then convert it back into whatever storage format we\n    use (which can't just be a real repo, because we don't allow\n    embedding repos).\n\nBut what I think _would_ be cool is to treat the command instructions as\nthe source of truth, but allow caching of the on-disk state at certain\npoints in a test script. I.e., imagine that we annotate some test\nsnippets to say \"I am setup, and it's OK if you don't run me every\ntime\". Like:\n\n  test_expect_success SETUP 'a really slow setup step' '\n\tfor i in $(test_seq 1000)\n\tdo\n\t\ttest_commit horribly-slow-$i\n\tdone\n  '\n\nand then the test harness would recognize the SETUP prereq as magical,\nand:\n\n  - look for t/cache/t1234.42.tar; if it exists, then replace the whole\n    trash-dir state with it and skip the test\n\n  - otherwise run the snippet and create t1234.42.tar for next time\n\nBonus points if you could specifically ask to cache t1234.57, even if it\n_isn't_ marked as SETUP, and then restart the script from that point,\nskipping over the intermediate tests (whether they were cached or not).\nThat would let you then run subsequent tests from a known point\ninstantly (e.g., if you're debugging some later test in the script and\nwant to run it over and over).\n\nThe downsides I see are:\n\n  1. It doesn't exercise the setup snippets as much. The idea is that\n     this shouldn't matter if it's just setup code, but I'm sure we do\n     get some extra coverage from it. But any fixture-based scheme\n     suffers from this.\n\n  2. Cache invalidation (isn't it always?). If you changed the setup\n     test or even fixed a bug elsewhere in Git, you'd want to re-run the\n     setup steps. It's always OK to blow away the cache and get a fresh\n     run, but sometimes it's easy to forget to do so (and the results\n     can be confusing).\n\n-Peff\n"},{"id":"378313","messageId":"c2e0ceef-c14d-ea1d-7522-eab0bf2e48ce@gmail.com","threadId":"51396","inReplyTo":"20190628235903.GB2625@sigill.intra.peff.net","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-29T01:36:14Z","receivedAt":"2019-06-29T01:36:20Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/28/2019 7:59 PM, Jeff King wrote:\n> On Fri, Jun 28, 2019 at 08:23:49AM -0400, Derrick Stolee wrote:\n> \n>> On 6/28/2019 2:45 AM, Jeff King wrote:\n>>> On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:\n>>>\n>>>>> t/helper/test-example-decorate.c\n>>>>> 0ebbcf70 29) one = lookup_unknown_object(&one_oid);\n>>>>> 0ebbcf70 30) two = lookup_unknown_object(&two_oid);\n>>>>> 0ebbcf70 59) three = lookup_unknown_object(&three_oid);\n>>>>\n>>>> Peff: again interesting that these lines you refactored were not covered, especially\n>>>> because they are part of a test helper. Perhaps the tests they were intended for are\n>>>> now defunct?\n>>>\n>>> They should be run by t9004 (and if I replace them with a `die`, they\n>>> clearly are). Are you sure your coverage script is not mistaken?\n>>\n>> It looks like I'm missing the 9000+ tests. The following line was in the script\n>> I adapted from another CI job:\n>>\n>> \trm -f t/t9*.sh\n>>\n>> This was probably because the job I adapted from needed to run quickly, but for\n>> this coverage report we should do the hard work of running whatever t9*.sh tests\n>> we can.\n> \n> I suspect most of those _are_ low-value. The git-p4 tests, for instance,\n> are mostly exercising the p4 script and not our C code, and the same\n> with git-svn. However I wouldn't be surprised if there are a few dusty\n> corners they manage to hit that aren't covered elsewhere.\n> \n> Still, if it's not too painful to add them in time-wise, it probably\n> makes sense for the coverage tests to be as exhaustive as possible.\n\nUnfortunately, even running the t9*.sh tests once (among the two runs:\nfirst with default options and then with several GIT_TEST_* options)\ncauses the build to go beyond the three hour limit, and the builds time\nout.\n\nI'll just need to keep this in mind and do some more diligence myself\nto check if things are covered in the 9000 tests before bugging people\nabout coverage.\n\nThanks,\n-Stolee\n \n"},{"id":"378315","messageId":"20190629045322.GA8155@sigill.intra.peff.net","threadId":"51396","inReplyTo":"20190628093751.GA3569@sigill.intra.peff.net","subject":"[PATCH v2 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T04:53:22Z","receivedAt":"2019-06-29T04:54:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll of just the first patch of this series, since that was\nthe one that got all the comments (and the others are textually\nindependent).\n\nThe changes are:\n\n  - drop the leftover in_dir assignment\n\n  - replace ${indir:+} magic with defaulting indir=. (so we always pass\n    it to \"-C\"\n\n  - replace eval formatting magic with \"%s\" printf formatters (safer and\n    gets rid of quoting issues in the callers).\n\n  - use a tempfile to avoid significant logic on the left-hand subshell\n    of a pipe. This actually  _doesn't_ save a process because we end up\n    having to call \"rm\" to get rid of the tempfile. But I think it makes\n    the logic easier to follow (we can get just call test_tick as normal\n    in our loop), and as a bonus it leaves something you can inspect if\n    the fast-import fails.\n\n  - I dropped the comparison to test_commit in the documentation, since\n    it isn't a direct replacement due to the lack of tag creation. I\n    think that makes it clear enough.\n\n    I _didn't_ move the documentation out to t/README. IMHO we should be\n    moving in the opposite direction. But either way, I think it's\n    something we should handle separately (either consistently moving it\n    all into t/README, or moving it closer to the definitions).\n\nI didn't re-run all of the timings, but I spot-checked a few and got\nsimilar improvements (weirdly all of my timings, both before and after,\nseem slightly faster today; apparently gremlins were slowing my machine\ndown yesterday?).\n\n-- >8 --\nSubject: test-lib: introduce test_commit_bulk\nSome tests need to create a string of commits. Doing this with\ntest_commit is very heavy-weight, as it needs at least one process per\ncommit (and in fact, uses several).\n\nFor bulk creation, we can do much better by using fast-import, but it's\noften a pain to generate the input. Let's provide a helper to do so.\n\nWe'll use t5310 as a guinea pig, as it has three 10-commit loops. Here\nare hyperfine results before and after:\n\n  [before]\n  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]\n    Range (min … max):    2.250 s …  3.210 s    10 runs\n\n  [after]\n  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]\n    Range (min … max):    1.999 s …  2.590 s    10 runs\n\nSo we're over 20% faster, while making the callers slightly shorter. We\nadded a lot more lines in test-lib-function.sh, of course, and the\nhelper is way more featureful than we need here. But my hope is that it\nwill be flexible enough to use in more places.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5310-pack-bitmaps.sh |  15 +----\n t/test-lib-functions.sh | 123 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 126 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a26c8ba9a2..3aab7024ca 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -21,15 +21,9 @@ has_any () {\n }\n \n test_expect_success 'setup repo with moderate-sized history' '\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit $i\n-\tdone &&\n+\ttest_commit_bulk --id=file 10 &&\n \tgit checkout -b other HEAD~5 &&\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit side-$i\n-\tdone &&\n+\ttest_commit_bulk --id=side 10 &&\n \tgit checkout master &&\n \tbitmaptip=$(git rev-parse master) &&\n \tblob=$(echo tagged-blob | git hash-object -w --stdin) &&\n@@ -106,10 +100,7 @@ test_expect_success 'clone from bitmapped repository' '\n '\n \n test_expect_success 'setup further non-bitmapped commits' '\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit further-$i\n-\tdone\n+\ttest_commit_bulk --id=further 10\n '\n \n rev_list_tests 'partial bitmap'\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0367cec5fd..9fd0fa2a89 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -233,6 +233,129 @@ test_merge () {\n \tgit tag \"$1\"\n }\n \n+# Efficiently create <nr> commits, each with a unique number (from 1 to <nr>\n+# by default) in the commit message.\n+#\n+# Usage: test_commit_bulk [options] <nr>\n+#   -C <dir>:\n+#\tRun all git commands in directory <dir>\n+#   --ref=<n>:\n+#\tref on which to create commits (default: HEAD)\n+#   --start=<n>:\n+#\tnumber commit messages from <n> (default: 1)\n+#   --message=<msg>:\n+#\tuse <msg> as the commit mesasge (default: \"commit %s\")\n+#   --filename=<fn>:\n+#\tmodify <fn> in each commit (default: %s.t)\n+#   --contents=<string>:\n+#\tplace <string> in each file (default: \"content %s\")\n+#   --id=<string>:\n+#\tshorthand to use <string> and %s in message, filename, and contents\n+#\n+# The message, filename, and contents strings are evaluated by printf, with the\n+# first \"%s\" replaced by the current commit number. So you can do:\n+#\n+#   test_commit_bulk --filename=file --contents=\"modification %s\"\n+#\n+# to have every commit touch the same file, but with unique content.\n+#\n+test_commit_bulk () {\n+\ttmpfile=.bulk-commit.input\n+\tindir=.\n+\tref=HEAD\n+\tn=1\n+\tmessage='commit %s'\n+\tfilename='%s.t'\n+\tcontents='content %s'\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-C)\n+\t\t\tindir=$2\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--ref=*)\n+\t\t\tref=${1#--*=}\n+\t\t\t;;\n+\t\t--start=*)\n+\t\t\tn=${1#--*=}\n+\t\t\t;;\n+\t\t--message=*)\n+\t\t\tmessage=${1#--*=}\n+\t\t\t;;\n+\t\t--filename=*)\n+\t\t\tfilename=${1#--*=}\n+\t\t\t;;\n+\t\t--contents=*)\n+\t\t\tcontents=${1#--*=}\n+\t\t\t;;\n+\t\t--id=*)\n+\t\t\tmessage=\"${1#--*=} %s\"\n+\t\t\tfilename=\"${1#--*=}-%s.t\"\n+\t\t\tcontents=\"${1#--*=} %s\"\n+\t\t\t;;\n+\t\t-*)\n+\t\t\tBUG \"invalid test_commit_bulk option: $1\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\ttotal=$1\n+\n+\t{\n+\t\t# A \"reset ... from\" instructs fastimport to build on an\n+\t\t# existing branch tip rather than trying to overwrite.\n+\t\tif tip=$(git -C \"$indir\" rev-parse --verify \"$ref\" 2>/dev/null)\n+\t\tthen\n+\t\t\techo \"reset $ref\"\n+\t\t\techo \"from $tip\"\n+\t\tfi\n+\n+\t\twhile test \"$total\" -gt 0\n+\t\tdo\n+\t\t\ttest_tick &&\n+\t\t\techo \"commit $ref\"\n+\t\t\tprintf 'author %s <%s> %s\\n' \\\n+\t\t\t\t\"$GIT_AUTHOR_NAME\" \\\n+\t\t\t\t\"$GIT_AUTHOR_EMAIL\" \\\n+\t\t\t\t\"$GIT_AUTHOR_DATE\"\n+\t\t\tprintf 'committer %s <%s> %s\\n' \\\n+\t\t\t\t\"$GIT_COMMITTER_NAME\" \\\n+\t\t\t\t\"$GIT_COMMITTER_EMAIL\" \\\n+\t\t\t\t\"$GIT_COMMITTER_DATE\"\n+\t\t\techo \"data <<EOF\"\n+\t\t\tprintf \"$message\\n\" $n\n+\t\t\techo \"EOF\"\n+\t\t\tprintf \"M 644 inline $filename\\n\" $n\n+\t\t\techo \"data <<EOF\"\n+\t\t\tprintf \"$contents\\n\" $n\n+\t\t\techo \"EOF\"\n+\t\t\techo\n+\t\t\tn=$((n + 1))\n+\t\t\ttotal=$((total - 1))\n+\t\tdone\n+\n+\t} >\"$tmpfile\"\n+\n+\tgit -C \"$indir\" \\\n+\t    -c fastimport.unpacklimit=0 \\\n+\t    fast-import <\"$tmpfile\" || return 1\n+\n+\t# This will be left in place on failure, which may aid debugging.\n+\trm -f \"$tmpfile\"\n+\n+\t# If we updated HEAD, then be nice and update the index and working\n+\t# tree, too.\n+\tif test \"$ref\" = \"HEAD\"\n+\tthen\n+\t\tgit -C \"$indir\" checkout -f HEAD || return 1\n+\tfi\n+\n+}\n+\n # This function helps systems where core.filemode=false is set.\n # Use it instead of plain 'chmod +x' to set or unset the executable bit\n # of a file in the working directory and add it to the index.\n-- \n2.22.0.775.g4ba9815492\n"},{"id":"378316","messageId":"20190629051559.GA8908@sigill.intra.peff.net","threadId":"51396","inReplyTo":"c2e0ceef-c14d-ea1d-7522-eab0bf2e48ce@gmail.com","subject":"Re: Git Test Coverage Report (Thurs. June 27)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T05:15:59Z","receivedAt":"2019-06-29T05:19:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 09:36:14PM -0400, Derrick Stolee wrote:\n\n> > Still, if it's not too painful to add them in time-wise, it probably\n> > makes sense for the coverage tests to be as exhaustive as possible.\n> \n> Unfortunately, even running the t9*.sh tests once (among the two runs:\n> first with default options and then with several GIT_TEST_* options)\n> causes the build to go beyond the three hour limit, and the builds time\n> out.\n\nIs that because you're running the tests sequentially, due to the\ncorruption of the gcov files?\n\nI think something like this would work to get per-script profiles:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 4b346467df..81841191d2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -369,6 +369,9 @@ TZ=UTC\n export LANG LC_ALL PAGER TZ\n EDITOR=:\n \n+GCOV_PREFIX=$TEST_RESULTS_BASE.gcov\n+export GCOV_PREFIX\n+\n # GIT_TEST_GETTEXT_POISON should not influence git commands executed\n # during initialization of test-lib and the test repo. Back it up,\n # unset and then restore after initialization is finished.\n\n\nAnd then you can reassemble that with something like this (gcov-tool\ncomes with gcc):\n\n  for i in t/test-results/t*.gcov; do\n    echo >&2 \"Merging $i...\"\n    gcov-tool merge -o . . \"$i/$PWD\"\n  done\n\nThe merge is pretty slow, though (and necessarily serial). I wonder if\nyou'd do better to dump gcov output from each directory and then collate\nit as text. I've heard lcov also has better support for handling\nmultiple runs like this.\n\n-Peff\n"},{"id":"378322","messageId":"20190629082457.GE21574@szeder.dev","threadId":"51396","inReplyTo":"CAPig+cQnvotvJakCp1ic_WP3AMojNmfZw-PoinR=VOODmtNBtw@mail.gmail.com","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-29T08:24:57Z","receivedAt":"2019-06-29T08:25:03Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 28, 2019 at 07:46:02PM -0400, Eric Sunshine wrote:\n> On Fri, Jun 28, 2019 at 7:04 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> > On Fri, Jun 28, 2019 at 05:32:35PM -0400, Eric Sunshine wrote:\n> > > On Fri, Jun 28, 2019 at 5:39 AM Jeff King <peff@peff.net> wrote:\n> > > > +       in_dir=${indir:+-C \"$indir\"}\n> > >\n> > > Doesn't this suffer the problem in which some older/broken\n> > > shells[1][2][3][4] incorrectly [...]\n> >\n> > I think we don't need any of those \"${indir:+-C \"$indir\"}\" parameter\n> > expansions and could simply use 'git -C \"$indir\" cmd...' everywhere.\n> > $indir is set to empty right at the start of the function, and 'git -C\n> > \"\" ...' works and doesn't change the working directory.\n> \n> I recall the discussion around the meaning of `-C \"\"` when that\n> command line option was introduced. The conclusion was that  the\n> zero-length argument should mean \"this directory\" since that's how `cd\n> \"\"` behaves. However, I don't think that behavior ever got documented,\n\nAlthough it's not documented (but see the patch below), we do\nexplicitly test it since 6a536e2076 (git: treat \"git -C '<path>'\" as a\nno-op when <path> is empty, 2015-03-06) and e.g. our completion script\nrelies on this behavior.\n\n> and it's not necessarily obvious. An alternative would be to default\n> 'indir' to \".\", which should give the same result and be easily\n> understood.\n\nThat's fine for me as well.\n\n\n   --- >8 ---\n\nSubject: [PATCH] Document that 'git -C \"\"' works and doesn't change directory\n\nIt's been behaving so since 6a536e2076 (git: treat \"git -C '<path>'\"\nas a no-op when <path> is empty, 2015-03-06).\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n Documentation/git.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex f9b09db89b..a9deca0acb 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -57,7 +57,8 @@ help ...`.\n \tRun as if git was started in '<path>' instead of the current working\n \tdirectory.  When multiple `-C` options are given, each subsequent\n \tnon-absolute `-C <path>` is interpreted relative to the preceding `-C\n-\t<path>`.\n+\t<path>`.  If '<path>' is present but empty, e.g. `-C \"\"`, then the\n+\tcurrent working directory is left unchanged.\n +\n This option affects options that expect path name like `--git-dir` and\n `--work-tree` in that their interpretations of the path names would be\n-- \n2.22.0.589.g5bd7971b91\n\n"},{"id":"378327","messageId":"CABPp-BEyq-9sj_9wxLdh66BJqqjQ80a8sCpXd_cMCArAHnM7kA@mail.gmail.com","threadId":"51396","inReplyTo":"20190629003057.GB3094@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] easy bulk commit creation in tests","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-06-29T16:38:43Z","receivedAt":"2019-06-29T16:38:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 28, 2019 at 6:32 PM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Jun 28, 2019 at 02:53:26PM +0200, Johannes Schindelin wrote:\n>\n> > > > It would be nice if we had a \"test_commits_bulk\" that used fast-import\n> > > > to create larger numbers of commits.\n> > >\n> > > So here's a patch to do that.\n> >\n> > I like the direction, especially because it would make it super easy to go\n> > one step further that would probably make a huge difference on Windows: to\n> > move `test_commit_bulk` to `test-tool commit-bulk`.\n>\n> I actually considered going directly there, but I don't think it would\n> make a big difference. In the biggest case we dropped 900 processes to\n> 4. If we really want to drop that to 1, we can:\n>\n>   - use a temp-file to avoid the left-hand-pipe subshell\n>\n>   - add a feature to fast-import to say \"build on top of ref X\", instead\n>     of using to use rev-parse to manually generates a \"reset\" line\n>     (maybe this is even possible already; I searched for it, but not\n>     very hard).\n\nIt already exists; quoting the fast-import documentation:\n\n\"The special case of restarting an incremental import from the\ncurrent branch value should be written as:\n\n            from refs/heads/branch^0\n\nThe ^0 suffix is necessary as fast-import does not permit a branch\nto start from itself, and the branch is created in memory before\nthe from command is even read from the input. Adding ^0 will force\nfast-import to resolve the commit through Git's revision parsing\nlibrary, rather than its internal branch table, thereby loading in\nthe existing value of the branch.\"\n\n>   - add a feature to fast-import to have it check out the result of HEAD\n>     if it was updated\n\nThat'd be cool if you could work out the various special cases; it'd\nbe nice to avoid the 'git reset --hard HEAD' afterwards that I always\ndo.\n\n> The third one is a little less elegant to me, because there are a lot of\n> questions about how to checkout (e.g., with \"-f\", what happens to\n> deleted files, etc).\n\nThere's a question with deleted files?  Why wouldn't you just delete\nthem from the index and working tree?  The more interesting questions\nto me in this case is what to do if the index or working tree were\ndirty before the import started; that seems like a mess, though maybe\nit's just a case where you abort before even importing.  On a similar\nnote, though, there could have been an untracked file that is in the\nway of a now-to-be-tracked file that you might not want to lose.\n\nElijah\n"},{"id":"378339","messageId":"20190630063410.GA31264@sigill.intra.peff.net","threadId":"51396","inReplyTo":"CABPp-BEyq-9sj_9wxLdh66BJqqjQ80a8sCpXd_cMCArAHnM7kA@mail.gmail.com","subject":"Re: [PATCH 0/6] easy bulk commit creation in tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-30T06:34:11Z","receivedAt":"2019-06-30T06:34:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 29, 2019 at 10:38:43AM -0600, Elijah Newren wrote:\n\n> >   - add a feature to fast-import to say \"build on top of ref X\", instead\n> >     of using to use rev-parse to manually generates a \"reset\" line\n> >     (maybe this is even possible already; I searched for it, but not\n> >     very hard).\n> \n> It already exists; quoting the fast-import documentation:\n> \n> \"The special case of restarting an incremental import from the\n> current branch value should be written as:\n> \n>             from refs/heads/branch^0\n\nThank you! I looked over the documentation several times for this, but I\nwas looking for an individual command similar to \"reset\".\n\nUnfortunately, I'm not sure we can use this to save ourselves a process.\nWhat I really want to say is \"if it does not exist, start from scratch\nand otherwise build on the existing branch\".\n\nI couldn't figure out a way to do that without first finding out myself\nif the branch exists (incurring a process) and then modifying my\nfast-import stream appropriately.\n\nSo I don't think it actually shaves off our processes, but as I argued\nelsewhere, I think it's probably not that important anyway. I do think\nthe end result is a bit simpler to read, too, as the while-loop now\ngenerates the input in its entirety (I didn't reindent it yet in the\ndiff below):\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 9fd0fa2a89..4233f408e8 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -305,14 +305,11 @@ test_commit_bulk () {\n \tdone\n \ttotal=$1\n \n-\t{\n-\t\t# A \"reset ... from\" instructs fastimport to build on an\n-\t\t# existing branch tip rather than trying to overwrite.\n-\t\tif tip=$(git -C \"$indir\" rev-parse --verify \"$ref\" 2>/dev/null)\n-\t\tthen\n-\t\t\techo \"reset $ref\"\n-\t\t\techo \"from $tip\"\n-\t\tfi\n+\tadd_from=\n+\tif git rev-parse --verify \"$ref\" >/dev/null 2>&1\n+\tthen\n+\t\tadd_from=t\n+\tfi\n \n \t\twhile test \"$total\" -gt 0\n \t\tdo\n@@ -329,16 +326,16 @@ test_commit_bulk () {\n \t\t\techo \"data <<EOF\"\n \t\t\tprintf \"$message\\n\" $n\n \t\t\techo \"EOF\"\n+\t\t\ttest -n \"$add_from\" && echo \"from $ref^0\"\n \t\t\tprintf \"M 644 inline $filename\\n\" $n\n \t\t\techo \"data <<EOF\"\n \t\t\tprintf \"$contents\\n\" $n\n \t\t\techo \"EOF\"\n \t\t\techo\n+\t\t\tadd_from=\n \t\t\tn=$((n + 1))\n \t\t\ttotal=$((total - 1))\n-\t\tdone\n-\n-\t} >\"$tmpfile\"\n+\t\tdone >\"$tmpfile\"\n \n \tgit -C \"$indir\" \\\n \t    -c fastimport.unpacklimit=0 \\\n\nActually, thinking about it more, avoiding the $() probably does save us\na subshell fork, too.\n\n> > The third one is a little less elegant to me, because there are a lot of\n> > questions about how to checkout (e.g., with \"-f\", what happens to\n> > deleted files, etc).\n> \n> There's a question with deleted files?  Why wouldn't you just delete\n> them from the index and working tree?  The more interesting questions\n> to me in this case is what to do if the index or working tree were\n> dirty before the import started; that seems like a mess, though maybe\n> it's just a case where you abort before even importing.  On a similar\n> note, though, there could have been an untracked file that is in the\n> way of a now-to-be-tracked file that you might not want to lose.\n\nSorry, by deleted I meant files that were already deleted in the working\ntree or index, not ones our fast-import stream deleted. I.e,. the same\ndirty case you're asking about. But modifications have the same problem,\ntoo (I was thinking we'd just overwrite them as if the user had done\n\"cat >dirty-file\" as part of their fast-import, but that only applies to\nfiles they actually touched).\n\nSo \"dirty\" is definitely the right way to think about it.\n\n-Peff\n"},{"id":"378416","messageId":"xmqqblyd7it9.fsf@gitster-ct.c.googlers.com","threadId":"51396","inReplyTo":"20190629082457.GE21574@szeder.dev","subject":"Re: [PATCH 1/6] test-lib: introduce test_commit_bulk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-01T17:42:26Z","receivedAt":"2019-07-01T17:42:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>> I recall the discussion around the meaning of `-C \"\"` when that\n>> command line option was introduced. The conclusion was that  the\n>> zero-length argument should mean \"this directory\" since that's how `cd\n>> \"\"` behaves. However, I don't think that behavior ever got documented,\n>\n> Although it's not documented (but see the patch below), we do\n> explicitly test it since 6a536e2076 (git: treat \"git -C '<path>'\" as a\n> no-op when <path> is empty, 2015-03-06) and e.g. our completion script\n> relies on this behavior.\n>\n>> and it's not necessarily obvious. An alternative would be to default\n>> 'indir' to \".\", which should give the same result and be easily\n>> understood.\n>\n> That's fine for me as well.\n\nI find the \"an empty string is the same as a dot and means the\ncurrent directory\" a bit counter-intuitive, but as long as we have\nkept Git working that way for this long, we should document it, too.\n\nFor the tests in the patch in question, I think \"-C .\" is a good\nthing to use.\n\nThanks.  Will queue.\n\n\n>    --- >8 ---\n>\n> Subject: [PATCH] Document that 'git -C \"\"' works and doesn't change directory\n>\n> It's been behaving so since 6a536e2076 (git: treat \"git -C '<path>'\"\n> as a no-op when <path> is empty, 2015-03-06).\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  Documentation/git.txt | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index f9b09db89b..a9deca0acb 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -57,7 +57,8 @@ help ...`.\n>  \tRun as if git was started in '<path>' instead of the current working\n>  \tdirectory.  When multiple `-C` options are given, each subsequent\n>  \tnon-absolute `-C <path>` is interpreted relative to the preceding `-C\n> -\t<path>`.\n> +\t<path>`.  If '<path>' is present but empty, e.g. `-C \"\"`, then the\n> +\tcurrent working directory is left unchanged.\n>  +\n>  This option affects options that expect path name like `--git-dir` and\n>  `--work-tree` in that their interpretations of the path names would be\n"},{"id":"378464","messageId":"xmqq7e915r6k.fsf@gitster-ct.c.googlers.com","threadId":"51396","inReplyTo":"20190629045322.GA8155@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/6] test-lib: introduce test_commit_bulk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-01T22:24:35Z","receivedAt":"2019-07-01T22:24:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here's a re-roll of just the first patch of this series, since that was\n> the one that got all the comments (and the others are textually\n> independent).\n\nOK, will replace and then queue an adjustment for 6200 which used to\nuse \\$n but now must use %s instead.  Let's see if people spot things\nworth pointing out in the remainder of the series (or this one, of\ncourse, but I found this step quite sensible).\n\nThanks.\n"},{"id":"378465","messageId":"xmqq36jp5qzm.fsf@gitster-ct.c.googlers.com","threadId":"51396","inReplyTo":"20190629045322.GA8155@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/6] test-lib: introduce test_commit_bulk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-01T22:28:45Z","receivedAt":"2019-07-01T22:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   - replace eval formatting magic with \"%s\" printf formatters (safer and\n>     gets rid of quoting issues in the callers).\n\nThis one actually made me think twice about safety, as we'd be using\nend-user supplied formatting string without any inspection.  I think\nit is fine as it is merely a test helper.  \n\nIf somebody is later making it into a test-tool function, I expect\nthat our interpolation engine, not the bare sprintf(), would be used\nthere, and it would hopefully also be safe?\n\nThanks.\n\n"},{"id":"378471","messageId":"20190702051649.GA16344@sigill.intra.peff.net","threadId":"51396","inReplyTo":"xmqq7e915r6k.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-02T05:16:49Z","receivedAt":"2019-07-02T05:16:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 01, 2019 at 03:24:35PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Here's a re-roll of just the first patch of this series, since that was\n> > the one that got all the comments (and the others are textually\n> > independent).\n> \n> OK, will replace and then queue an adjustment for 6200 which used to\n> use \\$n but now must use %s instead.  Let's see if people spot things\n> worth pointing out in the remainder of the series (or this one, of\n> course, but I found this step quite sensible).\n\nUrgh, I forgot I did have to tweak that later test. Thanks for noticing.\n\nI do have one more update based on the comments from Elijah: using\n\"from\" in the initial commit lets us simplify a few things (I posted the\nincremental earlier in the thread, but here it is as a complete\nreplacement for patch 1).\n\n-- >8 --\nSubject: [PATCH v3] test-lib: introduce test_commit_bulk\n\nSome tests need to create a string of commits. Doing this with\ntest_commit is very heavy-weight, as it needs at least one process per\ncommit (and in fact, uses several).\n\nFor bulk creation, we can do much better by using fast-import, but it's\noften a pain to generate the input. Let's provide a helper to do so.\n\nWe'll use t5310 as a guinea pig, as it has three 10-commit loops. Here\nare hyperfine results before and after:\n\n  [before]\n  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]\n    Range (min … max):    2.250 s …  3.210 s    10 runs\n\n  [after]\n  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests\n    Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]\n    Range (min … max):    1.999 s …  2.590 s    10 runs\n\nSo we're over 20% faster, while making the callers slightly shorter. We\nadded a lot more lines in test-lib-function.sh, of course, and the\nhelper is way more featureful than we need here. But my hope is that it\nwill be flexible enough to use in more places.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5310-pack-bitmaps.sh |  15 +----\n t/test-lib-functions.sh | 123 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 126 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a26c8ba9a2..3aab7024ca 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -21,15 +21,9 @@ has_any () {\n }\n \n test_expect_success 'setup repo with moderate-sized history' '\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit $i\n-\tdone &&\n+\ttest_commit_bulk --id=file 10 &&\n \tgit checkout -b other HEAD~5 &&\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit side-$i\n-\tdone &&\n+\ttest_commit_bulk --id=side 10 &&\n \tgit checkout master &&\n \tbitmaptip=$(git rev-parse master) &&\n \tblob=$(echo tagged-blob | git hash-object -w --stdin) &&\n@@ -106,10 +100,7 @@ test_expect_success 'clone from bitmapped repository' '\n '\n \n test_expect_success 'setup further non-bitmapped commits' '\n-\tfor i in $(test_seq 1 10)\n-\tdo\n-\t\ttest_commit further-$i\n-\tdone\n+\ttest_commit_bulk --id=further 10\n '\n \n rev_list_tests 'partial bitmap'\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0367cec5fd..6083cf483a 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -233,6 +233,129 @@ test_merge () {\n \tgit tag \"$1\"\n }\n \n+# Efficiently create <nr> commits, each with a unique number (from 1 to <nr>\n+# by default) in the commit message.\n+#\n+# Usage: test_commit_bulk [options] <nr>\n+#   -C <dir>:\n+#\tRun all git commands in directory <dir>\n+#   --ref=<n>:\n+#\tref on which to create commits (default: HEAD)\n+#   --start=<n>:\n+#\tnumber commit messages from <n> (default: 1)\n+#   --message=<msg>:\n+#\tuse <msg> as the commit mesasge (default: \"commit %s\")\n+#   --filename=<fn>:\n+#\tmodify <fn> in each commit (default: %s.t)\n+#   --contents=<string>:\n+#\tplace <string> in each file (default: \"content %s\")\n+#   --id=<string>:\n+#\tshorthand to use <string> and %s in message, filename, and contents\n+#\n+# The message, filename, and contents strings are evaluated by printf, with the\n+# first \"%s\" replaced by the current commit number. So you can do:\n+#\n+#   test_commit_bulk --filename=file --contents=\"modification %s\"\n+#\n+# to have every commit touch the same file, but with unique content.\n+#\n+test_commit_bulk () {\n+\ttmpfile=.bulk-commit.input\n+\tindir=.\n+\tref=HEAD\n+\tn=1\n+\tmessage='commit %s'\n+\tfilename='%s.t'\n+\tcontents='content %s'\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-C)\n+\t\t\tindir=$2\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--ref=*)\n+\t\t\tref=${1#--*=}\n+\t\t\t;;\n+\t\t--start=*)\n+\t\t\tn=${1#--*=}\n+\t\t\t;;\n+\t\t--message=*)\n+\t\t\tmessage=${1#--*=}\n+\t\t\t;;\n+\t\t--filename=*)\n+\t\t\tfilename=${1#--*=}\n+\t\t\t;;\n+\t\t--contents=*)\n+\t\t\tcontents=${1#--*=}\n+\t\t\t;;\n+\t\t--id=*)\n+\t\t\tmessage=\"${1#--*=} %s\"\n+\t\t\tfilename=\"${1#--*=}-%s.t\"\n+\t\t\tcontents=\"${1#--*=} %s\"\n+\t\t\t;;\n+\t\t-*)\n+\t\t\tBUG \"invalid test_commit_bulk option: $1\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\ttotal=$1\n+\n+\tadd_from=\n+\tif git -C \"$indir\" rev-parse --verify \"$ref\"\n+\tthen\n+\t\tadd_from=t\n+\tfi\n+\n+\twhile test \"$total\" -gt 0\n+\tdo\n+\t\ttest_tick &&\n+\t\techo \"commit $ref\"\n+\t\tprintf 'author %s <%s> %s\\n' \\\n+\t\t\t\"$GIT_AUTHOR_NAME\" \\\n+\t\t\t\"$GIT_AUTHOR_EMAIL\" \\\n+\t\t\t\"$GIT_AUTHOR_DATE\"\n+\t\tprintf 'committer %s <%s> %s\\n' \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \\\n+\t\t\t\"$GIT_COMMITTER_EMAIL\" \\\n+\t\t\t\"$GIT_COMMITTER_DATE\"\n+\t\techo \"data <<EOF\"\n+\t\tprintf \"$message\\n\" $n\n+\t\techo \"EOF\"\n+\t\tif test -n \"$add_from\"\n+\t\tthen\n+\t\t\techo \"from $ref^0\"\n+\t\t\tadd_from=\n+\t\tfi\n+\t\tprintf \"M 644 inline $filename\\n\" $n\n+\t\techo \"data <<EOF\"\n+\t\tprintf \"$contents\\n\" $n\n+\t\techo \"EOF\"\n+\t\techo\n+\t\tn=$((n + 1))\n+\t\ttotal=$((total - 1))\n+\tdone >\"$tmpfile\"\n+\n+\tgit -C \"$indir\" \\\n+\t    -c fastimport.unpacklimit=0 \\\n+\t    fast-import <\"$tmpfile\" || return 1\n+\n+\t# This will be left in place on failure, which may aid debugging.\n+\trm -f \"$tmpfile\"\n+\n+\t# If we updated HEAD, then be nice and update the index and working\n+\t# tree, too.\n+\tif test \"$ref\" = \"HEAD\"\n+\tthen\n+\t\tgit -C \"$indir\" checkout -f HEAD || return 1\n+\tfi\n+\n+}\n+\n # This function helps systems where core.filemode=false is set.\n # Use it instead of plain 'chmod +x' to set or unset the executable bit\n # of a file in the working directory and add it to the index.\n-- \n2.22.0.776.g16867c022c\n\n"},{"id":"378472","messageId":"20190702052243.GB16344@sigill.intra.peff.net","threadId":"51396","inReplyTo":"xmqq36jp5qzm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/6] test-lib: introduce test_commit_bulk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-02T05:22:44Z","receivedAt":"2019-07-02T05:22:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 01, 2019 at 03:28:45PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >   - replace eval formatting magic with \"%s\" printf formatters (safer and\n> >     gets rid of quoting issues in the callers).\n> \n> This one actually made me think twice about safety, as we'd be using\n> end-user supplied formatting string without any inspection.  I think\n> it is fine as it is merely a test helper.\n\nYeah, and most shells do something sensible with nonsense formats.\nE.g., \"%s %s\" will yield an empty string for the second one in both dash\nand bash (and that's what POSIX says, though I'd be happy with any\nimplementation that avoids segfaulting).\n\n> If somebody is later making it into a test-tool function, I expect\n> that our interpolation engine, not the bare sprintf(), would be used\n> there, and it would hopefully also be safe?\n\nYes, that was exactly my plan. It would also let you mention the number\nmore than once in the format, though I doubt any callers would care\nabout that feature.\n\nI also think more potential callers could be converted if the refname\nwas formatted, too (e.g., some of them seem to write to branch-1,\nbranch-2, etc). I drew the line there, but anybody is welcome to explore\nit further.\n\n-Peff\n"}]}