{"thread":{"id":"55540","subject":"[PATCH 0/7] Parallel Checkout (part 3)","startedAt":"2021-04-22T15:18:05Z","lastAt":"2021-05-26T23:00:52Z","messageCount":50,"participants":["Matheus Tavares","Derrick Stolee","Matheus Tavares Bernardino","Christian Couder","Torsten Bögershausen","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"422683","messageId":"cover.1619104091.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":null,"subject":"[PATCH 0/7] Parallel Checkout (part 3)","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-22T15:17:46Z","receivedAt":"2021-04-22T15:18:05Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This is the last part of the parallel checkout series :) I have a couple\nother small optimization and tuning ideas for later but, with this part,\nthe main functionality should be now complete.\n\nThis is based on mt/parallel-checkout-part-2.\n\nThe first three patches add parallel checkout support for three\ncodepaths that could not use the parallel mode yet:\n\n- git checkout-index\n- git checkout --patch\n- git checkout \n\nAll these three call `checkout_entry()` directly instead of\n`unpack_trees()`, so we need to enable parallel checkout on them before\nthey call `checkout_entry()`.\n\nThe four remaining patches add tests for the parallel checkout\nframework.\n\nMatheus Tavares (7):\n make_transient_cache_entry(): optionally alloc from mem_pool\n builtin/checkout.c: complete parallel checkout support\n checkout-index: add parallel checkout support\n parallel-checkout: add tests for basic operations\n parallel-checkout: add tests related to path collisions\n parallel-checkout: add tests related to .gitattributes\n ci: run test round with parallel-checkout enabled\n\n builtin/checkout--worker.c | 2 +-\n builtin/checkout-index.c | 24 +--\n builtin/checkout.c | 20 ++-\n builtin/difftool.c | 2 +-\n cache.h | 11 +-\n ci/run-build-and-tests.sh | 1 +\n parallel-checkout.c | 18 +++\n read-cache.c | 12 +-\n t/README | 4 +\n t/lib-encoding.sh | 25 +++\n t/lib-parallel-checkout.sh | 40 +++++\n t/t0028-working-tree-encoding.sh | 25 +--\n t/t2080-parallel-checkout-basics.sh | 150 ++++++++++++++++++\n t/t2081-parallel-checkout-collisions.sh | 162 ++++++++++++++++++++\n t/t2082-parallel-checkout-attributes.sh | 194 ++++++++++++++++++++++++\n unpack-trees.c | 2 +-\n 16 files changed, 643 insertions(+), 49 deletions(-)\n create mode 100644 t/lib-encoding.sh\n create mode 100644 t/lib-parallel-checkout.sh\n create mode 100755 t/t2080-parallel-checkout-basics.sh\n create mode 100755 t/t2081-parallel-checkout-collisions.sh\n create mode 100755 t/t2082-parallel-checkout-attributes.sh\n\n-- \n2.30.1\n\n"},{"id":"422684","messageId":"f870040bfb3e73ee8cd27352b0acc65bb54be560.1619104091.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619104091.git.matheus.bernardino@usp.br","subject":"[PATCH 1/7] make_transient_cache_entry(): optionally alloc from mem_pool","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-22T15:17:47Z","receivedAt":"2021-04-22T15:18:08Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Allow make_transient_cache_entry() to optionally receive a mem_pool\nstruct in which it should allocate the entry. This will be used in the\nfollowing patch, to store some transient entries which should persist\nuntil parallel checkout finishes.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout--worker.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/difftool.c | 2 +-\n cache.h | 11 ++++++-----\n read-cache.c | 12 ++++++++----\n unpack-trees.c | 2 +-\n 6 files changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c\nindex 31e0de2f7e..289a9b8f89 100644\n--- a/builtin/checkout--worker.c\n+++ b/builtin/checkout--worker.c\n@@ -39,7 +39,7 @@ static void packet_to_pc_item(const char *buffer, int len,\n \t}\n \n \tmemset(pc_item, 0, sizeof(*pc_item));\n-\tpc_item->ce = make_empty_transient_cache_entry(fixed_portion->name_len);\n+\tpc_item->ce = make_empty_transient_cache_entry(fixed_portion->name_len, NULL);\n \tpc_item->ce->ce_namelen = fixed_portion->name_len;\n \tpc_item->ce->ce_mode = fixed_portion->ce_mode;\n \tmemcpy(pc_item->ce->name, variant, pc_item->ce->ce_namelen);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 4c696ef480..db667d0267 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -291,7 +291,7 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko\n \tif (write_object_file(result_buf.ptr, result_buf.size, blob_type, &oid))\n \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n \tfree(result_buf.ptr);\n-\tce = make_transient_cache_entry(mode, &oid, path, 2);\n+\tce = make_transient_cache_entry(mode, &oid, path, 2, NULL);\n \tif (!ce)\n \t\tdie(_(\"make_cache_entry failed for path '%s'\"), path);\n \tstatus = checkout_entry(ce, state, NULL, nr_checkouts);\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex ef25729d49..afacbcd581 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -323,7 +323,7 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n \tstruct cache_entry *ce;\n \tint ret;\n \n-\tce = make_transient_cache_entry(mode, oid, path, 0);\n+\tce = make_transient_cache_entry(mode, oid, path, 0, NULL);\n \tret = checkout_entry(ce, state, NULL, NULL);\n \n \tdiscard_cache_entry(ce);\ndiff --git a/cache.h b/cache.h\nindex 148d9ab5f1..b6b42cc3f3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -356,16 +356,17 @@ struct cache_entry *make_empty_cache_entry(struct index_state *istate,\n \t\t\t\t\t size_t name_len);\n \n /*\n- * Create a cache_entry that is not intended to be added to an index.\n- * Caller is responsible for discarding the cache_entry\n- * with `discard_cache_entry`.\n+ * Create a cache_entry that is not intended to be added to an index. If `mp`\n+ * is not NULL, the entry is allocated within the given memory pool. Caller is\n+ * responsible for discarding \"loose\" entries with `discard_cache_entry()` and\n+ * the mem_pool with `mem_pool_discard(mp, should_validate_cache_entries())`.\n */\n struct cache_entry *make_transient_cache_entry(unsigned int mode,\n \t\t\t\t\t const struct object_id *oid,\n \t\t\t\t\t const char *path,\n-\t\t\t\t\t int stage);\n+\t\t\t\t\t int stage, struct mem_pool *mp);\n \n-struct cache_entry *make_empty_transient_cache_entry(size_t name_len);\n+struct cache_entry *make_empty_transient_cache_entry(size_t len, struct mem_pool *mp);\n \n /*\n * Discard cache entry.\ndiff --git a/read-cache.c b/read-cache.c\nindex 5a907af2fb..eb389ccb8f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -813,8 +813,10 @@ struct cache_entry *make_empty_cache_entry(struct index_state *istate, size_t le\n \treturn mem_pool__ce_calloc(find_mem_pool(istate), len);\n }\n \n-struct cache_entry *make_empty_transient_cache_entry(size_t len)\n+struct cache_entry *make_empty_transient_cache_entry(size_t len, struct mem_pool *mp)\n {\n+\tif (mp)\n+\t\treturn mem_pool__ce_calloc(mp, len);\n \treturn xcalloc(1, cache_entry_size(len));\n }\n \n@@ -848,8 +850,10 @@ struct cache_entry *make_cache_entry(struct index_state *istate,\n \treturn ret;\n }\n \n-struct cache_entry *make_transient_cache_entry(unsigned int mode, const struct object_id *oid,\n-\t\t\t\t\t const char *path, int stage)\n+struct cache_entry *make_transient_cache_entry(unsigned int mode,\n+\t\t\t\t\t const struct object_id *oid,\n+\t\t\t\t\t const char *path, int stage,\n+\t\t\t\t\t struct mem_pool *mp)\n {\n \tstruct cache_entry *ce;\n \tint len;\n@@ -860,7 +864,7 @@ struct cache_entry *make_transient_cache_entry(unsigned int mode, const struct o\n \t}\n \n \tlen = strlen(path);\n-\tce = make_empty_transient_cache_entry(len);\n+\tce = make_empty_transient_cache_entry(len, mp);\n \n \toidcpy(&ce->oid, oid);\n \tmemcpy(ce->name, path, len);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 4b77e52c6b..fa5b7ab7ee 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1034,7 +1034,7 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,\n \tsize_t len = traverse_path_len(info, tree_entry_len(n));\n \tstruct cache_entry *ce =\n \t\tis_transient ?\n-\t\tmake_empty_transient_cache_entry(len) :\n+\t\tmake_empty_transient_cache_entry(len, NULL) :\n \t\tmake_empty_cache_entry(istate, len);\n \n \tce->ce_mode = create_ce_mode(n->mode);\n-- \n2.30.1\n\n"},{"id":"422685","messageId":"5e0dee7beba083159f4277ddcd9e931859239bde.1619104091.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619104091.git.matheus.bernardino@usp.br","subject":"[PATCH 2/7] builtin/checkout.c: complete parallel checkout support","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-22T15:17:48Z","receivedAt":"2021-04-22T15:18:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"There is one code path in builtin/checkout.c which still doesn't benefit\nfrom parallel checkout because it calls checkout_entry() directly,\ninstead of unpack_trees(). Let's add parallel checkout support for this\nmissing spot as well. Note: the transient cache entries allocated in\ncheckout_merged() are now allocated in a mem_pool which is only\ndiscarded after parallel checkout finishes. This is done because the\nentries need to be valid when run_parallel_checkout() is called.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex db667d0267..b71dc08430 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -27,6 +27,7 @@\n #include \"wt-status.h\"\n #include \"xdiff-interface.h\"\n #include \"entry.h\"\n+#include \"parallel-checkout.h\"\n \n static const char * const checkout_usage[] = {\n \tN_(\"git checkout [] \"),\n@@ -230,7 +231,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,\n \t\treturn error(_(\"path '%s' does not have their version\"), ce->name);\n }\n \n-static int checkout_merged(int pos, const struct checkout *state, int *nr_checkouts)\n+static int checkout_merged(int pos, const struct checkout *state,\n+\t\t\t int *nr_checkouts, struct mem_pool *ce_mem_pool)\n {\n \tstruct cache_entry *ce = active_cache[pos];\n \tconst char *path = ce->name;\n@@ -291,11 +293,10 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko\n \tif (write_object_file(result_buf.ptr, result_buf.size, blob_type, &oid))\n \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n \tfree(result_buf.ptr);\n-\tce = make_transient_cache_entry(mode, &oid, path, 2, NULL);\n+\tce = make_transient_cache_entry(mode, &oid, path, 2, ce_mem_pool);\n \tif (!ce)\n \t\tdie(_(\"make_cache_entry failed for path '%s'\"), path);\n \tstatus = checkout_entry(ce, state, NULL, nr_checkouts);\n-\tdiscard_cache_entry(ce);\n \treturn status;\n }\n \n@@ -359,16 +360,22 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \tint nr_checkouts = 0, nr_unmerged = 0;\n \tint errs = 0;\n \tint pos;\n+\tint pc_workers, pc_threshold;\n+\tstruct mem_pool ce_mem_pool;\n \n \tstate.force = 1;\n \tstate.refresh_cache = 1;\n \tstate.istate = &the_index;\n \n+\tmem_pool_init(&ce_mem_pool, 0);\n+\tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n \tinit_checkout_metadata(&state.meta, info->refname,\n \t\t\t info->commit ? &info->commit->object.oid : &info->oid,\n \t\t\t NULL);\n \n \tenable_delayed_checkout(&state);\n+\tif (pc_workers > 1)\n+\t\tinit_parallel_checkout();\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n \t\tif (ce->ce_flags & CE_MATCHED) {\n@@ -384,10 +391,15 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \t\t\t\t\t\t &nr_checkouts, opts->overlay_mode);\n \t\t\telse if (opts->merge)\n \t\t\t\terrs |= checkout_merged(pos, &state,\n-\t\t\t\t\t\t\t&nr_unmerged);\n+\t\t\t\t\t\t\t&nr_unmerged,\n+\t\t\t\t\t\t\t&ce_mem_pool);\n \t\t\tpos = skip_same_name(ce, pos) - 1;\n \t\t}\n \t}\n+\tif (pc_workers > 1)\n+\t\terrs |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n+\t\t\t\t\t NULL, NULL);\n+\tmem_pool_discard(&ce_mem_pool, should_validate_cache_entries());\n \tremove_marked_cache_entries(&the_index, 1);\n \tremove_scheduled_dirs();\n \terrs |= finish_delayed_checkout(&state, &nr_checkouts);\n-- \n2.30.1\n\n"},{"id":"422686","messageId":"6bf1db2fd8e807f9f259787ef75651d43b61e03a.1619104091.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619104091.git.matheus.bernardino@usp.br","subject":"[PATCH 3/7] checkout-index: add parallel checkout support","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-22T15:17:49Z","receivedAt":"2021-04-22T15:18:10Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Note: previously, `checkout_all()` would not return on errors, but\ninstead call `exit()` with a non-zero code. However, it only did that\nafter calling `checkout_entry()` for all index entries, thus not\nstopping on the first error, but attempting to write the maximum number\nof entries possible. In order to maintain this behavior we now propagate\n`checkout_all()`s error status to `cmd_checkout_index()`, so that it can\ncall `run_parallel_checkout()` and attempt to write the queued entries\nbefore exiting with the error code.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout-index.c | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex c0bf4ac1b2..e8a82ea9ed 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -12,6 +12,7 @@\n #include \"cache-tree.h\"\n #include \"parse-options.h\"\n #include \"entry.h\"\n+#include \"parallel-checkout.h\"\n \n #define CHECKOUT_ALL 4\n static int nul_term_line;\n@@ -115,7 +116,7 @@ static int checkout_file(const char *name, const char *prefix)\n \treturn -1;\n }\n \n-static void checkout_all(const char *prefix, int prefix_length)\n+static int checkout_all(const char *prefix, int prefix_length)\n {\n \tint i, errs = 0;\n \tstruct cache_entry *last_ce = NULL;\n@@ -142,11 +143,7 @@ static void checkout_all(const char *prefix, int prefix_length)\n \t}\n \tif (last_ce && to_tempfile)\n \t\twrite_tempfile_record(last_ce->name, prefix);\n-\tif (errs)\n-\t\t/* we have already done our error reporting.\n-\t\t * exit with the same code as die().\n-\t\t */\n-\t\texit(128);\n+\treturn !!errs;\n }\n \n static const char * const builtin_checkout_index_usage[] = {\n@@ -182,6 +179,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tint force = 0, quiet = 0, not_new = 0;\n \tint index_opt = 0;\n \tint err = 0;\n+\tint pc_workers, pc_threshold;\n \tstruct option builtin_checkout_index_options[] = {\n \t\tOPT_BOOL('a', \"all\", &all,\n \t\t\tN_(\"check out all files in the index\")),\n@@ -236,6 +234,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n+\tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n+\tif (pc_workers > 1)\n+\t\tinit_parallel_checkout();\n+\n \t/* Check out named files first */\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n@@ -275,12 +277,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \t}\n \n+\tif (all)\n+\t\terr |= checkout_all(prefix, prefix_length);\n+\n+\tif (pc_workers > 1)\n+\t\terr |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n+\t\t\t\t\t NULL, NULL);\n+\n \tif (err)\n \t\treturn 1;\n \n-\tif (all)\n-\t\tcheckout_all(prefix, prefix_length);\n-\n \tif (is_lock_file_locked(&lock_file) &&\n \t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(\"Unable to write new index file\");\n-- \n2.30.1\n\n"},{"id":"422687","messageId":"6379b8df6a59361dd44733e379880a11c6cd977c.1619104091.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619104091.git.matheus.bernardino@usp.br","subject":"[PATCH 4/7] parallel-checkout: add tests for basic operations","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-22T15:17:50Z","receivedAt":"2021-04-22T15:18:27Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to populate the working tree during clone and checkout using\nsequential and parallel mode, to confirm that they produce identical\nresults. Also test basic checkout mechanics, such as checking for\nsymlinks in the leading directories and the abidance to --force.\n\nNote: some helper functions are added to a common lib file which is only\nincluded by t2080 for now. But they will also be used by other\nparallel-checkout tests in the following patches.\n\nOriginal-patch-by: Jeff Hostetler \nSigned-off-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n t/lib-parallel-checkout.sh | 37 +++++++\n t/t2080-parallel-checkout-basics.sh | 150 ++++++++++++++++++++++++++++\n 2 files changed, 187 insertions(+)\n create mode 100644 t/lib-parallel-checkout.sh\n create mode 100755 t/t2080-parallel-checkout-basics.sh\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nnew file mode 100644\nindex 0000000000..39fd36fdf6\n--- /dev/null\n+++ b/t/lib-parallel-checkout.sh\n@@ -0,0 +1,37 @@\n+# Helpers for t208* tests\n+\n+set_checkout_config () {\n+\tif test $# -ne 2\n+\tthen\n+\t\tBUG \"set_checkout_config() requires two arguments\"\n+\tfi &&\n+\n+\ttest_config_global checkout.workers $1 &&\n+\ttest_config_global checkout.thresholdForParallelism $2\n+}\n+\n+# Run \"${@:2}\" and check that $1 checkout workers were used\n+test_checkout_workers () {\n+\tif test $# -lt 2\n+\tthen\n+\t\tBUG \"too few arguments to test_checkout_workers()\"\n+\tfi &&\n+\n+\texpected_workers=$1 &&\n+\tshift &&\n+\n+\trm -f trace &&\n+\tGIT_TRACE2=\"$(pwd)/trace\" \"$@\" &&\n+\n+\tworkers=$(grep \"child_start\\[..*\\] git checkout--worker\" trace | wc -l) &&\n+\ttest $workers -eq $expected_workers &&\n+\trm -f trace\n+}\n+\n+# Verify that both the working tree and the index were created correctly\n+verify_checkout () {\n+\tgit -C \"$1\" diff-index --quiet HEAD -- &&\n+\tgit -C \"$1\" diff-index --quiet --cached HEAD -- &&\n+\tgit -C \"$1\" status --porcelain >\"$1\".status &&\n+\ttest_must_be_empty \"$1\".status\n+}\ndiff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\nnew file mode 100755\nindex 0000000000..0cb1493cdc\n--- /dev/null\n+++ b/t/t2080-parallel-checkout-basics.sh\n@@ -0,0 +1,150 @@\n+#!/bin/sh\n+\n+test_description='parallel-checkout basics\n+\n+Ensure that parallel-checkout basically works on clone and checkout, spawning\n+the required number of workers and correctly populating both the index and the\n+working tree.\n+'\n+\n+TEST_NO_CREATE_REPO=1\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+\n+# Test parallel-checkout with a branch switch containing file creations,\n+# deletions, and modification; with different entry types. Switching from B1 to\n+# B2 will have the following changes:\n+#\n+# - a (file): modified\n+# - e/x (file): deleted\n+# - b (symlink): deleted\n+# - b/f (file): created\n+# - e (symlink): created\n+# - d (submodule): created\n+#\n+test_expect_success SYMLINKS 'setup repo for checkout with various types of changes' '\n+\tgit init various &&\n+\t(\n+\t\tcd various &&\n+\t\tgit checkout -b B1 &&\n+\t\techo a >a &&\n+\t\tmkdir e &&\n+\t\techo e/x >e/x &&\n+\t\tln -s e b &&\n+\t\tgit add -A &&\n+\t\tgit commit -m B1 &&\n+\n+\t\tgit checkout -b B2 &&\n+\t\techo modified >a &&\n+\t\trm -rf e &&\n+\t\trm b &&\n+\t\tmkdir b &&\n+\t\techo b/f >b/f &&\n+\t\tln -s b e &&\n+\t\tgit init d &&\n+\t\ttest_commit -C d f &&\n+\t\tgit submodule add ./d &&\n+\t\tgit add -A &&\n+\t\tgit commit -m B2 &&\n+\n+\t\tgit checkout --recurse-submodules B1\n+\t)\n+'\n+\n+test_expect_success SYMLINKS 'sequential checkout' '\n+\tcp -R various various_sequential &&\n+\tset_checkout_config 1 0 &&\n+\ttest_checkout_workers 0 \\\n+\t\tgit -C various_sequential checkout --recurse-submodules B2 &&\n+\tverify_checkout various_sequential\n+'\n+\n+test_expect_success SYMLINKS 'parallel checkout' '\n+\tcp -R various various_parallel &&\n+\tset_checkout_config 2 0 &&\n+\ttest_checkout_workers 2 \\\n+\t\tgit -C various_parallel checkout --recurse-submodules B2 &&\n+\tverify_checkout various_parallel\n+'\n+\n+test_expect_success SYMLINKS 'fallback to sequential checkout (threshold)' '\n+\tcp -R various various_sequential_fallback &&\n+\tset_checkout_config 2 100 &&\n+\ttest_checkout_workers 0 \\\n+\t\tgit -C various_sequential_fallback checkout --recurse-submodules B2 &&\n+\tverify_checkout various_sequential_fallback\n+'\n+\n+test_expect_success SYMLINKS 'parallel checkout on clone' '\n+\tgit -C various checkout --recurse-submodules B2 &&\n+\tset_checkout_config 2 0 &&\n+\ttest_checkout_workers 2 \\\n+\t\tgit clone --recurse-submodules various various_parallel_clone &&\n+\tverify_checkout various_parallel_clone\n+'\n+\n+test_expect_success SYMLINKS 'fallback to sequential checkout on clone (threshold)' '\n+\tgit -C various checkout --recurse-submodules B2 &&\n+\tset_checkout_config 2 100 &&\n+\ttest_checkout_workers 0 \\\n+\t\tgit clone --recurse-submodules various various_sequential_fallback_clone &&\n+\tverify_checkout various_sequential_fallback_clone\n+'\n+\n+# Just to be paranoid, actually compare the working trees' contents directly.\n+test_expect_success SYMLINKS 'compare the working trees' '\n+\trm -rf various_*/.git &&\n+\trm -rf various_*/d/.git &&\n+\n+\tdiff -r various_sequential various_parallel &&\n+\tdiff -r various_sequential various_sequential_fallback &&\n+\tdiff -r various_sequential various_parallel_clone &&\n+\tdiff -r various_sequential various_sequential_fallback_clone\n+'\n+\n+test_expect_success 'parallel checkout respects --[no]-force' '\n+\tset_checkout_config 2 0 &&\n+\tgit init dirty &&\n+\t(\n+\t\tcd dirty &&\n+\t\tmkdir D &&\n+\t\ttest_commit D/F &&\n+\t\ttest_commit F &&\n+\n+\t\trm -rf D &&\n+\t\techo changed >D &&\n+\t\techo changed >F.t &&\n+\n+\t\t# We expect 0 workers because there is nothing to be done\n+\t\ttest_checkout_workers 0 git checkout HEAD &&\n+\t\ttest_path_is_file D &&\n+\t\tgrep changed D &&\n+\t\tgrep changed F.t &&\n+\n+\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n+\t\ttest_path_is_dir D &&\n+\t\tgrep D/F D/F.t &&\n+\t\tgrep F F.t\n+\t)\n+'\n+\n+test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading dirs' '\n+\tset_checkout_config 2 0 &&\n+\tgit init symlinks &&\n+\t(\n+\t\tcd symlinks &&\n+\t\tmkdir D untracked &&\n+\t\t# Commit 2 files to have enough work for 2 parallel workers\n+\t\ttest_commit D/A &&\n+\t\ttest_commit D/B &&\n+\t\trm -rf D &&\n+\t\tln -s untracked D &&\n+\n+\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n+\t\t! test -h D &&\n+\t\tgrep D/A D/A.t &&\n+\t\tgrep D/B D/B.t\n+\t)\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"422688","messageId":"cff50a467d7935be6789547233b2fa8276b36593.1619104091.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619104091.git.matheus.bernardino@usp.br","subject":"[PATCH 5/7] parallel-checkout: add tests related to path collisions","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-22T15:17:51Z","receivedAt":"2021-04-22T15:18:41Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to confirm that path collisions are properly detected by\ncheckout workers, both to avoid race conditions and to report colliding\nentries on clone.\n\nOriginal-patch-by: Jeff Hostetler \nSigned-off-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n parallel-checkout.c | 4 +\n t/lib-parallel-checkout.sh | 4 +-\n t/t2081-parallel-checkout-collisions.sh | 162 ++++++++++++++++++++++++\n 3 files changed, 168 insertions(+), 2 deletions(-)\n create mode 100755 t/t2081-parallel-checkout-collisions.sh\n\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 09e8b10a35..6fb3f1e6c9 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -8,6 +8,7 @@\n #include \"sigchain.h\"\n #include \"streaming.h\"\n #include \"thread-utils.h\"\n+#include \"trace2.h\"\n \n struct pc_worker {\n \tstruct child_process cp;\n@@ -326,6 +327,7 @@ void write_pc_item(struct parallel_checkout_item *pc_item,\n \tif (dir_sep && !has_dirs_only_path(path.buf, dir_sep - path.buf,\n \t\t\t\t\t state->base_dir_len)) {\n \t\tpc_item->status = PC_ITEM_COLLIDED;\n+\t\ttrace2_data_string(\"pcheckout\", NULL, \"collision/dirname\", path.buf);\n \t\tgoto out;\n \t}\n \n@@ -341,6 +343,8 @@ void write_pc_item(struct parallel_checkout_item *pc_item,\n \t\t\t * call should have already caught these cases.\n \t\t\t */\n \t\t\tpc_item->status = PC_ITEM_COLLIDED;\n+\t\t\ttrace2_data_string(\"pcheckout\", NULL,\n+\t\t\t\t\t \"collision/basename\", path.buf);\n \t\t} else {\n \t\t\terror_errno(\"failed to open file '%s'\", path.buf);\n \t\t\tpc_item->status = PC_ITEM_FAILED;\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex 39fd36fdf6..16ee18389b 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -21,12 +21,12 @@ test_checkout_workers () {\n \tshift &&\n \n \trm -f trace &&\n-\tGIT_TRACE2=\"$(pwd)/trace\" \"$@\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace\" \"$@\" 2>&8 &&\n \n \tworkers=$(grep \"child_start\\[..*\\] git checkout--worker\" trace | wc -l) &&\n \ttest $workers -eq $expected_workers &&\n \trm -f trace\n-}\n+} 8>&2 2>&4\n \n # Verify that both the working tree and the index were created correctly\n verify_checkout () {\ndiff --git a/t/t2081-parallel-checkout-collisions.sh b/t/t2081-parallel-checkout-collisions.sh\nnew file mode 100755\nindex 0000000000..f6fcfc0c1e\n--- /dev/null\n+++ b/t/t2081-parallel-checkout-collisions.sh\n@@ -0,0 +1,162 @@\n+#!/bin/sh\n+\n+test_description=\"path collisions during parallel checkout\n+\n+Parallel checkout must detect path collisions to:\n+\n+1) Avoid racily writing to different paths that represent the same file on disk.\n+2) Report the colliding entries on clone.\n+\n+The tests in this file exercise parallel checkout's collision detection code in\n+both these mechanics.\n+\"\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+\n+TEST_ROOT=\"$PWD\"\n+\n+test_expect_success CASE_INSENSITIVE_FS 'setup' '\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t100644 $empty_oid\tFILE_X\n+\t100644 $empty_oid\tFILE_x\n+\t100644 $empty_oid\tfile_X\n+\t100644 $empty_oid\tfile_x\n+\tEOF\n+\tgit update-index --index-info >filter.log\n+\tEOF\n+'\n+\n+test_workers_in_event_trace ()\n+{\n+\ttest $1 -eq $(grep \".event.:.child_start..*checkout--worker\" $2 | wc -l)\n+}\n+\n+test_expect_success CASE_INSENSITIVE_FS 'worker detects basename collision' '\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git \\\n+\t\t-c checkout.workers=2 -c checkout.thresholdForParallelism=0 \\\n+\t\tcheckout . &&\n+\n+\ttest_workers_in_event_trace 2 trace &&\n+\tcollisions=$(grep -i \"category.:.pcheckout.,.key.:.collision/basename.,.value.:.file_x.}\" trace | wc -l) &&\n+\ttest $collisions -eq 3\n+'\n+\n+test_expect_success CASE_INSENSITIVE_FS 'worker detects dirname collision' '\n+\ttest_config filter.logger.smudge \"\\\"$TEST_ROOT/logger_script\\\" %f\" &&\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t100644 $empty_oid\tA/B\n+\t100644 $empty_oid\tA/C\n+\t100644 $empty_oid\ta\n+\t100644 $attr_oid\t.gitattributes\n+\tEOF\n+\tgit rm -rf . &&\n+\tgit update-index --index-info expected.log &&\n+\ttest_cmp filter.log expected.log &&\n+\n+\t# Check that it used the right number of workers and detected the collisions\n+\ttest_workers_in_event_trace 2 trace &&\n+\tgrep \"category.:.pcheckout.,.key.:.collision/dirname.,.value.:.A/B.}\" trace &&\n+\tgrep \"category.:.pcheckout.,.key.:.collision/dirname.,.value.:.A/C.}\" trace\n+'\n+\n+test_expect_success SYMLINKS,CASE_INSENSITIVE_FS 'do not follow symlinks colliding with leading dir' '\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t120000 $symlink_oid\tD\n+\t100644 $empty_oid\td/x\n+\t100644 $empty_oid\te/y\n+\tEOF\n+\tgit rm -rf . &&\n+\tgit update-index --index-info stderr &&\n+\n+\tgrep FILE_X stderr &&\n+\tgrep FILE_x stderr &&\n+\tgrep file_X stderr &&\n+\tgrep file_x stderr &&\n+\tgrep \"the following paths have collided\" stderr\n+'\n+\n+# This test ensures that the collision report code is correctly looking for\n+# colliding peers in the second half of the cache_entry array. This is done by\n+# defining a smudge command for the *last* array entry, which makes it\n+# non-eligible for parallel-checkout. Thus, it is checked out *first*, before\n+# spawning the workers.\n+#\n+# Note: this test doesn't work on Windows because, on this system, the\n+# collision report code uses strcmp() to find the colliding pairs when\n+# core.ignoreCase is false. And we need this setting for this test so that only\n+# 'file_x' matches the pattern of the filter attribute. But the test works on\n+# OSX, where the colliding pairs are found using inode.\n+#\n+test_expect_success CASE_INSENSITIVE_FS,!MINGW,!CYGWIN \\\n+\t'collision report on clone (w/ colliding peer after the detected entry)' '\n+\n+\ttest_config_global filter.logger.smudge \"\\\"$TEST_ROOT/logger_script\\\" %f\" &&\n+\tgit reset --hard basename_collision &&\n+\techo \"file_x filter=logger\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\tgit commit -m \"filter for file_x\" &&\n+\n+\trm -rf clone-repo &&\n+\tset_checkout_config 2 0 &&\n+\ttest_checkout_workers 2 \\\n+\t\tgit -c core.ignoreCase=false clone . clone-repo 2>stderr &&\n+\n+\tgrep FILE_X stderr &&\n+\tgrep FILE_x stderr &&\n+\tgrep file_X stderr &&\n+\tgrep file_x stderr &&\n+\tgrep \"the following paths have collided\" stderr &&\n+\n+\t# Check that only \"file_x\" was filtered\n+\techo file_x >expected.log &&\n+\ttest_cmp clone-repo/filter.log expected.log\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"422689","messageId":"3a0845fdbb0481ead96b15be472df636bcf73aee.1619104091.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619104091.git.matheus.bernardino@usp.br","subject":"[PATCH 6/7] parallel-checkout: add tests related to .gitattributes","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-22T15:17:52Z","receivedAt":"2021-04-22T15:18:44Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to confirm that the `struct conv_attrs` data is correctly\npassed from the main process to the workers, and that they can properly\nconvert the blobs before writing them to the working tree.\n\nAlso check that parallel-ineligible entries, such as regular files that\nrequire external filters, are correctly smudge and written when\nparallel-checkout is enabled.\n\nNote: to avoid repeating code, some helper functions are extracted from\nt0028 into a common lib file.\n\nOriginal-patch-by: Jeff Hostetler \nSigned-off-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n t/lib-encoding.sh | 25 +++\n t/t0028-working-tree-encoding.sh | 25 +--\n t/t2082-parallel-checkout-attributes.sh | 194 ++++++++++++++++++++++++\n 3 files changed, 220 insertions(+), 24 deletions(-)\n create mode 100644 t/lib-encoding.sh\n create mode 100755 t/t2082-parallel-checkout-attributes.sh\n\ndiff --git a/t/lib-encoding.sh b/t/lib-encoding.sh\nnew file mode 100644\nindex 0000000000..c52ffbbed5\n--- /dev/null\n+++ b/t/lib-encoding.sh\n@@ -0,0 +1,25 @@\n+# Encoding helpers used by t0028 and t2082\n+\n+test_lazy_prereq NO_UTF16_BOM '\n+\ttest $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6\n+'\n+\n+test_lazy_prereq NO_UTF32_BOM '\n+\ttest $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12\n+'\n+\n+write_utf16 () {\n+\tif test_have_prereq NO_UTF16_BOM\n+\tthen\n+\t\tprintf '\\376\\377'\n+\tfi &&\n+\ticonv -f UTF-8 -t UTF-16\n+}\n+\n+write_utf32 () {\n+\tif test_have_prereq NO_UTF32_BOM\n+\tthen\n+\t\tprintf '\\0\\0\\376\\377'\n+\tfi &&\n+\ticonv -f UTF-8 -t UTF-32\n+}\ndiff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\nindex f970a9806b..82905a2156 100755\n--- a/t/t0028-working-tree-encoding.sh\n+++ b/t/t0028-working-tree-encoding.sh\n@@ -6,33 +6,10 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-encoding.sh\"\n \n GIT_TRACE_WORKING_TREE_ENCODING=1 && export GIT_TRACE_WORKING_TREE_ENCODING\n \n-test_lazy_prereq NO_UTF16_BOM '\n-\ttest $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6\n-'\n-\n-test_lazy_prereq NO_UTF32_BOM '\n-\ttest $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12\n-'\n-\n-write_utf16 () {\n-\tif test_have_prereq NO_UTF16_BOM\n-\tthen\n-\t\tprintf '\\376\\377'\n-\tfi &&\n-\ticonv -f UTF-8 -t UTF-16\n-}\n-\n-write_utf32 () {\n-\tif test_have_prereq NO_UTF32_BOM\n-\tthen\n-\t\tprintf '\\0\\0\\376\\377'\n-\tfi &&\n-\ticonv -f UTF-8 -t UTF-32\n-}\n-\n test_expect_success 'setup test files' '\n \tgit config core.eol lf &&\n \ndiff --git a/t/t2082-parallel-checkout-attributes.sh b/t/t2082-parallel-checkout-attributes.sh\nnew file mode 100755\nindex 0000000000..2525457961\n--- /dev/null\n+++ b/t/t2082-parallel-checkout-attributes.sh\n@@ -0,0 +1,194 @@\n+#!/bin/sh\n+\n+test_description='parallel-checkout: attributes\n+\n+Verify that parallel-checkout correctly creates files that require\n+conversions, as specified in .gitattributes. The main point here is\n+to check that the conv_attr data is correctly sent to the workers\n+and that it contains sufficient information to smudge files\n+properly (without access to the index or attribute stack).\n+'\n+\n+TEST_NO_CREATE_REPO=1\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+. \"$TEST_DIRECTORY/lib-encoding.sh\"\n+\n+test_expect_success 'parallel-checkout with ident' '\n+\tset_checkout_config 2 0 &&\n+\tgit init ident &&\n+\t(\n+\t\tcd ident &&\n+\t\techo \"A ident\" >.gitattributes &&\n+\t\techo \"\\$Id\\$\" >A &&\n+\t\techo \"\\$Id\\$\" >B &&\n+\t\tgit add -A &&\n+\t\tgit commit -m id &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git reset --hard &&\n+\t\thexsz=$(test_oid hexsz) &&\n+\t\tgrep -E \"\\\\\\$Id: [0-9a-f]{$hexsz} \\\\\\$\" A &&\n+\t\tgrep \"\\\\\\$Id\\\\\\$\" B\n+\t)\n+'\n+\n+test_expect_success 'parallel-checkout with re-encoding' '\n+\tset_checkout_config 2 0 &&\n+\tgit init encoding &&\n+\t(\n+\t\tcd encoding &&\n+\t\techo text >utf8-text &&\n+\t\twrite_utf16 utf16-text &&\n+\n+\t\techo \"A working-tree-encoding=UTF-16\" >.gitattributes &&\n+\t\tcp utf16-text A &&\n+\t\tcp utf8-text B &&\n+\t\tgit add A B .gitattributes &&\n+\t\tgit commit -m encoding &&\n+\n+\t\t# Check that A is stored in UTF-8\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp_bin utf8-text A.internal &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git checkout A B &&\n+\n+\t\t# Check that A (and only A) is re-encoded during checkout\n+\t\ttest_cmp_bin utf16-text A &&\n+\t\ttest_cmp_bin utf8-text B\n+\t)\n+'\n+\n+test_expect_success 'parallel-checkout with eol conversions' '\n+\tset_checkout_config 2 0 &&\n+\tgit init eol &&\n+\t(\n+\t\tcd eol &&\n+\t\tprintf \"multi\\r\\nline\\r\\ntext\" >crlf-text &&\n+\t\tprintf \"multi\\nline\\ntext\" >lf-text &&\n+\n+\t\tgit config core.autocrlf false &&\n+\t\techo \"A eol=crlf\" >.gitattributes &&\n+\t\tcp crlf-text A &&\n+\t\tcp lf-text B &&\n+\t\tgit add A B .gitattributes &&\n+\t\tgit commit -m eol &&\n+\n+\t\t# Check that A is stored with LF format\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp_bin lf-text A.internal &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git checkout A B &&\n+\n+\t\t# Check that A (and only A) is converted to CRLF during checkout\n+\t\ttest_cmp_bin crlf-text A &&\n+\t\ttest_cmp_bin lf-text B\n+\t)\n+'\n+\n+# Entries that require an external filter are not eligible for parallel\n+# checkout. Check that both the parallel-eligible and non-eligible entries are\n+# properly writen in a single checkout operation.\n+#\n+test_expect_success 'parallel-checkout and external filter' '\n+\tset_checkout_config 2 0 &&\n+\tgit init filter &&\n+\t(\n+\t\tcd filter &&\n+\t\twrite_script <<-\\EOF rot13.sh &&\n+\t\ttr \\\n+\t\t \"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ\" \\\n+\t\t \"nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM\"\n+\t\tEOF\n+\n+\t\tgit config filter.rot13.clean \"\\\"$(pwd)/rot13.sh\\\"\" &&\n+\t\tgit config filter.rot13.smudge \"\\\"$(pwd)/rot13.sh\\\"\" &&\n+\t\tgit config filter.rot13.required true &&\n+\n+\t\techo abcd >original &&\n+\t\techo nopq >rot13 &&\n+\n+\t\techo \"A filter=rot13\" >.gitattributes &&\n+\t\tcp original A &&\n+\t\tcp original B &&\n+\t\tcp original C &&\n+\t\tgit add A B C .gitattributes &&\n+\t\tgit commit -m filter &&\n+\n+\t\t# Check that A (and only A) was cleaned\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp rot13 A.internal &&\n+\t\tgit cat-file -p :B >B.internal &&\n+\t\ttest_cmp original B.internal &&\n+\t\tgit cat-file -p :C >C.internal &&\n+\t\ttest_cmp original C.internal &&\n+\n+\t\trm A B C *.internal &&\n+\t\ttest_checkout_workers 2 git checkout A B C &&\n+\n+\t\t# Check that A (and only A) was smudged during checkout\n+\t\ttest_cmp original A &&\n+\t\ttest_cmp original B &&\n+\t\ttest_cmp original C\n+\t)\n+'\n+\n+# The delayed queue is independent from the parallel queue, and they should be\n+# able to work together in the same checkout process.\n+#\n+test_expect_success PERL 'parallel-checkout and delayed checkout' '\n+\twrite_script rot13-filter.pl \"$PERL_PATH\" \\\n+\t\t<\"$TEST_DIRECTORY\"/t0021/rot13-filter.pl &&\n+\n+\ttest_config_global filter.delay.process \\\n+\t\t\"\\\"$(pwd)/rot13-filter.pl\\\" --always-delay \\\"$(pwd)/delayed.log\\\" clean smudge delay\" &&\n+\ttest_config_global filter.delay.required true &&\n+\n+\techo \"abcd\" >original &&\n+\techo \"nopq\" >rot13 &&\n+\n+\tgit init delayed &&\n+\t(\n+\t\tcd delayed &&\n+\t\techo \"*.d filter=delay\" >.gitattributes &&\n+\t\tcp ../original W.d &&\n+\t\tcp ../original X.d &&\n+\t\tcp ../original Y &&\n+\t\tcp ../original Z &&\n+\t\tgit add -A &&\n+\t\tgit commit -m delayed &&\n+\n+\t\t# Check that *.d files were cleaned\n+\t\tgit cat-file -p :W.d >W.d.internal &&\n+\t\ttest_cmp W.d.internal ../rot13 &&\n+\t\tgit cat-file -p :X.d >X.d.internal &&\n+\t\ttest_cmp X.d.internal ../rot13 &&\n+\t\tgit cat-file -p :Y >Y.internal &&\n+\t\ttest_cmp Y.internal ../original &&\n+\t\tgit cat-file -p :Z >Z.internal &&\n+\t\ttest_cmp Z.internal ../original &&\n+\n+\t\trm *\n+\t) &&\n+\n+\tset_checkout_config 2 0 &&\n+\ttest_checkout_workers 2 git -C delayed checkout -f &&\n+\tverify_checkout delayed &&\n+\n+\t# Check that the *.d files got to the delay queue and were filtered\n+\tgrep \"smudge W.d .* \\[DELAYED\\]\" delayed.log &&\n+\tgrep \"smudge X.d .* \\[DELAYED\\]\" delayed.log &&\n+\ttest_cmp delayed/W.d original &&\n+\ttest_cmp delayed/X.d original &&\n+\n+\t# Check that the parallel-eligible entries went to the right queue and\n+\t# were not filtered\n+\t! grep \"smudge Y .* \\[DELAYED\\]\" delayed.log &&\n+\t! grep \"smudge Z .* \\[DELAYED\\]\" delayed.log &&\n+\ttest_cmp delayed/Y original &&\n+\ttest_cmp delayed/Z original\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"422690","messageId":"f7c20e0b5bd7857b2303aab762f24c503e50030d.1619104091.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619104091.git.matheus.bernardino@usp.br","subject":"[PATCH 7/7] ci: run test round with parallel-checkout enabled","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-22T15:17:53Z","receivedAt":"2021-04-22T15:18:52Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"We already have tests for the basic parallel-checkout operations. But\nthis code can also run be executed by other commands, such as\ngit-read-tree and git-sparse-checkout, which are currently not tested\nwith multiple workers. To promote a wider test coverage without\nduplicating tests:\n\n1. Add the GIT_TEST_CHECKOUT_WORKERS environment variable, to optionally\n force parallel-checkout execution during the whole test suite.\n\n2. Set this variable (with a value of 2) in the second test round of our\n linux-gcc CI job. This round runs `make test` again with some\n optional GIT_TEST_* variables enabled, so there is no additional\n overhead in exercising the parallel-checkout code here.\n\nNote that tests checking out less than two parallel-eligible entries\nwill fall back to the sequential mode. Nevertheless, it's still a good\nexercise for the parallel-checkout framework as the fallback codepath\nalso writes the queued entries using the parallel-checkout functions\n(only without spawning any worker).\n\nSigned-off-by: Matheus Tavares \n---\n ci/run-build-and-tests.sh | 1 +\n parallel-checkout.c | 14 ++++++++++++++\n t/README | 4 ++++\n t/lib-parallel-checkout.sh | 3 +++\n 4 files changed, 22 insertions(+)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex a66b5e8c75..23b28e7391 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -25,6 +25,7 @@ linux-gcc)\n \texport GIT_TEST_ADD_I_USE_BUILTIN=1\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n \texport GIT_TEST_WRITE_REV_INDEX=1\n+\texport GIT_TEST_CHECKOUT_WORKERS=2\n \tmake test\n \t;;\n linux-clang)\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 6fb3f1e6c9..6b1af32bb3 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -35,6 +35,20 @@ static const int DEFAULT_NUM_WORKERS = 1;\n \n void get_parallel_checkout_configs(int *num_workers, int *threshold)\n {\n+\tchar *env_workers = getenv(\"GIT_TEST_CHECKOUT_WORKERS\");\n+\n+\tif (env_workers && *env_workers) {\n+\t\tif (strtol_i(env_workers, 10, num_workers)) {\n+\t\t\tdie(\"invalid value for GIT_TEST_CHECKOUT_WORKERS: '%s'\",\n+\t\t\t env_workers);\n+\t\t}\n+\t\tif (*num_workers < 1)\n+\t\t\t*num_workers = online_cpus();\n+\n+\t\t*threshold = 0;\n+\t\treturn;\n+\t}\n+\n \tif (git_config_get_int(\"checkout.workers\", num_workers))\n \t\t*num_workers = DEFAULT_NUM_WORKERS;\n \telse if (*num_workers < 1)\ndiff --git a/t/README b/t/README\nindex fd9375b146..a194488f27 100644\n--- a/t/README\n+++ b/t/README\n@@ -436,6 +436,10 @@ and \"sha256\".\n GIT_TEST_WRITE_REV_INDEX=, when true enables the\n 'pack.writeReverseIndex' setting.\n \n+GIT_TEST_CHECKOUT_WORKERS= overrides the 'checkout.workers' setting\n+to and 'checkout.thresholdForParallelism' to 0, forcing the\n+execution of the parallel-checkout code.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex 16ee18389b..0921a32b96 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -1,5 +1,8 @@\n # Helpers for t208* tests\n \n+# Parallel checkout tests need full control of the number of workers\n+unset GIT_TEST_CHECKOUT_WORKERS\n+\n set_checkout_config () {\n \tif test $# -ne 2\n \tthen\n-- \n2.30.1\n\n"},{"id":"422763","messageId":"058f6ca4-a575-a463-e29b-9071c35e5bd3@gmail.com","threadId":"55540","inReplyTo":"5e0dee7beba083159f4277ddcd9e931859239bde.1619104091.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 2/7] builtin/checkout.c: complete parallel checkout support","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-23T16:19:06Z","receivedAt":"2021-04-23T16:19:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/22/2021 11:17 AM, Matheus Tavares wrote:\n> There is one code path in builtin/checkout.c which still doesn't benefit\n> from parallel checkout because it calls checkout_entry() directly,> instead of unpack_trees(). Let's add parallel checkout support for this\n> missing spot as well.\n\nI couldn't tell immediately from the patch what would trigger this\ncode path. I had to trace the method calls to discover that it is\nfor the case of a pathspec-limited checkout:\n\n\tgit checkout -- \n\nI confirmed that this does work with this change, but it might be\nnice to have a test that verifies that parallelism is triggering for\nthis case.\n\nLooking ahead to patches 4-6, which add tests, I do not see one for this\ncode path. Yes, patch 7 will implicitly test it through optional\nsettings, but it would be nice to verify that the code is actually using\nparallel workers. The test_checkout_workers helper in patch 4 should be\nhelpful for this effort.\n\nPlease point out the test that covers this case, in case I'm just not\nseeing it.\n\nThe good news is that I can see a difference. By alternating checkouts\nof the Git repository's \"t\" directory between v2.20 and v2.31.1, I can\nsee these results for varying numbers of workers:\n\nBenchmark #1: 16 workers\n Time (mean ± σ): 108.6 ms ± 5.2 ms [User: 146.1 ms, System: 146.1 ms]\n Range (min … max): 95.5 ms … 124.9 ms 100 runs\n \nBenchmark #2: 8 workers\n Time (mean ± σ): 104.8 ms ± 4.8 ms [User: 128.3 ms, System: 131.7 ms]\n Range (min … max): 94.2 ms … 119.0 ms 100 runs\n \nBenchmark #3: 4 workers\n Time (mean ± σ): 112.3 ms ± 6.2 ms [User: 114.6 ms, System: 112.1 ms]\n Range (min … max): 100.0 ms … 127.4 ms 100 runs\n \nBenchmark #4: 2 workers\n Time (mean ± σ): 124.2 ms ± 4.2 ms [User: 106.5 ms, System: 102.0 ms]\n Range (min … max): 114.8 ms … 136.3 ms 100 runs\n \nBenchmark #5: sequential\n Time (mean ± σ): 154.6 ms ± 6.7 ms [User: 83.5 ms, System: 79.4 ms]\n Range (min … max): 142.1 ms … 176.0 ms 100 runs\n \nSummary\n '8 workers' ran\n 1.04 ± 0.07 times faster than '16 workers'\n 1.07 ± 0.08 times faster than '4 workers'\n 1.19 ± 0.07 times faster than '2 workers'\n 1.48 ± 0.09 times faster than 'sequential'\n\n(Note: these time measurements are for the round-trip of two checkout\ncommands.)\n> @@ -359,16 +360,22 @@ static int checkout_worktree(const struct checkout_opts *opts,\n> \tint nr_checkouts = 0, nr_unmerged = 0;\n> \tint errs = 0;\n> \tint pos;\n> +\tint pc_workers, pc_threshold;\n> +\tstruct mem_pool ce_mem_pool;\n> \n> \tstate.force = 1;\n> \tstate.refresh_cache = 1;\n> \tstate.istate = &the_index;\n> \n> +\tmem_pool_init(&ce_mem_pool, 0);\n> +\tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n> \tinit_checkout_metadata(&state.meta, info->refname,\n> \t\t\t info->commit ? &info->commit->object.oid : &info->oid,\n> \t\t\t NULL);\n> \n> \tenable_delayed_checkout(&state);\n> +\tif (pc_workers > 1)\n> +\t\tinit_parallel_checkout();\n\nI'm late to looking at your parallel checkout work, but I find this\nto be a really nice API to get things initialized.\n\n\n> \t\t\telse if (opts->merge)\n> \t\t\t\terrs |= checkout_merged(pos, &state,\n> -\t\t\t\t\t\t\t&nr_unmerged);\n> +\t\t\t\t\t\t\t&nr_unmerged,\n> +\t\t\t\t\t\t\t&ce_mem_pool);\n\nI see the need to populate these merged entries in the pool for now,\nbefore...\n\n> \t\t\tpos = skip_same_name(ce, pos) - 1;\n> \t\t}\n> \t}\n> +\tif (pc_workers > 1)\n> +\t\terrs |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n> +\t\t\t\t\t NULL, NULL);\n> +\tmem_pool_discard(&ce_mem_pool, should_validate_cache_entries());\n...now running the parallel checkout and discarding the entries.\n\nThanks,\n-Stolee\n"},{"id":"422770","messageId":"36615f6a-ce58-3737-8c1d-fa1253a85439@gmail.com","threadId":"55540","inReplyTo":"6bf1db2fd8e807f9f259787ef75651d43b61e03a.1619104091.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 3/7] checkout-index: add parallel checkout support","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-23T18:32:37Z","receivedAt":"2021-04-23T18:32:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/22/2021 11:17 AM, Matheus Tavares wrote:\n> @@ -142,11 +143,7 @@ static void checkout_all(const char *prefix, int prefix_length)\n> \t}\n> \tif (last_ce && to_tempfile)\n> \t\twrite_tempfile_record(last_ce->name, prefix);\n> -\tif (errs)\n> -\t\t/* we have already done our error reporting.\n> -\t\t * exit with the same code as die().\n> -\t\t */\n> -\t\texit(128);\n\nHere, it makes note of returning 128 as if it were a die(), but\n\n> +\tif (all)\n> +\t\terr |= checkout_all(prefix, prefix_length);\n> +\n> +\tif (pc_workers > 1)\n> +\t\terr |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n> +\t\t\t\t\t NULL, NULL);\n> +\n> \tif (err)\n> \t\treturn 1;\n\nThis returns 1 instead. Should we `return err` and use an error\ncode specific to the response? I imagine there are other reasons\nthat could cause a non-zero return for checkout_all() and\nrun_parallel_checkout().\n\nI suppose: is there a value in persisting the 128 response code\nhere, or is an exit of 1 sufficient? There is no documentation\nabout the exit code, so any dependence on 128 is not based on a\n\"written promise\" but I thought it was worth mentioning.\n\nThanks,\n-Stolee\n"},{"id":"422773","messageId":"1b1cdef5-7d90-c6f3-ea8d-e1c9d472ffff@gmail.com","threadId":"55540","inReplyTo":"6379b8df6a59361dd44733e379880a11c6cd977c.1619104091.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 4/7] parallel-checkout: add tests for basic operations","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-23T19:18:14Z","receivedAt":"2021-04-23T19:18:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/22/2021 11:17 AM, Matheus Tavares wrote:\n> Add tests to populate the working tree during clone and checkout using\n> sequential and parallel mode, to confirm that they produce identical\n> results. Also test basic checkout mechanics, such as checking for\n> symlinks in the leading directories and the abidance to --force.\n> \n> Note: some helper functions are added to a common lib file which is only\n> included by t2080 for now. But they will also be used by other\n> parallel-checkout tests in the following patches.\n> \n> Original-patch-by: Jeff Hostetler \n\nIs this a standard thing? Or, did you change the patch significantly\nenough that \"Co-authored-by:\" is no longer appropriate?\n\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -0,0 +1,37 @@\n> +# Helpers for t208* tests\n\nI could see tests outside of the t208* range possibly having\nvalue in interacting with parallel checkout in the future.\n\nPerhaps:\n\n\t# Helpers for tests invoking parallel-checkout\n\n> +\n> +set_checkout_config () {\n> +\tif test $# -ne 2\n> +\tthen\n> +\t\tBUG \"set_checkout_config() requires two arguments\"\n> +\tfi &&\n\nA usage comment is typically helpful for these helpers:\n\n# set_checkout_config \n# sets global config values to use the given number of\n# workers when the given threshold is met.\n\n> +\n> +\ttest_config_global checkout.workers $1 &&\n> +\ttest_config_global checkout.thresholdForParallelism $2\n> +}\n> +\n> +# Run \"${@:2}\" and check that $1 checkout workers were used\n> +test_checkout_workers () {\n\nThis simpler doc style works, too.\n\n> +\tif test $# -lt 2\n> +\tthen\n> +\t\tBUG \"too few arguments to test_checkout_workers()\"\n> +\tfi &&\n> +\n> +\texpected_workers=$1 &&\n> +\tshift &&\n> +\n> +\trm -f trace &&\n> +\tGIT_TRACE2=\"$(pwd)/trace\" \"$@\" &&\n> +\n> +\tworkers=$(grep \"child_start\\[..*\\] git checkout--worker\" trace | wc -l) &&\n> +\ttest $workers -eq $expected_workers &&\n> +\trm -f trace\n\nI wonder if this should be a \"test_when_finished rm -f trace\" being\nrecorded earlier in the step. It would also benefit from using a more\nspecific name than \"trace\". Something like \"trace-test-checkout-workers\"\nwould be unlikely to collide with someone else's trace.\n\n> +# Verify that both the working tree and the index were created correctly\n> +verify_checkout () {\n\nAdd usage of a repo in $1?\n\n> +\tgit -C \"$1\" diff-index --quiet HEAD -- &&\n> +\tgit -C \"$1\" diff-index --quiet --cached HEAD -- &&\n> +\tgit -C \"$1\" status --porcelain >\"$1\".status &&\n> +\ttest_must_be_empty \"$1\".status\n> +}\n\n\n> +TEST_NO_CREATE_REPO=1\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n> +\n> +# Test parallel-checkout with a branch switch containing file creations,\n> +# deletions, and modification; with different entry types. Switching from B1 to\n> +# B2 will have the following changes:\n> +#\n> +# - a (file): modified\n> +# - e/x (file): deleted\n> +# - b (symlink): deleted\n> +# - b/f (file): created\n> +# - e (symlink): created\n> +# - d (submodule): created\n> +#\n\nAn interesting set of changes. What about a directory/file conflict?\n\nSomething like this might be useful:\n\n # - f/t (file): deleted\n # - f (file): created\n\nin fact, it could be interesting to have a file conflict with each\nof these types, such as the symlink 'e' and the submodule 'd'.\n\nWhile we are at it, what about a symlink/submodule conflict? I know\nit makes the test bigger, but doing everything simultaneously through\na carefully designed repository helps prevent test case bloat.\n\n> +test_expect_success SYMLINKS 'setup repo for checkout with various types of changes' '\n\nCould we split this setup step into two parts? Once could\nset up everything except the symlinks and would not require\nthe SYMLINKS prereq. We could then have another test with\nthe SYMLINKS prereq that extends B1 and B2 to have symlinks\nand their conflicts. The remaining tests would work on any\nplatform without needing the SYMLINKS prereq.\n\n> +test_expect_success SYMLINKS 'sequential checkout' '\n> +\tcp -R various various_sequential &&\n> +\tset_checkout_config 1 0 &&\n> +\ttest_checkout_workers 0 \\\n> +\t\tgit -C various_sequential checkout --recurse-submodules B2 &&\n> +\tverify_checkout various_sequential\n> +'\n\nI see all these tests are very similar. Perhaps group\nthem to demonstrate their differences?\n\nparallel_test_case () {\n\ttest_expect_success \"$1\" \"\n\t\tcp -R various $2 &&\n\t\tset_checkout_config $3 $4 &&\n\t\ttest_checkout_workers $5 \\\n\t\t\tgit -C $2 checkout --recurse-submodules B2 &&\n\t\tverify_checkout $2\n\t\"\n}\n\nparallel_test_case 'sequential checkout' \\\n\tvarious_sequential 1 0 0\nparallel_test_case 'parallel checkout' \\\n\tvarious_parallel 2 0 2\nparallel_test_case 'fallback to sequential checkout (threshold)' \\\n\tvarious_sequential_fallback 2 100 0\n\n> +test_expect_success SYMLINKS 'parallel checkout on clone' '\n> +\tgit -C various checkout --recurse-submodules B2 &&\n> +\tset_checkout_config 2 0 &&\n> +\ttest_checkout_workers 2 \\\n> +\t\tgit clone --recurse-submodules various various_parallel_clone &&\n> +\tverify_checkout various_parallel_clone\n> +'\n> +\n> +test_expect_success SYMLINKS 'fallback to sequential checkout on clone (threshold)' '\n> +\tgit -C various checkout --recurse-submodules B2 &&\n> +\tset_checkout_config 2 100 &&\n> +\ttest_checkout_workers 0 \\\n> +\t\tgit clone --recurse-submodules various various_sequential_fallback_clone &&\n> +\tverify_checkout various_sequential_fallback_clone\n> +'\n\nDoing a similar grouping for the clone case might be interesting,\nif only for the possible future where more customization might be\nnecessary.\n\nSince the clone case is only caring about the contents at B2, it\nis good that B2 contains one of each type of entry.\n\n> +# Just to be paranoid, actually compare the working trees' contents directly.\n> +test_expect_success SYMLINKS 'compare the working trees' '\n> +\trm -rf various_*/.git &&\n> +\trm -rf various_*/d/.git &&\n> +\n> +\tdiff -r various_sequential various_parallel &&\n> +\tdiff -r various_sequential various_sequential_fallback &&\n> +\tdiff -r various_sequential various_parallel_clone &&\n> +\tdiff -r various_sequential various_sequential_fallback_clone\n> +'\n> +\n> +test_expect_success 'parallel checkout respects --[no]-force' '\n> +\tset_checkout_config 2 0 &&\n> +\tgit init dirty &&\n> +\t(\n> +\t\tcd dirty &&\n> +\t\tmkdir D &&\n> +\t\ttest_commit D/F &&\n> +\t\ttest_commit F &&\n> +\n> +\t\trm -rf D &&\n> +\t\techo changed >D &&\n> +\t\techo changed >F.t &&\n> +\n> +\t\t# We expect 0 workers because there is nothing to be done\n> +\t\ttest_checkout_workers 0 git checkout HEAD &&\n> +\t\ttest_path_is_file D &&\n> +\t\tgrep changed D &&\n> +\t\tgrep changed F.t &&\n> +\n> +\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n> +\t\ttest_path_is_dir D &&\n> +\t\tgrep D/F D/F.t &&\n> +\t\tgrep F F.t\n> +\t)\n> +'\n\nI see SYMLINKS is not necessary here due to creating a new repo.\nStill better to not have the prereq when not necessary.\n\n> +test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading dirs' '\n> +\tset_checkout_config 2 0 &&\n> +\tgit init symlinks &&\n> +\t(\n> +\t\tcd symlinks &&\n> +\t\tmkdir D untracked &&\n> +\t\t# Commit 2 files to have enough work for 2 parallel workers\n> +\t\ttest_commit D/A &&\n> +\t\ttest_commit D/B &&\n> +\t\trm -rf D &&\n> +\t\tln -s untracked D &&\n> +\n> +\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n> +\t\t! test -h D &&\n> +\t\tgrep D/A D/A.t &&\n> +\t\tgrep D/B D/B.t\n> +\t)\n> +'\n\nThis test can continue to require SYMLINKS.\n\nThanks,\n-Stolee\n"},{"id":"422805","messageId":"4ded5b1e-5936-8a45-ea4b-ebe8bce27a0b@gmail.com","threadId":"55540","inReplyTo":"3a0845fdbb0481ead96b15be472df636bcf73aee.1619104091.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 6/7] parallel-checkout: add tests related to .gitattributes","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-23T19:48:49Z","receivedAt":"2021-04-23T19:48:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/22/2021 11:17 AM, Matheus Tavares wrote:\n> Add tests to confirm that the `struct conv_attrs` data is correctly\n> passed from the main process to the workers, and that they can properly\n> convert the blobs before writing them to the working tree.\n> \n> Also check that parallel-ineligible entries, such as regular files that\n> require external filters, are correctly smudge and written when\n> parallel-checkout is enabled.\n> \n> Note: to avoid repeating code, some helper functions are extracted from\n> t0028 into a common lib file.\n\nnit: this movement of helper functions could be its own patch to reduce\ncognitive load when already reading a very detailed set of tests.\n\n> diff --git a/t/t2082-parallel-checkout-attributes.sh b/t/t2082-parallel-checkout-attributes.sh\n> new file mode 100755\n> index 0000000000..2525457961\n> --- /dev/null\n> +++ b/t/t2082-parallel-checkout-attributes.sh\n> @@ -0,0 +1,194 @@\n> +#!/bin/sh\n> +\n> +test_description='parallel-checkout: attributes\n> +\n> +Verify that parallel-checkout correctly creates files that require\n> +conversions, as specified in .gitattributes. The main point here is\n> +to check that the conv_attr data is correctly sent to the workers\n> +and that it contains sufficient information to smudge files\n> +properly (without access to the index or attribute stack).\n> +'\n\nThe tests themselves are quite dense, but definitely appear to be\ndoing valuable cases to cover an important feature of parallel\ncheckout. Each one is so different from the other that I can't\nfind ways to reorganize them for better readability.\n\nThanks,\n-Stolee\n \n"},{"id":"422806","messageId":"56757f2b-f203-3014-dbad-e86c49d0d64b@gmail.com","threadId":"55540","inReplyTo":"f7c20e0b5bd7857b2303aab762f24c503e50030d.1619104091.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 7/7] ci: run test round with parallel-checkout enabled","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-23T19:56:53Z","receivedAt":"2021-04-23T19:56:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/22/2021 11:17 AM, Matheus Tavares wrote:\n> We already have tests for the basic parallel-checkout operations. But\n> this code can also run be executed by other commands, such as\n> git-read-tree and git-sparse-checkout, which are currently not tested\n> with multiple workers. To promote a wider test coverage without\n> duplicating tests:\n> \n> 1. Add the GIT_TEST_CHECKOUT_WORKERS environment variable, to optionally\n> force parallel-checkout execution during the whole test suite.\n> \n> 2. Set this variable (with a value of 2) in the second test round of our\n> linux-gcc CI job. This round runs `make test` again with some\n> optional GIT_TEST_* variables enabled, so there is no additional\n> overhead in exercising the parallel-checkout code here.\n> \n> Note that tests checking out less than two parallel-eligible entries\n> will fall back to the sequential mode. Nevertheless, it's still a good\n> exercise for the parallel-checkout framework as the fallback codepath\n> also writes the queued entries using the parallel-checkout functions\n> (only without spawning any worker).\n\nI agree that this is helpful additional testing.\n\n> Signed-off-by: Matheus Tavares \n> ---\n> ci/run-build-and-tests.sh | 1 +\n> parallel-checkout.c | 14 ++++++++++++++\n> t/README | 4 ++++\n> t/lib-parallel-checkout.sh | 3 +++\n> 4 files changed, 22 insertions(+)\n> \n> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> index a66b5e8c75..23b28e7391 100755\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -25,6 +25,7 @@ linux-gcc)\n> \texport GIT_TEST_ADD_I_USE_BUILTIN=1\n> \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n> \texport GIT_TEST_WRITE_REV_INDEX=1\n> +\texport GIT_TEST_CHECKOUT_WORKERS=2\n> \tmake test\n> \t;;\n\n(Making a note to audit the entries here, as I believe some\nof these could be enabled by default or no longer do anything.)\n\n> void get_parallel_checkout_configs(int *num_workers, int *threshold)\n> {\n> +\tchar *env_workers = getenv(\"GIT_TEST_CHECKOUT_WORKERS\");\n> +\n> +\tif (env_workers && *env_workers) {\n> +\t\tif (strtol_i(env_workers, 10, num_workers)) {\n\nI had to look up how strtol_i() differs from strtol(). It seems\nlike we should use strtol_i() more often than we do.\n\n> +\t\t\tdie(\"invalid value for GIT_TEST_CHECKOUT_WORKERS: '%s'\",\n> +\t\t\t env_workers);\n> +\t\t}\n> +\t\tif (*num_workers < 1)\n> +\t\t\t*num_workers = online_cpus();\n> +\n> +\t\t*threshold = 0;\n> +\t\treturn;\n> +\t}\n\nLGTM.\n\nThanks,\n-Stolee\n\n\n"},{"id":"423047","messageId":"CAHd-oW6OawTsPNcecWK1cOLJgfBtP0QGPu-vXaRm7_AoZ6+k0Q@mail.gmail.com","threadId":"55540","inReplyTo":"058f6ca4-a575-a463-e29b-9071c35e5bd3@gmail.com","subject":"Re: [PATCH 2/7] builtin/checkout.c: complete parallel checkout support","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-26T21:54:00Z","receivedAt":"2021-04-26T21:54:15Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Apr 23, 2021 at 1:19 PM Derrick Stolee wrote:\n>\n> On 4/22/2021 11:17 AM, Matheus Tavares wrote:\n> > There is one code path in builtin/checkout.c which still doesn't benefit\n> > from parallel checkout because it calls checkout_entry() directly,> instead of unpack_trees(). Let's add parallel checkout support for this\n> > missing spot as well.\n>\n> I couldn't tell immediately from the patch what would trigger this\n> code path. I had to trace the method calls to discover that it is\n> for the case of a pathspec-limited checkout:\n>\n> git checkout -- \n\nOops, I should have mentioned that in the commit message. Thanks for\npointing it out.\n\n> I confirmed that this does work with this change, but it might be\n> nice to have a test that verifies that parallelism is triggering for\n> this case.\n>\n> Looking ahead to patches 4-6, which add tests, I do not see one for this\n> code path. Yes, patch 7 will implicitly test it through optional\n> settings, but it would be nice to verify that the code is actually using\n> parallel workers. The test_checkout_workers helper in patch 4 should be\n> helpful for this effort.\n>\n> Please point out the test that covers this case, in case I'm just not\n> seeing it.\n\nHmm, there are some tests at t2081 and t2082 that check the\npathspec-limited case with parallel workers. For example the collision\ntests run `test_checkout_workers 2 git checkout .`. We also test\ndirect pathnames in t2082, using `test_checkout_workers 2 git checkout\nA B`.\n\n> The good news is that I can see a difference. By alternating checkouts\n> of the Git repository's \"t\" directory between v2.20 and v2.31.1, I can\n> see these results for varying numbers of workers:\n>\n> Benchmark #1: 16 workers\n> Time (mean ± σ): 108.6 ms ± 5.2 ms [User: 146.1 ms, System: 146.1 ms]\n> Range (min … max): 95.5 ms … 124.9 ms 100 runs\n>\n> Benchmark #2: 8 workers\n> Time (mean ± σ): 104.8 ms ± 4.8 ms [User: 128.3 ms, System: 131.7 ms]\n> Range (min … max): 94.2 ms … 119.0 ms 100 runs\n>\n> Benchmark #3: 4 workers\n> Time (mean ± σ): 112.3 ms ± 6.2 ms [User: 114.6 ms, System: 112.1 ms]\n> Range (min … max): 100.0 ms … 127.4 ms 100 runs\n>\n> Benchmark #4: 2 workers\n> Time (mean ± σ): 124.2 ms ± 4.2 ms [User: 106.5 ms, System: 102.0 ms]\n> Range (min … max): 114.8 ms … 136.3 ms 100 runs\n>\n> Benchmark #5: sequential\n> Time (mean ± σ): 154.6 ms ± 6.7 ms [User: 83.5 ms, System: 79.4 ms]\n> Range (min … max): 142.1 ms … 176.0 ms 100 runs\n>\n> Summary\n> '8 workers' ran\n> 1.04 ± 0.07 times faster than '16 workers'\n> 1.07 ± 0.08 times faster than '4 workers'\n> 1.19 ± 0.07 times faster than '2 workers'\n> 1.48 ± 0.09 times faster than 'sequential'\n\nNice! Thanks for the benchmark!\n\n> (Note: these time measurements are for the round-trip of two checkout\n> commands.)\n> > @@ -359,16 +360,22 @@ static int checkout_worktree(const struct checkout_opts *opts,\n> > int nr_checkouts = 0, nr_unmerged = 0;\n> > int errs = 0;\n> > int pos;\n> > + int pc_workers, pc_threshold;\n> > + struct mem_pool ce_mem_pool;\n> >\n> > state.force = 1;\n> > state.refresh_cache = 1;\n> > state.istate = &the_index;\n> >\n> > + mem_pool_init(&ce_mem_pool, 0);\n> > + get_parallel_checkout_configs(&pc_workers, &pc_threshold);\n> > init_checkout_metadata(&state.meta, info->refname,\n> > info->commit ? &info->commit->object.oid : &info->oid,\n> > NULL);\n> >\n> > enable_delayed_checkout(&state);\n> > + if (pc_workers > 1)\n> > + init_parallel_checkout();\n>\n> I'm late to looking at your parallel checkout work, but I find this\n> to be a really nice API to get things initialized.\n\nThanks :)\n"},{"id":"423048","messageId":"CAHd-oW547EXAg6bCFx84pKX0j_8Ry2RraYxVscNbK17+0ObkRA@mail.gmail.com","threadId":"55540","inReplyTo":"36615f6a-ce58-3737-8c1d-fa1253a85439@gmail.com","subject":"Re: [PATCH 3/7] checkout-index: add parallel checkout support","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-26T22:30:44Z","receivedAt":"2021-04-26T22:31:01Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Apr 23, 2021 at 3:32 PM Derrick Stolee wrote:\n>\n> On 4/22/2021 11:17 AM, Matheus Tavares wrote:\n> > @@ -142,11 +143,7 @@ static void checkout_all(const char *prefix, int prefix_length)\n> > }\n> > if (last_ce && to_tempfile)\n> > write_tempfile_record(last_ce->name, prefix);\n> > - if (errs)\n> > - /* we have already done our error reporting.\n> > - * exit with the same code as die().\n> > - */\n> > - exit(128);\n>\n> Here, it makes note of returning 128 as if it were a die(), but\n>\n> > + if (all)\n> > + err |= checkout_all(prefix, prefix_length);\n> > +\n> > + if (pc_workers > 1)\n> > + err |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n> > + NULL, NULL);\n> > +\n> > if (err)\n> > return 1;\n>\n> This returns 1 instead. Should we `return err` and use an error\n> code specific to the response? I imagine there are other reasons\n> that could cause a non-zero return for checkout_all() and\n> run_parallel_checkout().\n\nHmm, I think checkout_entry() returns -1 for any kind of error, and\ncheckout_all()'s `err` only holds the number of checkout_entry()'s\nfailures. run_parallel_checkout() also always use -1 for errors.\n\n> I suppose: is there a value in persisting the 128 response code\n> here, or is an exit of 1 sufficient? There is no documentation\n> about the exit code, so any dependence on 128 is not based on a\n> \"written promise\" but I thought it was worth mentioning.\n\nYeah, I also wondered if we should keep the 128 exit code here... But\nI couldn't see much value in doing so, unless there are scripts\nactually expecting 128. But I guess this is unlikely, right?\n\nThe additional bonus of unifying the exit path for `git checkout-index\n--all` and `git checkout-index ` is that the code flow becomes a\nlittle easier to read, especially now that they both need to\n[conditionally] call run_parallel_checkout() before exiting.\n"},{"id":"423052","messageId":"CAHd-oW7-Lv9hZZGDjVHjvDKvYpJ0wektVVPNrazZ441K25SczQ@mail.gmail.com","threadId":"55540","inReplyTo":"1b1cdef5-7d90-c6f3-ea8d-e1c9d472ffff@gmail.com","subject":"Re: [PATCH 4/7] parallel-checkout: add tests for basic operations","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-27T02:30:08Z","receivedAt":"2021-04-27T02:30:22Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Apr 23, 2021 at 4:18 PM Derrick Stolee wrote:\n>\n> On 4/22/2021 11:17 AM, Matheus Tavares wrote:\n> > Add tests to populate the working tree during clone and checkout using\n> > sequential and parallel mode, to confirm that they produce identical\n> > results. Also test basic checkout mechanics, such as checking for\n> > symlinks in the leading directories and the abidance to --force.\n> >\n> > Note: some helper functions are added to a common lib file which is only\n> > included by t2080 for now. But they will also be used by other\n> > parallel-checkout tests in the following patches.\n> >\n> > Original-patch-by: Jeff Hostetler \n>\n> Is this a standard thing? Or, did you change the patch significantly\n> enough that \"Co-authored-by:\" is no longer appropriate?\n\nNo, I think \"Co-authored-by\" is appropriate, let's change this trailer :)\n\n> > +++ b/t/lib-parallel-checkout.sh\n> > @@ -0,0 +1,37 @@\n> > +# Helpers for t208* tests\n>\n> I could see tests outside of the t208* range possibly having\n> value in interacting with parallel checkout in the future.\n>\n> Perhaps:\n>\n> # Helpers for tests invoking parallel-checkout\n\nWill do!\n\n> > +\n> > +set_checkout_config () {\n> > + if test $# -ne 2\n> > + then\n> > + BUG \"set_checkout_config() requires two arguments\"\n> > + fi &&\n>\n> A usage comment is typically helpful for these helpers:\n>\n> # set_checkout_config \n> # sets global config values to use the given number of\n> # workers when the given threshold is met.\n\nOK, I'll change that.\n\n> > +\n> > + test_config_global checkout.workers $1 &&\n> > + test_config_global checkout.thresholdForParallelism $2\n> > +}\n> > +\n> > +# Run \"${@:2}\" and check that $1 checkout workers were used\n> > +test_checkout_workers () {\n>\n> This simpler doc style works, too.\n>\n> > + if test $# -lt 2\n> > + then\n> > + BUG \"too few arguments to test_checkout_workers()\"\n> > + fi &&\n> > +\n> > + expected_workers=$1 &&\n> > + shift &&\n> > +\n> > + rm -f trace &&\n> > + GIT_TRACE2=\"$(pwd)/trace\" \"$@\" &&\n> > +\n> > + workers=$(grep \"child_start\\[..*\\] git checkout--worker\" trace | wc -l) &&\n> > + test $workers -eq $expected_workers &&\n> > + rm -f trace\n>\n> I wonder if this should be a \"test_when_finished rm -f trace\" being\n> recorded earlier in the step.It would also benefit from using a more\n> specific name than \"trace\". Something like \"trace-test-checkout-workers\"\n> would be unlikely to collide with someone else's trace.\n\nGood idea, I'll change the trace file name.\n\nUnfortunately, I think we won't be able to use `test_when_finished`\nhere as this function is sometimes called inside subshells, and\n`test_when_finished` doesn't work in this situation :(\n\n> > +# Verify that both the working tree and the index were created correctly\n> > +verify_checkout () {\n>\n> Add usage of a repo in $1?\n\nSure!\n\n> > + git -C \"$1\" diff-index --quiet HEAD -- &&\n> > + git -C \"$1\" diff-index --quiet --cached HEAD -- &&\n> > + git -C \"$1\" status --porcelain >\"$1\".status &&\n> > + test_must_be_empty \"$1\".status\n> > +}\n>\n>\n> > +TEST_NO_CREATE_REPO=1\n> > +. ./test-lib.sh\n> > +. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n> > +\n> > +# Test parallel-checkout with a branch switch containing file creations,\n> > +# deletions, and modification; with different entry types. Switching from B1 to\n> > +# B2 will have the following changes:\n> > +#\n> > +# - a (file): modified\n> > +# - e/x (file): deleted\n> > +# - b (symlink): deleted\n> > +# - b/f (file): created\n> > +# - e (symlink): created\n> > +# - d (submodule): created\n> > +#\n>\n> An interesting set of changes. What about a directory/file conflict?\n>\n> Something like this might be useful:\n>\n> # - f/t (file): deleted\n> # - f (file): created\n>\n> in fact, it could be interesting to have a file conflict with each\n> of these types, such as the symlink 'e' and the submodule 'd'.\n\nSure, I'll add those. (But we already have the symlink case with 'e/x'\n(file) and 'e' (symlink), no?)\n\n> While we are at it, what about a symlink/submodule conflict? I know\n> it makes the test bigger, but doing everything simultaneously through\n> a carefully designed repository helps prevent test case bloat.\n>\n> > +test_expect_success SYMLINKS 'setup repo for checkout with various types of changes' '\n>\n> Could we split this setup step into two parts? Once could\n> set up everything except the symlinks and would not require\n> the SYMLINKS prereq. We could then have another test with\n> the SYMLINKS prereq that extends B1 and B2 to have symlinks\n> and their conflicts. The remaining tests would work on any\n> platform without needing the SYMLINKS prereq.\n\nGood point. I think we might be able to create the symlinks with\n`test_ln_s_add` and completely get rid of the SYMLINKS prereq :)\n\n> > +test_expect_success SYMLINKS 'sequential checkout' '\n> > + cp -R various various_sequential &&\n> > + set_checkout_config 1 0 &&\n> > + test_checkout_workers 0 \\\n> > + git -C various_sequential checkout --recurse-submodules B2 &&\n> > + verify_checkout various_sequential\n> > +'\n>\n> I see all these tests are very similar. Perhaps group\n> them to demonstrate their differences?\n\nMakes sense, I'll do that.\n\n> parallel_test_case () {\n> test_expect_success \"$1\" \"\n> cp -R various $2 &&\n> set_checkout_config $3 $4 &&\n> test_checkout_workers $5 \\\n> git -C $2 checkout --recurse-submodules B2 &&\n> verify_checkout $2\n> \"\n> }\n>\n> parallel_test_case 'sequential checkout' \\\n> various_sequential 1 0 0\n> parallel_test_case 'parallel checkout' \\\n> various_parallel 2 0 2\n> parallel_test_case 'fallback to sequential checkout (threshold)' \\\n> various_sequential_fallback 2 100 0\n>\n> > +test_expect_success SYMLINKS 'parallel checkout on clone' '\n> > + git -C various checkout --recurse-submodules B2 &&\n> > + set_checkout_config 2 0 &&\n> > + test_checkout_workers 2 \\\n> > + git clone --recurse-submodules various various_parallel_clone &&\n> > + verify_checkout various_parallel_clone\n> > +'\n"},{"id":"423377","messageId":"cover.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619104091.git.matheus.bernardino@usp.br","subject":"[PATCH v2 0/8] Parallel Checkout (part 3)","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:27Z","receivedAt":"2021-04-30T21:40:48Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This is the last part of the parallel checkout series. It's based on\nmt/parallel-checkout-part-2. This part adds parallel checkout support to\n\"git checkout-index\" and \"git checkout \", and adds tests for\nthe parallel checkout framework.\n\nChanges since v2:\n\nPatch 2:\n- Mentioned in the commit message which checkout mode the patch is\n adding parallel checkout support for (i.e. pathspec-limited mode).\n\nPatch 4:\n\n t/lib-parallel-checkout.sh:\n - Added/improved usage message for the helper functions.\n - Renamed trace file in test_checkout_workers to avoid collisions.\n - At verify_checkout:\n\n * Used --ignore-submodules=none for the diff-index execution. (I\n\thadn't noticed before that the default behavior changed to\n\t'untracked' on git v2.31.0.)\n\n * Removed the `diff-index --cached` check as it was redundant.\n\tWe are already implicitly checking the index by asserting that\n\t`git status` is empty after the `git diff-index HEAD` check.\n\n t/t2080:\n - Used `test_ln_s_add`, to avoid requiring the SYMLINKS prereq in all\n tests but one.\n - Grouped the basic checkout/clone tests in a for loop to make make it\n easier to see what are the differences between then (i.e. the\n different cases they are testing).\n - Added more entry type changes in the branch switch used to check\n parallel checkout correctness.\n - Added a test to ensure submodules can also use parallel checkout.\n - Compare the working trees with `git diff --no-index` instead of\n `diff -r` to avoid following symlinks.\n\nPatch 6:\n- Split patch into two: one moving the helper functions from t0028 to\n lib-encoding.sh, and another actually adding the t2082 tests.\n\nMatheus Tavares (8):\n make_transient_cache_entry(): optionally alloc from mem_pool\n builtin/checkout.c: complete parallel checkout support\n checkout-index: add parallel checkout support\n parallel-checkout: add tests for basic operations\n parallel-checkout: add tests related to path collisions\n t0028: extract encoding helpers to lib-encoding.sh\n parallel-checkout: add tests related to .gitattributes\n ci: run test round with parallel-checkout enabled\n\n builtin/checkout--worker.c | 2 +-\n builtin/checkout-index.c | 24 ++-\n builtin/checkout.c | 20 ++-\n builtin/difftool.c | 2 +-\n cache.h | 11 +-\n ci/run-build-and-tests.sh | 1 +\n parallel-checkout.c | 18 ++\n read-cache.c | 12 +-\n t/README | 4 +\n t/lib-encoding.sh | 25 +++\n t/lib-parallel-checkout.sh | 45 +++++\n t/t0028-working-tree-encoding.sh | 25 +--\n t/t2080-parallel-checkout-basics.sh | 229 ++++++++++++++++++++++++\n t/t2081-parallel-checkout-collisions.sh | 162 +++++++++++++++++\n t/t2082-parallel-checkout-attributes.sh | 194 ++++++++++++++++++++\n unpack-trees.c | 2 +-\n 16 files changed, 727 insertions(+), 49 deletions(-)\n create mode 100644 t/lib-encoding.sh\n create mode 100644 t/lib-parallel-checkout.sh\n create mode 100755 t/t2080-parallel-checkout-basics.sh\n create mode 100755 t/t2081-parallel-checkout-collisions.sh\n create mode 100755 t/t2082-parallel-checkout-attributes.sh\n\nRange-diff against v1:\n1: f870040bfb = 1: f870040bfb make_transient_cache_entry(): optionally alloc from mem_pool\n2: 5e0dee7beb ! 2: e2d82c4337 builtin/checkout.c: complete parallel checkout support\n @@ Metadata\n ## Commit message ##\n builtin/checkout.c: complete parallel checkout support\n \n - There is one code path in builtin/checkout.c which still doesn't benefit\n - from parallel checkout because it calls checkout_entry() directly,\n - instead of unpack_trees(). Let's add parallel checkout support for this\n - missing spot as well. Note: the transient cache entries allocated in\n - checkout_merged() are now allocated in a mem_pool which is only\n - discarded after parallel checkout finishes. This is done because the\n - entries need to be valid when run_parallel_checkout() is called.\n + Pathspec-limited checkouts (like `git checkout *.txt`) are performed by\n + a code path that doesn't yet support parallel checkout because it calls\n + checkout_entry() directly, instead of unpack_trees(). Let's add parallel\n + checkout support for this code path too.\n +\n + Note: the transient cache entries allocated in checkout_merged() are now\n + allocated in a mem_pool which is only discarded after parallel checkout\n + finishes. This is done because the entries need to be valid when\n + run_parallel_checkout() is called.\n \n Signed-off-by: Matheus Tavares \n \n3: 6bf1db2fd8 = 3: 0fe1a5fabc checkout-index: add parallel checkout support\n4: 46128ba8d8 ! 4: f604c50dba parallel-checkout: add tests for basic operations\n @@ Commit message\n included by t2080 for now. But they will also be used by other\n parallel-checkout tests in the following patches.\n \n - Original-patch-by: Jeff Hostetler \n - Signed-off-by: Jeff Hostetler \n + Co-authored-by: Jeff Hostetler \n Signed-off-by: Matheus Tavares \n \n ## t/lib-parallel-checkout.sh (new) ##\n @@\n -+# Helpers for t208* tests\n ++# Helpers for tests invoking parallel-checkout\n +\n +set_checkout_config () {\n +\tif test $# -ne 2\n +\tthen\n -+\t\tBUG \"set_checkout_config() requires two arguments\"\n ++\t\tBUG \"usage: set_checkout_config \"\n +\tfi &&\n +\n +\ttest_config_global checkout.workers $1 &&\n @@ t/lib-parallel-checkout.sh (new)\n +test_checkout_workers () {\n +\tif test $# -lt 2\n +\tthen\n -+\t\tBUG \"too few arguments to test_checkout_workers()\"\n ++\t\tBUG \"too few arguments to test_checkout_workers\"\n +\tfi &&\n +\n -+\texpected_workers=$1 &&\n ++\tlocal expected_workers=$1 &&\n +\tshift &&\n +\n -+\trm -f trace &&\n -+\tGIT_TRACE2=\"$(pwd)/trace\" \"$@\" &&\n ++\tlocal trace_file=trace-test-checkout-workers &&\n ++\trm -f \"$trace_file\" &&\n ++\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" &&\n +\n -+\tworkers=$(grep \"child_start\\[..*\\] git checkout--worker\" trace | wc -l) &&\n ++\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n +\ttest $workers -eq $expected_workers &&\n -+\trm -f trace\n ++\trm \"$trace_file\"\n +}\n +\n +# Verify that both the working tree and the index were created correctly\n +verify_checkout () {\n -+\tgit -C \"$1\" diff-index --quiet HEAD -- &&\n -+\tgit -C \"$1\" diff-index --quiet --cached HEAD -- &&\n ++\tif test $# -ne 1\n ++\tthen\n ++\t\tBUG \"usage: verify_checkout \"\n ++\tfi &&\n ++\n ++\tgit -C \"$1\" diff-index --ignore-submodules=none --exit-code HEAD -- &&\n +\tgit -C \"$1\" status --porcelain >\"$1\".status &&\n +\ttest_must_be_empty \"$1\".status\n +}\n @@ t/t2080-parallel-checkout-basics.sh (new)\n +. ./test-lib.sh\n +. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n +\n -+# Test parallel-checkout with a branch switch containing file creations,\n -+# deletions, and modification; with different entry types. Switching from B1 to\n -+# B2 will have the following changes:\n ++# Test parallel-checkout with a branch switch containing a variety of file\n ++# creations, deletions, and modifications, involving different entry types.\n ++# The branches B1 and B2 have the following paths:\n ++#\n ++# B1 B2\n ++# a/a (file) a (file)\n ++# b (file) b/b (file)\n ++#\n ++# c/c (file) c (symlink)\n ++# d (symlink) d/d (file)\n ++#\n ++# e/e (file) e (submodule)\n ++# f (submodule) f/f (file)\n ++#\n ++# g (submodule) g (symlink)\n ++# h (symlink) h (submodule)\n ++#\n ++# Additionally, the following paths are present on both branches, but with\n ++# different contents:\n ++#\n ++# i (file) i (file)\n ++# j (symlink) j (symlink)\n ++# k (submodule) k (submodule)\n +#\n -+# - a (file): modified\n -+# - e/x (file): deleted\n -+# - b (symlink): deleted\n -+# - b/f (file): created\n -+# - e (symlink): created\n -+# - d (submodule): created\n ++# And the following paths are only present in one of the branches:\n +#\n -+test_expect_success SYMLINKS 'setup repo for checkout with various types of changes' '\n ++# l/l (file) -\n ++# - m/m (file)\n ++#\n ++test_expect_success 'setup repo for checkout with various types of changes' '\n ++\tgit init sub &&\n ++\t(\n ++\t\tcd sub &&\n ++\t\tgit checkout -b B2 &&\n ++\t\techo B2 >file &&\n ++\t\tgit add file &&\n ++\t\tgit commit -m file &&\n ++\n ++\t\tgit checkout -b B1 &&\n ++\t\techo B1 >file &&\n ++\t\tgit add file &&\n ++\t\tgit commit -m file\n ++\t) &&\n ++\n +\tgit init various &&\n +\t(\n +\t\tcd various &&\n ++\n +\t\tgit checkout -b B1 &&\n -+\t\techo a >a &&\n -+\t\tmkdir e &&\n -+\t\techo e/x >e/x &&\n -+\t\tln -s e b &&\n -+\t\tgit add -A &&\n ++\t\tmkdir a c e &&\n ++\t\techo a/a >a/a &&\n ++\t\techo b >b &&\n ++\t\techo c/c >c/c &&\n ++\t\ttest_ln_s_add c d &&\n ++\t\techo e/e >e/e &&\n ++\t\tgit submodule add ../sub f &&\n ++\t\tgit submodule add ../sub g &&\n ++\t\ttest_ln_s_add c h &&\n ++\n ++\t\techo \"B1 i\" >i &&\n ++\t\ttest_ln_s_add c j &&\n ++\t\tgit submodule add -b B1 ../sub k &&\n ++\t\tmkdir l &&\n ++\t\techo l/l >l/l &&\n ++\n ++\t\tgit add . &&\n +\t\tgit commit -m B1 &&\n +\n +\t\tgit checkout -b B2 &&\n -+\t\techo modified >a &&\n -+\t\trm -rf e &&\n -+\t\trm b &&\n -+\t\tmkdir b &&\n -+\t\techo b/f >b/f &&\n -+\t\tln -s b e &&\n -+\t\tgit init d &&\n -+\t\ttest_commit -C d f &&\n -+\t\tgit submodule add ./d &&\n -+\t\tgit add -A &&\n ++\t\tgit rm -rf :^.gitmodules :^k &&\n ++\t\tmkdir b d f &&\n ++\t\techo a >a &&\n ++\t\techo b/b >b/b &&\n ++\t\ttest_ln_s_add b c &&\n ++\t\techo d/d >d/d &&\n ++\t\tgit submodule add ../sub e &&\n ++\t\techo f/f >f/f &&\n ++\t\ttest_ln_s_add b g &&\n ++\t\tgit submodule add ../sub h &&\n ++\n ++\t\techo \"B2 i\" >i &&\n ++\t\ttest_ln_s_add b j &&\n ++\t\tgit -C k checkout B2 &&\n ++\t\tmkdir m &&\n ++\t\techo m/m >m/m &&\n ++\n ++\t\tgit add . &&\n +\t\tgit commit -m B2 &&\n +\n +\t\tgit checkout --recurse-submodules B1\n +\t)\n +'\n +\n -+test_expect_success SYMLINKS 'sequential checkout' '\n -+\tcp -R various various_sequential &&\n -+\tset_checkout_config 1 0 &&\n -+\ttest_checkout_workers 0 \\\n -+\t\tgit -C various_sequential checkout --recurse-submodules B2 &&\n -+\tverify_checkout various_sequential\n -+'\n ++for mode in sequential parallel sequential-fallback\n ++do\n ++\tcase $mode in\n ++\tsequential) workers=1 threshold=0 expected_workers=0 ;;\n ++\tparallel) workers=2 threshold=0 expected_workers=2 ;;\n ++\tsequential-fallback) workers=2 threshold=100 expected_workers=0 ;;\n ++\tesac\n +\n -+test_expect_success SYMLINKS 'parallel checkout' '\n -+\tcp -R various various_parallel &&\n -+\tset_checkout_config 2 0 &&\n -+\ttest_checkout_workers 2 \\\n -+\t\tgit -C various_parallel checkout --recurse-submodules B2 &&\n -+\tverify_checkout various_parallel\n -+'\n ++\ttest_expect_success \"$mode checkout\" '\n ++\t\trepo=various_$mode &&\n ++\t\tcp -R various $repo &&\n +\n -+test_expect_success SYMLINKS 'fallback to sequential checkout (threshold)' '\n -+\tcp -R various various_sequential_fallback &&\n -+\tset_checkout_config 2 100 &&\n -+\ttest_checkout_workers 0 \\\n -+\t\tgit -C various_sequential_fallback checkout --recurse-submodules B2 &&\n -+\tverify_checkout various_sequential_fallback\n -+'\n ++\t\t# The just copied files have more recent timestamps than their\n ++\t\t# associated index entries. So refresh the cached timestamps\n ++\t\t# to avoid an \"entry not up-to-date\" error from `git checkout`.\n ++\t\t# We only have to do this for the submodules as `git checkout`\n ++\t\t# will already refresh the superproject index before performing\n ++\t\t# the up-to-date check.\n ++\t\t#\n ++\t\tgit -C $repo submodule foreach \"git update-index --refresh\" &&\n +\n -+test_expect_success SYMLINKS 'parallel checkout on clone' '\n -+\tgit -C various checkout --recurse-submodules B2 &&\n -+\tset_checkout_config 2 0 &&\n -+\ttest_checkout_workers 2 \\\n -+\t\tgit clone --recurse-submodules various various_parallel_clone &&\n -+\tverify_checkout various_parallel_clone\n -+'\n ++\t\tset_checkout_config $workers $threshold &&\n ++\t\ttest_checkout_workers $expected_workers \\\n ++\t\t\tgit -C $repo checkout --recurse-submodules B2 &&\n ++\t\tverify_checkout $repo\n ++\t'\n ++done\n +\n -+test_expect_success SYMLINKS 'fallback to sequential checkout on clone (threshold)' '\n -+\tgit -C various checkout --recurse-submodules B2 &&\n -+\tset_checkout_config 2 100 &&\n -+\ttest_checkout_workers 0 \\\n -+\t\tgit clone --recurse-submodules various various_sequential_fallback_clone &&\n -+\tverify_checkout various_sequential_fallback_clone\n -+'\n ++for mode in parallel sequential-fallback\n ++do\n ++\tcase $mode in\n ++\tparallel) workers=2 threshold=0 expected_workers=2 ;;\n ++\tsequential-fallback) workers=2 threshold=100 expected_workers=0 ;;\n ++\tesac\n ++\n ++\ttest_expect_success \"$mode checkout on clone\" '\n ++\t\trepo=various_${mode}_clone &&\n ++\t\tset_checkout_config $workers $threshold &&\n ++\t\ttest_checkout_workers $expected_workers \\\n ++\t\t\tgit clone --recurse-submodules --branch B2 various $repo &&\n ++\t\tverify_checkout $repo\n ++\t'\n ++done\n +\n +# Just to be paranoid, actually compare the working trees' contents directly.\n -+test_expect_success SYMLINKS 'compare the working trees' '\n ++test_expect_success 'compare the working trees' '\n +\trm -rf various_*/.git &&\n -+\trm -rf various_*/d/.git &&\n ++\trm -rf various_*/*/.git &&\n ++\n ++\t# We use `git diff` instead of `diff -r` because the latter would\n ++\t# follow symlinks, and not all `diff` implementations support the\n ++\t# `--no-dereference` option.\n ++\t#\n ++\tgit diff --no-index various_sequential various_parallel &&\n ++\tgit diff --no-index various_sequential various_parallel_clone &&\n ++\tgit diff --no-index various_sequential various_sequential-fallback &&\n ++\tgit diff --no-index various_sequential various_sequential-fallback_clone\n ++'\n +\n -+\tdiff -r various_sequential various_parallel &&\n -+\tdiff -r various_sequential various_sequential_fallback &&\n -+\tdiff -r various_sequential various_parallel_clone &&\n -+\tdiff -r various_sequential various_sequential_fallback_clone\n ++# Currently, each submodule is checked out in a separated child process, but\n ++# these subprocesses must also be able to use parallel checkout workers to\n ++# write the submodules' entries.\n ++test_expect_success 'submodules can use parallel checkout' '\n ++\tset_checkout_config 2 0 &&\n ++\tgit init super &&\n ++\t(\n ++\t\tcd super &&\n ++\t\tgit init sub &&\n ++\t\ttest_commit -C sub A &&\n ++\t\ttest_commit -C sub B &&\n ++\t\tgit submodule add ./sub &&\n ++\t\tgit commit -m sub &&\n ++\t\trm sub/* &&\n ++\t\ttest_checkout_workers 2 git checkout --recurse-submodules .\n ++\t)\n +'\n +\n +test_expect_success 'parallel checkout respects --[no]-force' '\n5: 71be08b895 ! 5: eae6d3a1c1 parallel-checkout: add tests related to path collisions\n @@ Commit message\n checkout workers, both to avoid race conditions and to report colliding\n entries on clone.\n \n - Original-patch-by: Jeff Hostetler \n - Signed-off-by: Jeff Hostetler \n + Co-authored-by: Jeff Hostetler \n Signed-off-by: Matheus Tavares \n \n ## parallel-checkout.c ##\n @@ parallel-checkout.c: void write_pc_item(struct parallel_checkout_item *pc_item,\n \n ## t/lib-parallel-checkout.sh ##\n @@ t/lib-parallel-checkout.sh: test_checkout_workers () {\n - \tshift &&\n \n - \trm -f trace &&\n --\tGIT_TRACE2=\"$(pwd)/trace\" \"$@\" &&\n -+\tGIT_TRACE2=\"$(pwd)/trace\" \"$@\" 2>&8 &&\n + \tlocal trace_file=trace-test-checkout-workers &&\n + \trm -f \"$trace_file\" &&\n +-\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" &&\n ++\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n \n - \tworkers=$(grep \"child_start\\[..*\\] git checkout--worker\" trace | wc -l) &&\n + \tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n \ttest $workers -eq $expected_workers &&\n - \trm -f trace\n + \trm \"$trace_file\"\n -}\n +} 8>&2 2>&4\n \n-: ---------- > 6: 9161cd1503 t0028: extract encoding helpers to lib-encoding.sh\n6: 1dedfd994a ! 7: bc584897e6 parallel-checkout: add tests related to .gitattributes\n @@ Commit message\n require external filters, are correctly smudge and written when\n parallel-checkout is enabled.\n \n - Note: to avoid repeating code, some helper functions are extracted from\n - t0028 into a common lib file.\n -\n - Original-patch-by: Jeff Hostetler \n - Signed-off-by: Jeff Hostetler \n + Co-authored-by: Jeff Hostetler \n Signed-off-by: Matheus Tavares \n \n - ## t/lib-encoding.sh (new) ##\n -@@\n -+# Encoding helpers used by t0028 and t2082\n -+\n -+test_lazy_prereq NO_UTF16_BOM '\n -+\ttest $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6\n -+'\n -+\n -+test_lazy_prereq NO_UTF32_BOM '\n -+\ttest $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12\n -+'\n -+\n -+write_utf16 () {\n -+\tif test_have_prereq NO_UTF16_BOM\n -+\tthen\n -+\t\tprintf '\\376\\377'\n -+\tfi &&\n -+\ticonv -f UTF-8 -t UTF-16\n -+}\n -+\n -+write_utf32 () {\n -+\tif test_have_prereq NO_UTF32_BOM\n -+\tthen\n -+\t\tprintf '\\0\\0\\376\\377'\n -+\tfi &&\n -+\ticonv -f UTF-8 -t UTF-32\n -+}\n -\n - ## t/t0028-working-tree-encoding.sh ##\n -@@ t/t0028-working-tree-encoding.sh: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n - export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n - \n - . ./test-lib.sh\n -+. \"$TEST_DIRECTORY/lib-encoding.sh\"\n - \n - GIT_TRACE_WORKING_TREE_ENCODING=1 && export GIT_TRACE_WORKING_TREE_ENCODING\n - \n --test_lazy_prereq NO_UTF16_BOM '\n --\ttest $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6\n --'\n --\n --test_lazy_prereq NO_UTF32_BOM '\n --\ttest $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12\n --'\n --\n --write_utf16 () {\n --\tif test_have_prereq NO_UTF16_BOM\n --\tthen\n --\t\tprintf '\\376\\377'\n --\tfi &&\n --\ticonv -f UTF-8 -t UTF-16\n --}\n --\n --write_utf32 () {\n --\tif test_have_prereq NO_UTF32_BOM\n --\tthen\n --\t\tprintf '\\0\\0\\376\\377'\n --\tfi &&\n --\ticonv -f UTF-8 -t UTF-32\n --}\n --\n - test_expect_success 'setup test files' '\n - \tgit config core.eol lf &&\n - \n -\n ## t/t2082-parallel-checkout-attributes.sh (new) ##\n @@\n +#!/bin/sh\n7: 3cc5b10081 ! 8: 1bc5c523c5 ci: run test round with parallel-checkout enabled\n @@ t/README: and \"sha256\".\n \n ## t/lib-parallel-checkout.sh ##\n @@\n - # Helpers for t208* tests\n + # Helpers for tests invoking parallel-checkout\n \n +# Parallel checkout tests need full control of the number of workers\n +unset GIT_TEST_CHECKOUT_WORKERS\n-- \n2.30.1\n\n"},{"id":"423378","messageId":"f870040bfb3e73ee8cd27352b0acc65bb54be560.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v2 1/8] make_transient_cache_entry(): optionally alloc from mem_pool","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:28Z","receivedAt":"2021-04-30T21:40:56Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Allow make_transient_cache_entry() to optionally receive a mem_pool\nstruct in which it should allocate the entry. This will be used in the\nfollowing patch, to store some transient entries which should persist\nuntil parallel checkout finishes.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout--worker.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/difftool.c | 2 +-\n cache.h | 11 ++++++-----\n read-cache.c | 12 ++++++++----\n unpack-trees.c | 2 +-\n 6 files changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c\nindex 31e0de2f7e..289a9b8f89 100644\n--- a/builtin/checkout--worker.c\n+++ b/builtin/checkout--worker.c\n@@ -39,7 +39,7 @@ static void packet_to_pc_item(const char *buffer, int len,\n \t}\n \n \tmemset(pc_item, 0, sizeof(*pc_item));\n-\tpc_item->ce = make_empty_transient_cache_entry(fixed_portion->name_len);\n+\tpc_item->ce = make_empty_transient_cache_entry(fixed_portion->name_len, NULL);\n \tpc_item->ce->ce_namelen = fixed_portion->name_len;\n \tpc_item->ce->ce_mode = fixed_portion->ce_mode;\n \tmemcpy(pc_item->ce->name, variant, pc_item->ce->ce_namelen);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 4c696ef480..db667d0267 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -291,7 +291,7 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko\n \tif (write_object_file(result_buf.ptr, result_buf.size, blob_type, &oid))\n \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n \tfree(result_buf.ptr);\n-\tce = make_transient_cache_entry(mode, &oid, path, 2);\n+\tce = make_transient_cache_entry(mode, &oid, path, 2, NULL);\n \tif (!ce)\n \t\tdie(_(\"make_cache_entry failed for path '%s'\"), path);\n \tstatus = checkout_entry(ce, state, NULL, nr_checkouts);\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex ef25729d49..afacbcd581 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -323,7 +323,7 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n \tstruct cache_entry *ce;\n \tint ret;\n \n-\tce = make_transient_cache_entry(mode, oid, path, 0);\n+\tce = make_transient_cache_entry(mode, oid, path, 0, NULL);\n \tret = checkout_entry(ce, state, NULL, NULL);\n \n \tdiscard_cache_entry(ce);\ndiff --git a/cache.h b/cache.h\nindex 148d9ab5f1..b6b42cc3f3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -356,16 +356,17 @@ struct cache_entry *make_empty_cache_entry(struct index_state *istate,\n \t\t\t\t\t size_t name_len);\n \n /*\n- * Create a cache_entry that is not intended to be added to an index.\n- * Caller is responsible for discarding the cache_entry\n- * with `discard_cache_entry`.\n+ * Create a cache_entry that is not intended to be added to an index. If `mp`\n+ * is not NULL, the entry is allocated within the given memory pool. Caller is\n+ * responsible for discarding \"loose\" entries with `discard_cache_entry()` and\n+ * the mem_pool with `mem_pool_discard(mp, should_validate_cache_entries())`.\n */\n struct cache_entry *make_transient_cache_entry(unsigned int mode,\n \t\t\t\t\t const struct object_id *oid,\n \t\t\t\t\t const char *path,\n-\t\t\t\t\t int stage);\n+\t\t\t\t\t int stage, struct mem_pool *mp);\n \n-struct cache_entry *make_empty_transient_cache_entry(size_t name_len);\n+struct cache_entry *make_empty_transient_cache_entry(size_t len, struct mem_pool *mp);\n \n /*\n * Discard cache entry.\ndiff --git a/read-cache.c b/read-cache.c\nindex 5a907af2fb..eb389ccb8f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -813,8 +813,10 @@ struct cache_entry *make_empty_cache_entry(struct index_state *istate, size_t le\n \treturn mem_pool__ce_calloc(find_mem_pool(istate), len);\n }\n \n-struct cache_entry *make_empty_transient_cache_entry(size_t len)\n+struct cache_entry *make_empty_transient_cache_entry(size_t len, struct mem_pool *mp)\n {\n+\tif (mp)\n+\t\treturn mem_pool__ce_calloc(mp, len);\n \treturn xcalloc(1, cache_entry_size(len));\n }\n \n@@ -848,8 +850,10 @@ struct cache_entry *make_cache_entry(struct index_state *istate,\n \treturn ret;\n }\n \n-struct cache_entry *make_transient_cache_entry(unsigned int mode, const struct object_id *oid,\n-\t\t\t\t\t const char *path, int stage)\n+struct cache_entry *make_transient_cache_entry(unsigned int mode,\n+\t\t\t\t\t const struct object_id *oid,\n+\t\t\t\t\t const char *path, int stage,\n+\t\t\t\t\t struct mem_pool *mp)\n {\n \tstruct cache_entry *ce;\n \tint len;\n@@ -860,7 +864,7 @@ struct cache_entry *make_transient_cache_entry(unsigned int mode, const struct o\n \t}\n \n \tlen = strlen(path);\n-\tce = make_empty_transient_cache_entry(len);\n+\tce = make_empty_transient_cache_entry(len, mp);\n \n \toidcpy(&ce->oid, oid);\n \tmemcpy(ce->name, path, len);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 4b77e52c6b..fa5b7ab7ee 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1034,7 +1034,7 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,\n \tsize_t len = traverse_path_len(info, tree_entry_len(n));\n \tstruct cache_entry *ce =\n \t\tis_transient ?\n-\t\tmake_empty_transient_cache_entry(len) :\n+\t\tmake_empty_transient_cache_entry(len, NULL) :\n \t\tmake_empty_cache_entry(istate, len);\n \n \tce->ce_mode = create_ce_mode(n->mode);\n-- \n2.30.1\n\n"},{"id":"423379","messageId":"e2d82c4337dc002d7296454c59d4006c3bb43c27.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v2 2/8] builtin/checkout.c: complete parallel checkout support","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:29Z","receivedAt":"2021-04-30T21:40:57Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Pathspec-limited checkouts (like `git checkout *.txt`) are performed by\na code path that doesn't yet support parallel checkout because it calls\ncheckout_entry() directly, instead of unpack_trees(). Let's add parallel\ncheckout support for this code path too.\n\nNote: the transient cache entries allocated in checkout_merged() are now\nallocated in a mem_pool which is only discarded after parallel checkout\nfinishes. This is done because the entries need to be valid when\nrun_parallel_checkout() is called.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex db667d0267..b71dc08430 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -27,6 +27,7 @@\n #include \"wt-status.h\"\n #include \"xdiff-interface.h\"\n #include \"entry.h\"\n+#include \"parallel-checkout.h\"\n \n static const char * const checkout_usage[] = {\n \tN_(\"git checkout [] \"),\n@@ -230,7 +231,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,\n \t\treturn error(_(\"path '%s' does not have their version\"), ce->name);\n }\n \n-static int checkout_merged(int pos, const struct checkout *state, int *nr_checkouts)\n+static int checkout_merged(int pos, const struct checkout *state,\n+\t\t\t int *nr_checkouts, struct mem_pool *ce_mem_pool)\n {\n \tstruct cache_entry *ce = active_cache[pos];\n \tconst char *path = ce->name;\n@@ -291,11 +293,10 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko\n \tif (write_object_file(result_buf.ptr, result_buf.size, blob_type, &oid))\n \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n \tfree(result_buf.ptr);\n-\tce = make_transient_cache_entry(mode, &oid, path, 2, NULL);\n+\tce = make_transient_cache_entry(mode, &oid, path, 2, ce_mem_pool);\n \tif (!ce)\n \t\tdie(_(\"make_cache_entry failed for path '%s'\"), path);\n \tstatus = checkout_entry(ce, state, NULL, nr_checkouts);\n-\tdiscard_cache_entry(ce);\n \treturn status;\n }\n \n@@ -359,16 +360,22 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \tint nr_checkouts = 0, nr_unmerged = 0;\n \tint errs = 0;\n \tint pos;\n+\tint pc_workers, pc_threshold;\n+\tstruct mem_pool ce_mem_pool;\n \n \tstate.force = 1;\n \tstate.refresh_cache = 1;\n \tstate.istate = &the_index;\n \n+\tmem_pool_init(&ce_mem_pool, 0);\n+\tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n \tinit_checkout_metadata(&state.meta, info->refname,\n \t\t\t info->commit ? &info->commit->object.oid : &info->oid,\n \t\t\t NULL);\n \n \tenable_delayed_checkout(&state);\n+\tif (pc_workers > 1)\n+\t\tinit_parallel_checkout();\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n \t\tif (ce->ce_flags & CE_MATCHED) {\n@@ -384,10 +391,15 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \t\t\t\t\t\t &nr_checkouts, opts->overlay_mode);\n \t\t\telse if (opts->merge)\n \t\t\t\terrs |= checkout_merged(pos, &state,\n-\t\t\t\t\t\t\t&nr_unmerged);\n+\t\t\t\t\t\t\t&nr_unmerged,\n+\t\t\t\t\t\t\t&ce_mem_pool);\n \t\t\tpos = skip_same_name(ce, pos) - 1;\n \t\t}\n \t}\n+\tif (pc_workers > 1)\n+\t\terrs |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n+\t\t\t\t\t NULL, NULL);\n+\tmem_pool_discard(&ce_mem_pool, should_validate_cache_entries());\n \tremove_marked_cache_entries(&the_index, 1);\n \tremove_scheduled_dirs();\n \terrs |= finish_delayed_checkout(&state, &nr_checkouts);\n-- \n2.30.1\n\n"},{"id":"423380","messageId":"0fe1a5fabc8cb5f7c2421afaefae030d399d28ed.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v2 3/8] checkout-index: add parallel checkout support","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:30Z","receivedAt":"2021-04-30T21:40:59Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Note: previously, `checkout_all()` would not return on errors, but\ninstead call `exit()` with a non-zero code. However, it only did that\nafter calling `checkout_entry()` for all index entries, thus not\nstopping on the first error, but attempting to write the maximum number\nof entries possible. In order to maintain this behavior we now propagate\n`checkout_all()`s error status to `cmd_checkout_index()`, so that it can\ncall `run_parallel_checkout()` and attempt to write the queued entries\nbefore exiting with the error code.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout-index.c | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex c0bf4ac1b2..e8a82ea9ed 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -12,6 +12,7 @@\n #include \"cache-tree.h\"\n #include \"parse-options.h\"\n #include \"entry.h\"\n+#include \"parallel-checkout.h\"\n \n #define CHECKOUT_ALL 4\n static int nul_term_line;\n@@ -115,7 +116,7 @@ static int checkout_file(const char *name, const char *prefix)\n \treturn -1;\n }\n \n-static void checkout_all(const char *prefix, int prefix_length)\n+static int checkout_all(const char *prefix, int prefix_length)\n {\n \tint i, errs = 0;\n \tstruct cache_entry *last_ce = NULL;\n@@ -142,11 +143,7 @@ static void checkout_all(const char *prefix, int prefix_length)\n \t}\n \tif (last_ce && to_tempfile)\n \t\twrite_tempfile_record(last_ce->name, prefix);\n-\tif (errs)\n-\t\t/* we have already done our error reporting.\n-\t\t * exit with the same code as die().\n-\t\t */\n-\t\texit(128);\n+\treturn !!errs;\n }\n \n static const char * const builtin_checkout_index_usage[] = {\n@@ -182,6 +179,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tint force = 0, quiet = 0, not_new = 0;\n \tint index_opt = 0;\n \tint err = 0;\n+\tint pc_workers, pc_threshold;\n \tstruct option builtin_checkout_index_options[] = {\n \t\tOPT_BOOL('a', \"all\", &all,\n \t\t\tN_(\"check out all files in the index\")),\n@@ -236,6 +234,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n+\tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n+\tif (pc_workers > 1)\n+\t\tinit_parallel_checkout();\n+\n \t/* Check out named files first */\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n@@ -275,12 +277,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \t}\n \n+\tif (all)\n+\t\terr |= checkout_all(prefix, prefix_length);\n+\n+\tif (pc_workers > 1)\n+\t\terr |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n+\t\t\t\t\t NULL, NULL);\n+\n \tif (err)\n \t\treturn 1;\n \n-\tif (all)\n-\t\tcheckout_all(prefix, prefix_length);\n-\n \tif (is_lock_file_locked(&lock_file) &&\n \t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(\"Unable to write new index file\");\n-- \n2.30.1\n\n"},{"id":"423381","messageId":"f604c50dba723ae5f4c45e5292e1ba76e743ecce.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v2 4/8] parallel-checkout: add tests for basic operations","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:31Z","receivedAt":"2021-04-30T21:41:00Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to populate the working tree during clone and checkout using\nsequential and parallel mode, to confirm that they produce identical\nresults. Also test basic checkout mechanics, such as checking for\nsymlinks in the leading directories and the abidance to --force.\n\nNote: some helper functions are added to a common lib file which is only\nincluded by t2080 for now. But they will also be used by other\nparallel-checkout tests in the following patches.\n\nCo-authored-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n t/lib-parallel-checkout.sh | 42 +++++\n t/t2080-parallel-checkout-basics.sh | 229 ++++++++++++++++++++++++++++\n 2 files changed, 271 insertions(+)\n create mode 100644 t/lib-parallel-checkout.sh\n create mode 100755 t/t2080-parallel-checkout-basics.sh\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nnew file mode 100644\nindex 0000000000..f60b22ef34\n--- /dev/null\n+++ b/t/lib-parallel-checkout.sh\n@@ -0,0 +1,42 @@\n+# Helpers for tests invoking parallel-checkout\n+\n+set_checkout_config () {\n+\tif test $# -ne 2\n+\tthen\n+\t\tBUG \"usage: set_checkout_config \"\n+\tfi &&\n+\n+\ttest_config_global checkout.workers $1 &&\n+\ttest_config_global checkout.thresholdForParallelism $2\n+}\n+\n+# Run \"${@:2}\" and check that $1 checkout workers were used\n+test_checkout_workers () {\n+\tif test $# -lt 2\n+\tthen\n+\t\tBUG \"too few arguments to test_checkout_workers\"\n+\tfi &&\n+\n+\tlocal expected_workers=$1 &&\n+\tshift &&\n+\n+\tlocal trace_file=trace-test-checkout-workers &&\n+\trm -f \"$trace_file\" &&\n+\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" &&\n+\n+\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n+\ttest $workers -eq $expected_workers &&\n+\trm \"$trace_file\"\n+}\n+\n+# Verify that both the working tree and the index were created correctly\n+verify_checkout () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tBUG \"usage: verify_checkout \"\n+\tfi &&\n+\n+\tgit -C \"$1\" diff-index --ignore-submodules=none --exit-code HEAD -- &&\n+\tgit -C \"$1\" status --porcelain >\"$1\".status &&\n+\ttest_must_be_empty \"$1\".status\n+}\ndiff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\nnew file mode 100755\nindex 0000000000..7087818550\n--- /dev/null\n+++ b/t/t2080-parallel-checkout-basics.sh\n@@ -0,0 +1,229 @@\n+#!/bin/sh\n+\n+test_description='parallel-checkout basics\n+\n+Ensure that parallel-checkout basically works on clone and checkout, spawning\n+the required number of workers and correctly populating both the index and the\n+working tree.\n+'\n+\n+TEST_NO_CREATE_REPO=1\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+\n+# Test parallel-checkout with a branch switch containing a variety of file\n+# creations, deletions, and modifications, involving different entry types.\n+# The branches B1 and B2 have the following paths:\n+#\n+# B1 B2\n+# a/a (file) a (file)\n+# b (file) b/b (file)\n+#\n+# c/c (file) c (symlink)\n+# d (symlink) d/d (file)\n+#\n+# e/e (file) e (submodule)\n+# f (submodule) f/f (file)\n+#\n+# g (submodule) g (symlink)\n+# h (symlink) h (submodule)\n+#\n+# Additionally, the following paths are present on both branches, but with\n+# different contents:\n+#\n+# i (file) i (file)\n+# j (symlink) j (symlink)\n+# k (submodule) k (submodule)\n+#\n+# And the following paths are only present in one of the branches:\n+#\n+# l/l (file) -\n+# - m/m (file)\n+#\n+test_expect_success 'setup repo for checkout with various types of changes' '\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit checkout -b B2 &&\n+\t\techo B2 >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m file &&\n+\n+\t\tgit checkout -b B1 &&\n+\t\techo B1 >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m file\n+\t) &&\n+\n+\tgit init various &&\n+\t(\n+\t\tcd various &&\n+\n+\t\tgit checkout -b B1 &&\n+\t\tmkdir a c e &&\n+\t\techo a/a >a/a &&\n+\t\techo b >b &&\n+\t\techo c/c >c/c &&\n+\t\ttest_ln_s_add c d &&\n+\t\techo e/e >e/e &&\n+\t\tgit submodule add ../sub f &&\n+\t\tgit submodule add ../sub g &&\n+\t\ttest_ln_s_add c h &&\n+\n+\t\techo \"B1 i\" >i &&\n+\t\ttest_ln_s_add c j &&\n+\t\tgit submodule add -b B1 ../sub k &&\n+\t\tmkdir l &&\n+\t\techo l/l >l/l &&\n+\n+\t\tgit add . &&\n+\t\tgit commit -m B1 &&\n+\n+\t\tgit checkout -b B2 &&\n+\t\tgit rm -rf :^.gitmodules :^k &&\n+\t\tmkdir b d f &&\n+\t\techo a >a &&\n+\t\techo b/b >b/b &&\n+\t\ttest_ln_s_add b c &&\n+\t\techo d/d >d/d &&\n+\t\tgit submodule add ../sub e &&\n+\t\techo f/f >f/f &&\n+\t\ttest_ln_s_add b g &&\n+\t\tgit submodule add ../sub h &&\n+\n+\t\techo \"B2 i\" >i &&\n+\t\ttest_ln_s_add b j &&\n+\t\tgit -C k checkout B2 &&\n+\t\tmkdir m &&\n+\t\techo m/m >m/m &&\n+\n+\t\tgit add . &&\n+\t\tgit commit -m B2 &&\n+\n+\t\tgit checkout --recurse-submodules B1\n+\t)\n+'\n+\n+for mode in sequential parallel sequential-fallback\n+do\n+\tcase $mode in\n+\tsequential) workers=1 threshold=0 expected_workers=0 ;;\n+\tparallel) workers=2 threshold=0 expected_workers=2 ;;\n+\tsequential-fallback) workers=2 threshold=100 expected_workers=0 ;;\n+\tesac\n+\n+\ttest_expect_success \"$mode checkout\" '\n+\t\trepo=various_$mode &&\n+\t\tcp -R various $repo &&\n+\n+\t\t# The just copied files have more recent timestamps than their\n+\t\t# associated index entries. So refresh the cached timestamps\n+\t\t# to avoid an \"entry not up-to-date\" error from `git checkout`.\n+\t\t# We only have to do this for the submodules as `git checkout`\n+\t\t# will already refresh the superproject index before performing\n+\t\t# the up-to-date check.\n+\t\t#\n+\t\tgit -C $repo submodule foreach \"git update-index --refresh\" &&\n+\n+\t\tset_checkout_config $workers $threshold &&\n+\t\ttest_checkout_workers $expected_workers \\\n+\t\t\tgit -C $repo checkout --recurse-submodules B2 &&\n+\t\tverify_checkout $repo\n+\t'\n+done\n+\n+for mode in parallel sequential-fallback\n+do\n+\tcase $mode in\n+\tparallel) workers=2 threshold=0 expected_workers=2 ;;\n+\tsequential-fallback) workers=2 threshold=100 expected_workers=0 ;;\n+\tesac\n+\n+\ttest_expect_success \"$mode checkout on clone\" '\n+\t\trepo=various_${mode}_clone &&\n+\t\tset_checkout_config $workers $threshold &&\n+\t\ttest_checkout_workers $expected_workers \\\n+\t\t\tgit clone --recurse-submodules --branch B2 various $repo &&\n+\t\tverify_checkout $repo\n+\t'\n+done\n+\n+# Just to be paranoid, actually compare the working trees' contents directly.\n+test_expect_success 'compare the working trees' '\n+\trm -rf various_*/.git &&\n+\trm -rf various_*/*/.git &&\n+\n+\t# We use `git diff` instead of `diff -r` because the latter would\n+\t# follow symlinks, and not all `diff` implementations support the\n+\t# `--no-dereference` option.\n+\t#\n+\tgit diff --no-index various_sequential various_parallel &&\n+\tgit diff --no-index various_sequential various_parallel_clone &&\n+\tgit diff --no-index various_sequential various_sequential-fallback &&\n+\tgit diff --no-index various_sequential various_sequential-fallback_clone\n+'\n+\n+# Currently, each submodule is checked out in a separated child process, but\n+# these subprocesses must also be able to use parallel checkout workers to\n+# write the submodules' entries.\n+test_expect_success 'submodules can use parallel checkout' '\n+\tset_checkout_config 2 0 &&\n+\tgit init super &&\n+\t(\n+\t\tcd super &&\n+\t\tgit init sub &&\n+\t\ttest_commit -C sub A &&\n+\t\ttest_commit -C sub B &&\n+\t\tgit submodule add ./sub &&\n+\t\tgit commit -m sub &&\n+\t\trm sub/* &&\n+\t\ttest_checkout_workers 2 git checkout --recurse-submodules .\n+\t)\n+'\n+\n+test_expect_success 'parallel checkout respects --[no]-force' '\n+\tset_checkout_config 2 0 &&\n+\tgit init dirty &&\n+\t(\n+\t\tcd dirty &&\n+\t\tmkdir D &&\n+\t\ttest_commit D/F &&\n+\t\ttest_commit F &&\n+\n+\t\trm -rf D &&\n+\t\techo changed >D &&\n+\t\techo changed >F.t &&\n+\n+\t\t# We expect 0 workers because there is nothing to be done\n+\t\ttest_checkout_workers 0 git checkout HEAD &&\n+\t\ttest_path_is_file D &&\n+\t\tgrep changed D &&\n+\t\tgrep changed F.t &&\n+\n+\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n+\t\ttest_path_is_dir D &&\n+\t\tgrep D/F D/F.t &&\n+\t\tgrep F F.t\n+\t)\n+'\n+\n+test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading dirs' '\n+\tset_checkout_config 2 0 &&\n+\tgit init symlinks &&\n+\t(\n+\t\tcd symlinks &&\n+\t\tmkdir D untracked &&\n+\t\t# Commit 2 files to have enough work for 2 parallel workers\n+\t\ttest_commit D/A &&\n+\t\ttest_commit D/B &&\n+\t\trm -rf D &&\n+\t\tln -s untracked D &&\n+\n+\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n+\t\t! test -h D &&\n+\t\tgrep D/A D/A.t &&\n+\t\tgrep D/B D/B.t\n+\t)\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"423382","messageId":"eae6d3a1c16e440f18fd60d69b061d15ffbfe8e7.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v2 5/8] parallel-checkout: add tests related to path collisions","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:32Z","receivedAt":"2021-04-30T21:41:02Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to confirm that path collisions are properly detected by\ncheckout workers, both to avoid race conditions and to report colliding\nentries on clone.\n\nCo-authored-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n parallel-checkout.c | 4 +\n t/lib-parallel-checkout.sh | 4 +-\n t/t2081-parallel-checkout-collisions.sh | 162 ++++++++++++++++++++++++\n 3 files changed, 168 insertions(+), 2 deletions(-)\n create mode 100755 t/t2081-parallel-checkout-collisions.sh\n\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 09e8b10a35..6fb3f1e6c9 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -8,6 +8,7 @@\n #include \"sigchain.h\"\n #include \"streaming.h\"\n #include \"thread-utils.h\"\n+#include \"trace2.h\"\n \n struct pc_worker {\n \tstruct child_process cp;\n@@ -326,6 +327,7 @@ void write_pc_item(struct parallel_checkout_item *pc_item,\n \tif (dir_sep && !has_dirs_only_path(path.buf, dir_sep - path.buf,\n \t\t\t\t\t state->base_dir_len)) {\n \t\tpc_item->status = PC_ITEM_COLLIDED;\n+\t\ttrace2_data_string(\"pcheckout\", NULL, \"collision/dirname\", path.buf);\n \t\tgoto out;\n \t}\n \n@@ -341,6 +343,8 @@ void write_pc_item(struct parallel_checkout_item *pc_item,\n \t\t\t * call should have already caught these cases.\n \t\t\t */\n \t\t\tpc_item->status = PC_ITEM_COLLIDED;\n+\t\t\ttrace2_data_string(\"pcheckout\", NULL,\n+\t\t\t\t\t \"collision/basename\", path.buf);\n \t\t} else {\n \t\t\terror_errno(\"failed to open file '%s'\", path.buf);\n \t\t\tpc_item->status = PC_ITEM_FAILED;\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex f60b22ef34..d6740425b1 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -22,12 +22,12 @@ test_checkout_workers () {\n \n \tlocal trace_file=trace-test-checkout-workers &&\n \trm -f \"$trace_file\" &&\n-\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" &&\n+\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n \n \tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n \ttest $workers -eq $expected_workers &&\n \trm \"$trace_file\"\n-}\n+} 8>&2 2>&4\n \n # Verify that both the working tree and the index were created correctly\n verify_checkout () {\ndiff --git a/t/t2081-parallel-checkout-collisions.sh b/t/t2081-parallel-checkout-collisions.sh\nnew file mode 100755\nindex 0000000000..f6fcfc0c1e\n--- /dev/null\n+++ b/t/t2081-parallel-checkout-collisions.sh\n@@ -0,0 +1,162 @@\n+#!/bin/sh\n+\n+test_description=\"path collisions during parallel checkout\n+\n+Parallel checkout must detect path collisions to:\n+\n+1) Avoid racily writing to different paths that represent the same file on disk.\n+2) Report the colliding entries on clone.\n+\n+The tests in this file exercise parallel checkout's collision detection code in\n+both these mechanics.\n+\"\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+\n+TEST_ROOT=\"$PWD\"\n+\n+test_expect_success CASE_INSENSITIVE_FS 'setup' '\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t100644 $empty_oid\tFILE_X\n+\t100644 $empty_oid\tFILE_x\n+\t100644 $empty_oid\tfile_X\n+\t100644 $empty_oid\tfile_x\n+\tEOF\n+\tgit update-index --index-info >filter.log\n+\tEOF\n+'\n+\n+test_workers_in_event_trace ()\n+{\n+\ttest $1 -eq $(grep \".event.:.child_start..*checkout--worker\" $2 | wc -l)\n+}\n+\n+test_expect_success CASE_INSENSITIVE_FS 'worker detects basename collision' '\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git \\\n+\t\t-c checkout.workers=2 -c checkout.thresholdForParallelism=0 \\\n+\t\tcheckout . &&\n+\n+\ttest_workers_in_event_trace 2 trace &&\n+\tcollisions=$(grep -i \"category.:.pcheckout.,.key.:.collision/basename.,.value.:.file_x.}\" trace | wc -l) &&\n+\ttest $collisions -eq 3\n+'\n+\n+test_expect_success CASE_INSENSITIVE_FS 'worker detects dirname collision' '\n+\ttest_config filter.logger.smudge \"\\\"$TEST_ROOT/logger_script\\\" %f\" &&\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t100644 $empty_oid\tA/B\n+\t100644 $empty_oid\tA/C\n+\t100644 $empty_oid\ta\n+\t100644 $attr_oid\t.gitattributes\n+\tEOF\n+\tgit rm -rf . &&\n+\tgit update-index --index-info expected.log &&\n+\ttest_cmp filter.log expected.log &&\n+\n+\t# Check that it used the right number of workers and detected the collisions\n+\ttest_workers_in_event_trace 2 trace &&\n+\tgrep \"category.:.pcheckout.,.key.:.collision/dirname.,.value.:.A/B.}\" trace &&\n+\tgrep \"category.:.pcheckout.,.key.:.collision/dirname.,.value.:.A/C.}\" trace\n+'\n+\n+test_expect_success SYMLINKS,CASE_INSENSITIVE_FS 'do not follow symlinks colliding with leading dir' '\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t120000 $symlink_oid\tD\n+\t100644 $empty_oid\td/x\n+\t100644 $empty_oid\te/y\n+\tEOF\n+\tgit rm -rf . &&\n+\tgit update-index --index-info stderr &&\n+\n+\tgrep FILE_X stderr &&\n+\tgrep FILE_x stderr &&\n+\tgrep file_X stderr &&\n+\tgrep file_x stderr &&\n+\tgrep \"the following paths have collided\" stderr\n+'\n+\n+# This test ensures that the collision report code is correctly looking for\n+# colliding peers in the second half of the cache_entry array. This is done by\n+# defining a smudge command for the *last* array entry, which makes it\n+# non-eligible for parallel-checkout. Thus, it is checked out *first*, before\n+# spawning the workers.\n+#\n+# Note: this test doesn't work on Windows because, on this system, the\n+# collision report code uses strcmp() to find the colliding pairs when\n+# core.ignoreCase is false. And we need this setting for this test so that only\n+# 'file_x' matches the pattern of the filter attribute. But the test works on\n+# OSX, where the colliding pairs are found using inode.\n+#\n+test_expect_success CASE_INSENSITIVE_FS,!MINGW,!CYGWIN \\\n+\t'collision report on clone (w/ colliding peer after the detected entry)' '\n+\n+\ttest_config_global filter.logger.smudge \"\\\"$TEST_ROOT/logger_script\\\" %f\" &&\n+\tgit reset --hard basename_collision &&\n+\techo \"file_x filter=logger\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\tgit commit -m \"filter for file_x\" &&\n+\n+\trm -rf clone-repo &&\n+\tset_checkout_config 2 0 &&\n+\ttest_checkout_workers 2 \\\n+\t\tgit -c core.ignoreCase=false clone . clone-repo 2>stderr &&\n+\n+\tgrep FILE_X stderr &&\n+\tgrep FILE_x stderr &&\n+\tgrep file_X stderr &&\n+\tgrep file_x stderr &&\n+\tgrep \"the following paths have collided\" stderr &&\n+\n+\t# Check that only \"file_x\" was filtered\n+\techo file_x >expected.log &&\n+\ttest_cmp clone-repo/filter.log expected.log\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"423383","messageId":"9161cd1503cc1a43b91bd899746b406e460b427b.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v2 6/8] t0028: extract encoding helpers to lib-encoding.sh","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:33Z","receivedAt":"2021-04-30T21:41:02Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The following patch will add tests outside t0028 which will also need to\nre-encode some strings. Extract the auxiliary encoding functions from\nt0028 to a common lib file so that they can be reused.\n\nSigned-off-by: Matheus Tavares \n---\n t/lib-encoding.sh | 25 +++++++++++++++++++++++++\n t/t0028-working-tree-encoding.sh | 25 +------------------------\n 2 files changed, 26 insertions(+), 24 deletions(-)\n create mode 100644 t/lib-encoding.sh\n\ndiff --git a/t/lib-encoding.sh b/t/lib-encoding.sh\nnew file mode 100644\nindex 0000000000..2dabc8c73e\n--- /dev/null\n+++ b/t/lib-encoding.sh\n@@ -0,0 +1,25 @@\n+# Encoding helpers\n+\n+test_lazy_prereq NO_UTF16_BOM '\n+\ttest $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6\n+'\n+\n+test_lazy_prereq NO_UTF32_BOM '\n+\ttest $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12\n+'\n+\n+write_utf16 () {\n+\tif test_have_prereq NO_UTF16_BOM\n+\tthen\n+\t\tprintf '\\376\\377'\n+\tfi &&\n+\ticonv -f UTF-8 -t UTF-16\n+}\n+\n+write_utf32 () {\n+\tif test_have_prereq NO_UTF32_BOM\n+\tthen\n+\t\tprintf '\\0\\0\\376\\377'\n+\tfi &&\n+\ticonv -f UTF-8 -t UTF-32\n+}\ndiff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\nindex f970a9806b..82905a2156 100755\n--- a/t/t0028-working-tree-encoding.sh\n+++ b/t/t0028-working-tree-encoding.sh\n@@ -6,33 +6,10 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-encoding.sh\"\n \n GIT_TRACE_WORKING_TREE_ENCODING=1 && export GIT_TRACE_WORKING_TREE_ENCODING\n \n-test_lazy_prereq NO_UTF16_BOM '\n-\ttest $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6\n-'\n-\n-test_lazy_prereq NO_UTF32_BOM '\n-\ttest $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12\n-'\n-\n-write_utf16 () {\n-\tif test_have_prereq NO_UTF16_BOM\n-\tthen\n-\t\tprintf '\\376\\377'\n-\tfi &&\n-\ticonv -f UTF-8 -t UTF-16\n-}\n-\n-write_utf32 () {\n-\tif test_have_prereq NO_UTF32_BOM\n-\tthen\n-\t\tprintf '\\0\\0\\376\\377'\n-\tfi &&\n-\ticonv -f UTF-8 -t UTF-32\n-}\n-\n test_expect_success 'setup test files' '\n \tgit config core.eol lf &&\n \n-- \n2.30.1\n\n"},{"id":"423384","messageId":"bc584897e648cd95ce3c003b5a61d62604bac9e3.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v2 7/8] parallel-checkout: add tests related to .gitattributes","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:34Z","receivedAt":"2021-04-30T21:41:03Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to confirm that the `struct conv_attrs` data is correctly\npassed from the main process to the workers, and that they can properly\nconvert the blobs before writing them to the working tree.\n\nAlso check that parallel-ineligible entries, such as regular files that\nrequire external filters, are correctly smudge and written when\nparallel-checkout is enabled.\n\nCo-authored-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n t/t2082-parallel-checkout-attributes.sh | 194 ++++++++++++++++++++++++\n 1 file changed, 194 insertions(+)\n create mode 100755 t/t2082-parallel-checkout-attributes.sh\n\ndiff --git a/t/t2082-parallel-checkout-attributes.sh b/t/t2082-parallel-checkout-attributes.sh\nnew file mode 100755\nindex 0000000000..2525457961\n--- /dev/null\n+++ b/t/t2082-parallel-checkout-attributes.sh\n@@ -0,0 +1,194 @@\n+#!/bin/sh\n+\n+test_description='parallel-checkout: attributes\n+\n+Verify that parallel-checkout correctly creates files that require\n+conversions, as specified in .gitattributes. The main point here is\n+to check that the conv_attr data is correctly sent to the workers\n+and that it contains sufficient information to smudge files\n+properly (without access to the index or attribute stack).\n+'\n+\n+TEST_NO_CREATE_REPO=1\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+. \"$TEST_DIRECTORY/lib-encoding.sh\"\n+\n+test_expect_success 'parallel-checkout with ident' '\n+\tset_checkout_config 2 0 &&\n+\tgit init ident &&\n+\t(\n+\t\tcd ident &&\n+\t\techo \"A ident\" >.gitattributes &&\n+\t\techo \"\\$Id\\$\" >A &&\n+\t\techo \"\\$Id\\$\" >B &&\n+\t\tgit add -A &&\n+\t\tgit commit -m id &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git reset --hard &&\n+\t\thexsz=$(test_oid hexsz) &&\n+\t\tgrep -E \"\\\\\\$Id: [0-9a-f]{$hexsz} \\\\\\$\" A &&\n+\t\tgrep \"\\\\\\$Id\\\\\\$\" B\n+\t)\n+'\n+\n+test_expect_success 'parallel-checkout with re-encoding' '\n+\tset_checkout_config 2 0 &&\n+\tgit init encoding &&\n+\t(\n+\t\tcd encoding &&\n+\t\techo text >utf8-text &&\n+\t\twrite_utf16 utf16-text &&\n+\n+\t\techo \"A working-tree-encoding=UTF-16\" >.gitattributes &&\n+\t\tcp utf16-text A &&\n+\t\tcp utf8-text B &&\n+\t\tgit add A B .gitattributes &&\n+\t\tgit commit -m encoding &&\n+\n+\t\t# Check that A is stored in UTF-8\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp_bin utf8-text A.internal &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git checkout A B &&\n+\n+\t\t# Check that A (and only A) is re-encoded during checkout\n+\t\ttest_cmp_bin utf16-text A &&\n+\t\ttest_cmp_bin utf8-text B\n+\t)\n+'\n+\n+test_expect_success 'parallel-checkout with eol conversions' '\n+\tset_checkout_config 2 0 &&\n+\tgit init eol &&\n+\t(\n+\t\tcd eol &&\n+\t\tprintf \"multi\\r\\nline\\r\\ntext\" >crlf-text &&\n+\t\tprintf \"multi\\nline\\ntext\" >lf-text &&\n+\n+\t\tgit config core.autocrlf false &&\n+\t\techo \"A eol=crlf\" >.gitattributes &&\n+\t\tcp crlf-text A &&\n+\t\tcp lf-text B &&\n+\t\tgit add A B .gitattributes &&\n+\t\tgit commit -m eol &&\n+\n+\t\t# Check that A is stored with LF format\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp_bin lf-text A.internal &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git checkout A B &&\n+\n+\t\t# Check that A (and only A) is converted to CRLF during checkout\n+\t\ttest_cmp_bin crlf-text A &&\n+\t\ttest_cmp_bin lf-text B\n+\t)\n+'\n+\n+# Entries that require an external filter are not eligible for parallel\n+# checkout. Check that both the parallel-eligible and non-eligible entries are\n+# properly writen in a single checkout operation.\n+#\n+test_expect_success 'parallel-checkout and external filter' '\n+\tset_checkout_config 2 0 &&\n+\tgit init filter &&\n+\t(\n+\t\tcd filter &&\n+\t\twrite_script <<-\\EOF rot13.sh &&\n+\t\ttr \\\n+\t\t \"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ\" \\\n+\t\t \"nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM\"\n+\t\tEOF\n+\n+\t\tgit config filter.rot13.clean \"\\\"$(pwd)/rot13.sh\\\"\" &&\n+\t\tgit config filter.rot13.smudge \"\\\"$(pwd)/rot13.sh\\\"\" &&\n+\t\tgit config filter.rot13.required true &&\n+\n+\t\techo abcd >original &&\n+\t\techo nopq >rot13 &&\n+\n+\t\techo \"A filter=rot13\" >.gitattributes &&\n+\t\tcp original A &&\n+\t\tcp original B &&\n+\t\tcp original C &&\n+\t\tgit add A B C .gitattributes &&\n+\t\tgit commit -m filter &&\n+\n+\t\t# Check that A (and only A) was cleaned\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp rot13 A.internal &&\n+\t\tgit cat-file -p :B >B.internal &&\n+\t\ttest_cmp original B.internal &&\n+\t\tgit cat-file -p :C >C.internal &&\n+\t\ttest_cmp original C.internal &&\n+\n+\t\trm A B C *.internal &&\n+\t\ttest_checkout_workers 2 git checkout A B C &&\n+\n+\t\t# Check that A (and only A) was smudged during checkout\n+\t\ttest_cmp original A &&\n+\t\ttest_cmp original B &&\n+\t\ttest_cmp original C\n+\t)\n+'\n+\n+# The delayed queue is independent from the parallel queue, and they should be\n+# able to work together in the same checkout process.\n+#\n+test_expect_success PERL 'parallel-checkout and delayed checkout' '\n+\twrite_script rot13-filter.pl \"$PERL_PATH\" \\\n+\t\t<\"$TEST_DIRECTORY\"/t0021/rot13-filter.pl &&\n+\n+\ttest_config_global filter.delay.process \\\n+\t\t\"\\\"$(pwd)/rot13-filter.pl\\\" --always-delay \\\"$(pwd)/delayed.log\\\" clean smudge delay\" &&\n+\ttest_config_global filter.delay.required true &&\n+\n+\techo \"abcd\" >original &&\n+\techo \"nopq\" >rot13 &&\n+\n+\tgit init delayed &&\n+\t(\n+\t\tcd delayed &&\n+\t\techo \"*.d filter=delay\" >.gitattributes &&\n+\t\tcp ../original W.d &&\n+\t\tcp ../original X.d &&\n+\t\tcp ../original Y &&\n+\t\tcp ../original Z &&\n+\t\tgit add -A &&\n+\t\tgit commit -m delayed &&\n+\n+\t\t# Check that *.d files were cleaned\n+\t\tgit cat-file -p :W.d >W.d.internal &&\n+\t\ttest_cmp W.d.internal ../rot13 &&\n+\t\tgit cat-file -p :X.d >X.d.internal &&\n+\t\ttest_cmp X.d.internal ../rot13 &&\n+\t\tgit cat-file -p :Y >Y.internal &&\n+\t\ttest_cmp Y.internal ../original &&\n+\t\tgit cat-file -p :Z >Z.internal &&\n+\t\ttest_cmp Z.internal ../original &&\n+\n+\t\trm *\n+\t) &&\n+\n+\tset_checkout_config 2 0 &&\n+\ttest_checkout_workers 2 git -C delayed checkout -f &&\n+\tverify_checkout delayed &&\n+\n+\t# Check that the *.d files got to the delay queue and were filtered\n+\tgrep \"smudge W.d .* \\[DELAYED\\]\" delayed.log &&\n+\tgrep \"smudge X.d .* \\[DELAYED\\]\" delayed.log &&\n+\ttest_cmp delayed/W.d original &&\n+\ttest_cmp delayed/X.d original &&\n+\n+\t# Check that the parallel-eligible entries went to the right queue and\n+\t# were not filtered\n+\t! grep \"smudge Y .* \\[DELAYED\\]\" delayed.log &&\n+\t! grep \"smudge Z .* \\[DELAYED\\]\" delayed.log &&\n+\ttest_cmp delayed/Y original &&\n+\ttest_cmp delayed/Z original\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"423385","messageId":"1bc5c523c5f8522facf5c08b07fd58fb4a99987f.1619818517.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v2 8/8] ci: run test round with parallel-checkout enabled","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-30T21:40:35Z","receivedAt":"2021-04-30T21:41:04Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"We already have tests for the basic parallel-checkout operations. But\nthis code can also run be executed by other commands, such as\ngit-read-tree and git-sparse-checkout, which are currently not tested\nwith multiple workers. To promote a wider test coverage without\nduplicating tests:\n\n1. Add the GIT_TEST_CHECKOUT_WORKERS environment variable, to optionally\n force parallel-checkout execution during the whole test suite.\n\n2. Set this variable (with a value of 2) in the second test round of our\n linux-gcc CI job. This round runs `make test` again with some\n optional GIT_TEST_* variables enabled, so there is no additional\n overhead in exercising the parallel-checkout code here.\n\nNote that tests checking out less than two parallel-eligible entries\nwill fall back to the sequential mode. Nevertheless, it's still a good\nexercise for the parallel-checkout framework as the fallback codepath\nalso writes the queued entries using the parallel-checkout functions\n(only without spawning any worker).\n\nSigned-off-by: Matheus Tavares \n---\n ci/run-build-and-tests.sh | 1 +\n parallel-checkout.c | 14 ++++++++++++++\n t/README | 4 ++++\n t/lib-parallel-checkout.sh | 3 +++\n 4 files changed, 22 insertions(+)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex a66b5e8c75..23b28e7391 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -25,6 +25,7 @@ linux-gcc)\n \texport GIT_TEST_ADD_I_USE_BUILTIN=1\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n \texport GIT_TEST_WRITE_REV_INDEX=1\n+\texport GIT_TEST_CHECKOUT_WORKERS=2\n \tmake test\n \t;;\n linux-clang)\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 6fb3f1e6c9..6b1af32bb3 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -35,6 +35,20 @@ static const int DEFAULT_NUM_WORKERS = 1;\n \n void get_parallel_checkout_configs(int *num_workers, int *threshold)\n {\n+\tchar *env_workers = getenv(\"GIT_TEST_CHECKOUT_WORKERS\");\n+\n+\tif (env_workers && *env_workers) {\n+\t\tif (strtol_i(env_workers, 10, num_workers)) {\n+\t\t\tdie(\"invalid value for GIT_TEST_CHECKOUT_WORKERS: '%s'\",\n+\t\t\t env_workers);\n+\t\t}\n+\t\tif (*num_workers < 1)\n+\t\t\t*num_workers = online_cpus();\n+\n+\t\t*threshold = 0;\n+\t\treturn;\n+\t}\n+\n \tif (git_config_get_int(\"checkout.workers\", num_workers))\n \t\t*num_workers = DEFAULT_NUM_WORKERS;\n \telse if (*num_workers < 1)\ndiff --git a/t/README b/t/README\nindex fd9375b146..a194488f27 100644\n--- a/t/README\n+++ b/t/README\n@@ -436,6 +436,10 @@ and \"sha256\".\n GIT_TEST_WRITE_REV_INDEX=, when true enables the\n 'pack.writeReverseIndex' setting.\n \n+GIT_TEST_CHECKOUT_WORKERS= overrides the 'checkout.workers' setting\n+to and 'checkout.thresholdForParallelism' to 0, forcing the\n+execution of the parallel-checkout code.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex d6740425b1..21f5759732 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -1,5 +1,8 @@\n # Helpers for tests invoking parallel-checkout\n \n+# Parallel checkout tests need full control of the number of workers\n+unset GIT_TEST_CHECKOUT_WORKERS\n+\n set_checkout_config () {\n \tif test $# -ne 2\n \tthen\n-- \n2.30.1\n\n"},{"id":"423417","messageId":"CAP8UFD1dz=u-nXyxSKArP-fAiX6mq3FV+oiiKCHCqbWMMf4TWw@mail.gmail.com","threadId":"55540","inReplyTo":"f870040bfb3e73ee8cd27352b0acc65bb54be560.1619818517.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 1/8] make_transient_cache_entry(): optionally alloc from mem_pool","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-05-01T17:06:31Z","receivedAt":"2021-05-01T17:06:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"> struct cache_entry *make_transient_cache_entry(unsigned int mode,\n> const struct object_id *oid,\n> const char *path,\n> - int stage);\n> + int stage, struct mem_pool *mp);\n\nIt's a bit strange that `int stage` isn't on its own line here, as\nother parameters are. And if line length was the issue, it looks like\nit could have been on the same line as `const char *path`.\n\n> -struct cache_entry *make_transient_cache_entry(unsigned int mode, const struct object_id *oid,\n> - const char *path, int stage)\n> +struct cache_entry *make_transient_cache_entry(unsigned int mode,\n> + const struct object_id *oid,\n> + const char *path, int stage,\n\nHere also, it's a bit strange that `int stage` isn't on its own line,\nas it looks like you want to put others parameters on their own line.\nAnd this is not consistent with the above declaration.\n\n> + struct mem_pool *mp)\n"},{"id":"423418","messageId":"CAP8UFD16N3MiYawrRLCxsrYuo4KkvD3SS5F4UqyEF37j4T9yVA@mail.gmail.com","threadId":"55540","inReplyTo":"e2d82c4337dc002d7296454c59d4006c3bb43c27.1619818517.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 2/8] builtin/checkout.c: complete parallel checkout support","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-05-01T17:08:07Z","receivedAt":"2021-05-01T17:08:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Apr 30, 2021 at 11:40 PM Matheus Tavares\n wrote:\n>\n> Pathspec-limited checkouts (like `git checkout *.txt`) are performed by\n> a code path that doesn't yet support parallel checkout because it calls\n> checkout_entry() directly, instead of unpack_trees(). Let's add parallel\n> checkout support for this code path too.\n>\n> Note: the transient cache entries allocated in checkout_merged() are now\n\ns/Note: the/The/\n\n> allocated in a mem_pool which is only discarded after parallel checkout\n> finishes. This is done because the entries need to be valid when\n> run_parallel_checkout() is called.\n\n> -static int checkout_merged(int pos, const struct checkout *state, int *nr_checkouts)\n> +static int checkout_merged(int pos, const struct checkout *state,\n> + int *nr_checkouts, struct mem_pool *ce_mem_pool)\n\nFor consistency with the previous patch, maybe: s/ce_mem_pool/ce_mp/\n"},{"id":"423419","messageId":"CAP8UFD1X6ZZXCMfEDfJDjr0mB2XwQAvuhTAT0beJka-QR2k6nQ@mail.gmail.com","threadId":"55540","inReplyTo":"0fe1a5fabc8cb5f7c2421afaefae030d399d28ed.1619818517.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 3/8] checkout-index: add parallel checkout support","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-05-01T17:08:32Z","receivedAt":"2021-05-01T17:08:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Apr 30, 2021 at 11:40 PM Matheus Tavares\n wrote:\n>\n> Note: previously, `checkout_all()` would not return on errors, but\n\ns/Note: previously/Previously/\n\n> instead call `exit()` with a non-zero code. However, it only did that\n> after calling `checkout_entry()` for all index entries, thus not\n> stopping on the first error, but attempting to write the maximum number\n> of entries possible. In order to maintain this behavior we now propagate\n> `checkout_all()`s error status to `cmd_checkout_index()`, so that it can\n> call `run_parallel_checkout()` and attempt to write the queued entries\n> before exiting with the error code.\n>\n> Signed-off-by: Matheus Tavares \n\n> @@ -142,11 +143,7 @@ static void checkout_all(const char *prefix, int prefix_length)\n> }\n> if (last_ce && to_tempfile)\n> write_tempfile_record(last_ce->name, prefix);\n> - if (errs)\n> - /* we have already done our error reporting.\n> - * exit with the same code as die().\n> - */\n> - exit(128);\n\nSo when there were errors in checkout_all(), we used to exit() with\nerror code 128 (same as die())...\n\n> @@ -275,12 +277,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n> strbuf_release(&buf);\n> }\n>\n> + if (all)\n> + err |= checkout_all(prefix, prefix_length);\n> +\n> + if (pc_workers > 1)\n> + err |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n> + NULL, NULL);\n> +\n> if (err)\n> return 1;\n\n...but now it looks like we will exit with error code 1. I see that\nyou already answered this comment in the previous round of review, but\nyou didn't add the explanations to the commit message.\n"},{"id":"423442","messageId":"20210502075920.d2rdmuix7r34sas2@tb-raspi4","threadId":"55540","inReplyTo":"eae6d3a1c16e440f18fd60d69b061d15ffbfe8e7.1619818517.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 5/8] parallel-checkout: add tests related to path collisions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-05-02T07:59:20Z","receivedAt":"2021-05-02T07:59:28Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Apr 30, 2021 at 06:40:32PM -0300, Matheus Tavares wrote:\n> Add tests to confirm that path collisions are properly detected by\n> checkout workers, both to avoid race conditions and to report colliding\n> entries on clone.\n>\n> Co-authored-by: Jeff Hostetler \n> Signed-off-by: Matheus Tavares \n> ---\n> parallel-checkout.c | 4 +\n> t/lib-parallel-checkout.sh | 4 +-\n> t/t2081-parallel-checkout-collisions.sh | 162 ++++++++++++++++++++++++\n> 3 files changed, 168 insertions(+), 2 deletions(-)\n> create mode 100755 t/t2081-parallel-checkout-collisions.sh\n>\n> diff --git a/parallel-checkout.c b/parallel-checkout.c\n> index 09e8b10a35..6fb3f1e6c9 100644\n> --- a/parallel-checkout.c\n> +++ b/parallel-checkout.c\n> @@ -8,6 +8,7 @@\n> #include \"sigchain.h\"\n> #include \"streaming.h\"\n> #include \"thread-utils.h\"\n> +#include \"trace2.h\"\n>\n> struct pc_worker {\n> \tstruct child_process cp;\n> @@ -326,6 +327,7 @@ void write_pc_item(struct parallel_checkout_item *pc_item,\n> \tif (dir_sep && !has_dirs_only_path(path.buf, dir_sep - path.buf,\n> \t\t\t\t\t state->base_dir_len)) {\n> \t\tpc_item->status = PC_ITEM_COLLIDED;\n> +\t\ttrace2_data_string(\"pcheckout\", NULL, \"collision/dirname\", path.buf);\n> \t\tgoto out;\n> \t}\n>\n> @@ -341,6 +343,8 @@ void write_pc_item(struct parallel_checkout_item *pc_item,\n> \t\t\t * call should have already caught these cases.\n> \t\t\t */\n> \t\t\tpc_item->status = PC_ITEM_COLLIDED;\n> +\t\t\ttrace2_data_string(\"pcheckout\", NULL,\n> +\t\t\t\t\t \"collision/basename\", path.buf);\n> \t\t} else {\n> \t\t\terror_errno(\"failed to open file '%s'\", path.buf);\n> \t\t\tpc_item->status = PC_ITEM_FAILED;\n> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> index f60b22ef34..d6740425b1 100644\n> --- a/t/lib-parallel-checkout.sh\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -22,12 +22,12 @@ test_checkout_workers () {\n>\n> \tlocal trace_file=trace-test-checkout-workers &&\n> \trm -f \"$trace_file\" &&\n> -\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" &&\n> +\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n>\n> \tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n> \ttest $workers -eq $expected_workers &&\n> \trm \"$trace_file\"\n> -}\n> +} 8>&2 2>&4\n>\n> # Verify that both the working tree and the index were created correctly\n> verify_checkout () {\n> diff --git a/t/t2081-parallel-checkout-collisions.sh b/t/t2081-parallel-checkout-collisions.sh\n> new file mode 100755\n> index 0000000000..f6fcfc0c1e\n> --- /dev/null\n> +++ b/t/t2081-parallel-checkout-collisions.sh\n> @@ -0,0 +1,162 @@\n> +#!/bin/sh\n> +\n> +test_description=\"path collisions during parallel checkout\n> +\n> +Parallel checkout must detect path collisions to:\n> +\n> +1) Avoid racily writing to different paths that represent the same file on disk.\n> +2) Report the colliding entries on clone.\n> +\n> +The tests in this file exercise parallel checkout's collision detection code in\n> +both these mechanics.\n> +\"\n> +\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n\nWhy this $TEST_DIRECTORY ?\nAren't all files under the t/ directory, where test-lib.sh is as well ?\n(The $TEST_DIRECTORY macro is used at different places, so I may have missed\nsomething)\n\n> +\n> +TEST_ROOT=\"$PWD\"\n> +\n> +test_expect_success CASE_INSENSITIVE_FS 'setup' '\n> +\tempty_oid=$(git hash-object -w --stdin +\tcat >objs <<-EOF &&\n> +\t100644 $empty_oid\tFILE_X\n> +\t100644 $empty_oid\tFILE_x\n> +\t100644 $empty_oid\tfile_X\n> +\t100644 $empty_oid\tfile_x\n> +\tEOF\n> +\tgit update-index --index-info +\tgit commit -m \"colliding files\" &&\n> +\tgit tag basename_collision &&\n> +\n> +\twrite_script \"$TEST_ROOT\"/logger_script <<-\\EOF\n> +\techo \"$@\" >>filter.log\n> +\tEOF\n> +'\n> +\n> +test_workers_in_event_trace ()\n> +{\n> +\ttest $1 -eq $(grep \".event.:.child_start..*checkout--worker\" $2 | wc -l)\n> +}\n> +\n> +test_expect_success CASE_INSENSITIVE_FS 'worker detects basename collision' '\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git \\\n> +\t\t-c checkout.workers=2 -c checkout.thresholdForParallelism=0 \\\n> +\t\tcheckout . &&\n> +\n> +\ttest_workers_in_event_trace 2 trace &&\n> +\tcollisions=$(grep -i \"category.:.pcheckout.,.key.:.collision/basename.,.value.:.file_x.}\" trace | wc -l) &&\n> +\ttest $collisions -eq 3\n> +'\n> +\n> +test_expect_success CASE_INSENSITIVE_FS 'worker detects dirname collision' '\n> +\ttest_config filter.logger.smudge \"\\\"$TEST_ROOT/logger_script\\\" %f\" &&\n> +\tempty_oid=$(git hash-object -w --stdin +\n> +\t# By setting a filter command to \"a\", we make it ineligible for parallel\n> +\t# checkout, and thus it is checked out *first*. This way we can ensure\n> +\t# that \"A/B\" and \"A/C\" will both collide with the regular file \"a\".\n> +\t#\n> +\tattr_oid=$(echo \"a filter=logger\" | git hash-object -w --stdin) &&\n> +\n> +\tcat >objs <<-EOF &&\n> +\t100644 $empty_oid\tA/B\n> +\t100644 $empty_oid\tA/C\n> +\t100644 $empty_oid\ta\n> +\t100644 $attr_oid\t.gitattributes\n> +\tEOF\n> +\tgit rm -rf . &&\n> +\tgit update-index --index-info +\n> +\trm -f trace filter.log &&\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git \\\n> +\t\t-c checkout.workers=2 -c checkout.thresholdForParallelism=0 \\\n> +\t\tcheckout . &&\n> +\n> +\t# Check that \"a\" (and only \"a\") was filtered\n> +\techo a >expected.log &&\n> +\ttest_cmp filter.log expected.log &&\n> +\n> +\t# Check that it used the right number of workers and detected the collisions\n> +\ttest_workers_in_event_trace 2 trace &&\n> +\tgrep \"category.:.pcheckout.,.key.:.collision/dirname.,.value.:.A/B.}\" trace &&\n> +\tgrep \"category.:.pcheckout.,.key.:.collision/dirname.,.value.:.A/C.}\" trace\n> +'\n> +\n> +test_expect_success SYMLINKS,CASE_INSENSITIVE_FS 'do not follow symlinks colliding with leading dir' '\n> +\tempty_oid=$(git hash-object -w --stdin +\tsymlink_oid=$(echo \"./e\" | git hash-object -w --stdin) &&\n> +\tmkdir e &&\n> +\n> +\tcat >objs <<-EOF &&\n> +\t120000 $symlink_oid\tD\n> +\t100644 $empty_oid\td/x\n> +\t100644 $empty_oid\te/y\n> +\tEOF\n> +\tgit rm -rf . &&\n> +\tgit update-index --index-info +\n> +\tset_checkout_config 2 0 &&\n> +\ttest_checkout_workers 2 git checkout . &&\n> +\ttest_path_is_dir e &&\n> +\ttest_path_is_missing e/x\n> +'\n> +\n> +# The two following tests check that parallel checkout correctly reports\n> +# colliding entries on clone. The sequential code detects a collision by\n> +# calling lstat() before trying to open(O_CREAT) a file. (Note that this only\n> +# works for clone.) Then, to find the pair of a colliding item k, it searches\n> +# cache_entry[0, k-1]. This is not sufficient in parallel checkout because:\n> +#\n> +# - A colliding file may be created between the lstat() and open() calls;\n> +# - A colliding entry might appear in the second half of the cache_entry array.\n> +#\n> +test_expect_success CASE_INSENSITIVE_FS 'collision report on clone (w/ racy file creation)' '\n> +\tgit reset --hard basename_collision &&\n> +\tset_checkout_config 2 0 &&\n> +\ttest_checkout_workers 2 git clone . clone-repo 2>stderr &&\n> +\n> +\tgrep FILE_X stderr &&\n> +\tgrep FILE_x stderr &&\n> +\tgrep file_X stderr &&\n> +\tgrep file_x stderr &&\n> +\tgrep \"the following paths have collided\" stderr\n> +'\n> +\n> +# This test ensures that the collision report code is correctly looking for\n> +# colliding peers in the second half of the cache_entry array. This is done by\n> +# defining a smudge command for the *last* array entry, which makes it\n> +# non-eligible for parallel-checkout. Thus, it is checked out *first*, before\n> +# spawning the workers.\n> +#\n> +# Note: this test doesn't work on Windows because, on this system, the\n> +# collision report code uses strcmp() to find the colliding pairs when\n> +# core.ignoreCase is false. And we need this setting for this test so that only\n> +# 'file_x' matches the pattern of the filter attribute. But the test works on\n> +# OSX, where the colliding pairs are found using inode.\n> +#\n> +test_expect_success CASE_INSENSITIVE_FS,!MINGW,!CYGWIN \\\n> +\t'collision report on clone (w/ colliding peer after the detected entry)' '\n> +\n> +\ttest_config_global filter.logger.smudge \"\\\"$TEST_ROOT/logger_script\\\" %f\" &&\n> +\tgit reset --hard basename_collision &&\n> +\techo \"file_x filter=logger\" >.gitattributes &&\n> +\tgit add .gitattributes &&\n> +\tgit commit -m \"filter for file_x\" &&\n> +\n> +\trm -rf clone-repo &&\n> +\tset_checkout_config 2 0 &&\n> +\ttest_checkout_workers 2 \\\n> +\t\tgit -c core.ignoreCase=false clone . clone-repo 2>stderr &&\n> +\n> +\tgrep FILE_X stderr &&\n> +\tgrep FILE_x stderr &&\n> +\tgrep file_X stderr &&\n> +\tgrep file_x stderr &&\n> +\tgrep \"the following paths have collided\" stderr &&\n> +\n> +\t# Check that only \"file_x\" was filtered\n> +\techo file_x >expected.log &&\n> +\ttest_cmp clone-repo/filter.log expected.log\n> +'\n> +\n> +test_done\n> --\n> 2.30.1\n>\n"},{"id":"423443","messageId":"20210502101239.dyevkjcp7jzov2rd@tb-raspi4","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 0/8] Parallel Checkout (part 3)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-05-02T10:12:39Z","receivedAt":"2021-05-02T10:12:47Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Apr 30, 2021 at 06:40:27PM -0300, Matheus Tavares wrote:\n\n[]\n\nThank's for the work.\nI just make a quick test:\n\n\"swapping\" git.git between\n\nHEAD is now at e870325ee8 The third batch\nand\nHEAD is now at 7e39198978 The thirteenth batch\n\nOn a Desktop running Linux.\nUsing the git.git repo, mounted as nfs on an oldish NAS\nwith spinning disks (and WLAN) gave this timing:\n\ngit -c checkout.workers=1\n2 minutes, some seconds.\n\ngit -c checkout.workers=4\n1 minute, some seconds.\n\ngit -c checkout.workers=16\n40..44 seconds\n\nNot a scientific measurement, just a positive feedback from a real-world.\n\nGood work, I'm looking forward to have this feature in git.git\n\n\n"},{"id":"423490","messageId":"CAHd-oW6ErNfar43R4gt_i480xnrp8Nu8B6t+Aya_p11-ia8QGQ@mail.gmail.com","threadId":"55540","inReplyTo":"CAP8UFD1dz=u-nXyxSKArP-fAiX6mq3FV+oiiKCHCqbWMMf4TWw@mail.gmail.com","subject":"Re: [PATCH v2 1/8] make_transient_cache_entry(): optionally alloc from mem_pool","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-03T14:11:58Z","receivedAt":"2021-05-03T14:12:12Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, May 1, 2021 at 2:06 PM Christian Couder\n wrote:\n>\n> > struct cache_entry *make_transient_cache_entry(unsigned int mode,\n> > const struct object_id *oid,\n> > const char *path,\n> > - int stage);\n> > + int stage, struct mem_pool *mp);\n>\n> It's a bit strange that `int stage` isn't on its own line here, as\n> other parameters are. And if line length was the issue, it looks like\n> it could have been on the same line as `const char *path`.\n>\n> > -struct cache_entry *make_transient_cache_entry(unsigned int mode, const struct object_id *oid,\n> > - const char *path, int stage)\n> > +struct cache_entry *make_transient_cache_entry(unsigned int mode,\n> > + const struct object_id *oid,\n> > + const char *path, int stage,\n>\n> Here also, it's a bit strange that `int stage` isn't on its own line,\n> as it looks like you want to put others parameters on their own line.\n> And this is not consistent with the above declaration.\n\nThanks. I'll put each parameter in its own line and make this\nconsistent with the declaration.\n"},{"id":"423491","messageId":"CAHd-oW57tFTQtqKPhS7J=52yhcatd9wQh3sTHVAmjwvPdWw=HA@mail.gmail.com","threadId":"55540","inReplyTo":"CAP8UFD16N3MiYawrRLCxsrYuo4KkvD3SS5F4UqyEF37j4T9yVA@mail.gmail.com","subject":"Re: [PATCH v2 2/8] builtin/checkout.c: complete parallel checkout support","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-03T14:21:31Z","receivedAt":"2021-05-03T14:21:47Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, May 1, 2021 at 2:08 PM Christian Couder\n wrote:\n>\n> On Fri, Apr 30, 2021 at 11:40 PM Matheus Tavares\n> wrote:\n> >\n> > Pathspec-limited checkouts (like `git checkout *.txt`) are performed by\n> > a code path that doesn't yet support parallel checkout because it calls\n> > checkout_entry() directly, instead of unpack_trees(). Let's add parallel\n> > checkout support for this code path too.\n> >\n> > Note: the transient cache entries allocated in checkout_merged() are now\n>\n> s/Note: the/The/\n\nThanks\n\n> > allocated in a mem_pool which is only discarded after parallel checkout\n> > finishes. This is done because the entries need to be valid when\n> > run_parallel_checkout() is called.\n>\n> > -static int checkout_merged(int pos, const struct checkout *state, int *nr_checkouts)\n> > +static int checkout_merged(int pos, const struct checkout *state,\n> > + int *nr_checkouts, struct mem_pool *ce_mem_pool)\n>\n> For consistency with the previous patch, maybe: s/ce_mem_pool/ce_mp/\n\nYeah, I agree that it's a good idea to keep this consistent. In fact,\ninstead of changing `ce_mem_pool` here, I think I should change `mp`\nin the previous patch to either `mem_pool` or `ce_mem_pool`. Those\nseem to be the most common names, at read-cache.c, for a memory pool\nstruct holding cache entries.\n"},{"id":"423492","messageId":"CAHd-oW7oeoQmoqkWCYRvsJ9JbqdrefQhQceami7BgohLYmYRzQ@mail.gmail.com","threadId":"55540","inReplyTo":"CAP8UFD1X6ZZXCMfEDfJDjr0mB2XwQAvuhTAT0beJka-QR2k6nQ@mail.gmail.com","subject":"Re: [PATCH v2 3/8] checkout-index: add parallel checkout support","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-03T14:22:36Z","receivedAt":"2021-05-03T14:22:51Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, May 1, 2021 at 2:08 PM Christian Couder\n wrote:\n>\n> On Fri, Apr 30, 2021 at 11:40 PM Matheus Tavares\n> wrote:\n> >\n> > Note: previously, `checkout_all()` would not return on errors, but\n>\n> s/Note: previously/Previously/\n>\n> > instead call `exit()` with a non-zero code. However, it only did that\n> > after calling `checkout_entry()` for all index entries, thus not\n> > stopping on the first error, but attempting to write the maximum number\n> > of entries possible. In order to maintain this behavior we now propagate\n> > `checkout_all()`s error status to `cmd_checkout_index()`, so that it can\n> > call `run_parallel_checkout()` and attempt to write the queued entries\n> > before exiting with the error code.\n> >\n> > Signed-off-by: Matheus Tavares \n>\n> > @@ -142,11 +143,7 @@ static void checkout_all(const char *prefix, int prefix_length)\n> > }\n> > if (last_ce && to_tempfile)\n> > write_tempfile_record(last_ce->name, prefix);\n> > - if (errs)\n> > - /* we have already done our error reporting.\n> > - * exit with the same code as die().\n> > - */\n> > - exit(128);\n>\n> So when there were errors in checkout_all(), we used to exit() with\n> error code 128 (same as die())...\n>\n> > @@ -275,12 +277,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n> > strbuf_release(&buf);\n> > }\n> >\n> > + if (all)\n> > + err |= checkout_all(prefix, prefix_length);\n> > +\n> > + if (pc_workers > 1)\n> > + err |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n> > + NULL, NULL);\n> > +\n> > if (err)\n> > return 1;\n>\n> ...but now it looks like we will exit with error code 1. I see that\n> you already answered this comment in the previous round of review, but\n> you didn't add the explanations to the commit message.\n\nOops, good catch! I'll add the explanation for v3. Thanks.\n"},{"id":"423496","messageId":"CAHd-oW46GR8cWHPq65QHHfrsN-9MN1xO301eADwLigwaLq7Y-w@mail.gmail.com","threadId":"55540","inReplyTo":"20210502075920.d2rdmuix7r34sas2@tb-raspi4","subject":"Re: [PATCH v2 5/8] parallel-checkout: add tests related to path collisions","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-03T14:58:17Z","receivedAt":"2021-05-03T14:58:34Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sun, May 2, 2021 at 4:59 AM Torsten Bögershausen wrote:\n>\n> On Fri, Apr 30, 2021 at 06:40:32PM -0300, Matheus Tavares wrote:\n> >\n> > diff --git a/t/t2081-parallel-checkout-collisions.sh b/t/t2081-parallel-checkout-collisions.sh\n> > new file mode 100755\n> > index 0000000000..f6fcfc0c1e\n> > --- /dev/null\n> > +++ b/t/t2081-parallel-checkout-collisions.sh\n> > @@ -0,0 +1,162 @@\n> > +#!/bin/sh\n> > +\n> > +test_description=\"path collisions during parallel checkout\n> > +\n> > +Parallel checkout must detect path collisions to:\n> > +\n> > +1) Avoid racily writing to different paths that represent the same file on disk.\n> > +2) Report the colliding entries on clone.\n> > +\n> > +The tests in this file exercise parallel checkout's collision detection code in\n> > +both these mechanics.\n> > +\"\n> > +\n> > +. ./test-lib.sh\n> > +. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n>\n> Why this $TEST_DIRECTORY ?\n> Aren't all files under the t/ directory, where test-lib.sh is as well ?\n\nGood point. From what I understand, the reason why we need the macro\nhere is that, when running the test with --root=, the trash dir\nmight actually be at a path outside `t/`. So the test can't rely on\n`./` to read files from `t/`. We don't need the macro for\n`test-lib.sh` because the working dir is only changed after it is\nsourced (by `test-lib.sh` itself).\n"},{"id":"423497","messageId":"CAHd-oW4_1V91dFM9O3Q1S1D5mkLvWeyC+nwSyxd38dgqKhS8OQ@mail.gmail.com","threadId":"55540","inReplyTo":"20210502101239.dyevkjcp7jzov2rd@tb-raspi4","subject":"Re: [PATCH v2 0/8] Parallel Checkout (part 3)","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-03T15:01:20Z","receivedAt":"2021-05-03T15:01:39Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sun, May 2, 2021 at 7:12 AM Torsten Bögershausen wrote:\n>\n> On Fri, Apr 30, 2021 at 06:40:27PM -0300, Matheus Tavares wrote:\n>\n> []\n>\n> Thank's for the work.\n> I just make a quick test:\n>\n> \"swapping\" git.git between\n>\n> HEAD is now at e870325ee8 The third batch\n> and\n> HEAD is now at 7e39198978 The thirteenth batch\n>\n> On a Desktop running Linux.\n> Using the git.git repo, mounted as nfs on an oldish NAS\n> with spinning disks (and WLAN) gave this timing:\n>\n> git -c checkout.workers=1\n> 2 minutes, some seconds.\n>\n> git -c checkout.workers=4\n> 1 minute, some seconds.\n>\n> git -c checkout.workers=16\n> 40..44 seconds\n>\n> Not a scientific measurement, just a positive feedback from a real-world.\n\nNice! Thanks for testing the parallel version and for your feedback on\nit, I appreciate it.\n\n> Good work, I'm looking forward to have this feature in git.git\n\nThanks :)\n"},{"id":"423582","messageId":"cover.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1619818517.git.matheus.bernardino@usp.br","subject":"[PATCH v3 0/8] Parallel Checkout (part 3)","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:27Z","receivedAt":"2021-05-04T16:27:47Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This is the last part of the parallel checkout series. It adds tests and\nparallel checkout support to `git checkout-index` and\n`git checkout `.\n\nI rebased this version onto `master`, as part-2 was merged down to\n`master`.\n\nChanges since v2:\n\n* Patch 1: Aligned function parameters in make_transient_cache_entry(),\n and renamed the `struct mem_pool *` argument from `mp` to\n `ce_mem_pool`. This is more consistent with the other functions using\n in read-cache.c and with how we call this function in the next patch.\n\n* Patch 2: Removed unnecessary 'Note: ' from commit message.\n\n* Patch 3: Rewrote commit message to better explain why we unified the\n exit paths for `checkout_all()` and `checkout_file()` modes, and\n changed `git checkout-index --all`s error code from 128 to 1.\n\nMatheus Tavares (8):\n make_transient_cache_entry(): optionally alloc from mem_pool\n builtin/checkout.c: complete parallel checkout support\n checkout-index: add parallel checkout support\n parallel-checkout: add tests for basic operations\n parallel-checkout: add tests related to path collisions\n t0028: extract encoding helpers to lib-encoding.sh\n parallel-checkout: add tests related to .gitattributes\n ci: run test round with parallel-checkout enabled\n\n builtin/checkout--worker.c | 2 +-\n builtin/checkout-index.c | 24 ++-\n builtin/checkout.c | 20 ++-\n builtin/difftool.c | 2 +-\n cache.h | 14 +-\n ci/run-build-and-tests.sh | 1 +\n parallel-checkout.c | 18 ++\n read-cache.c | 14 +-\n t/README | 4 +\n t/lib-encoding.sh | 25 +++\n t/lib-parallel-checkout.sh | 45 +++++\n t/t0028-working-tree-encoding.sh | 25 +--\n t/t2080-parallel-checkout-basics.sh | 229 ++++++++++++++++++++++++\n t/t2081-parallel-checkout-collisions.sh | 162 +++++++++++++++++\n t/t2082-parallel-checkout-attributes.sh | 194 ++++++++++++++++++++\n unpack-trees.c | 2 +-\n 16 files changed, 732 insertions(+), 49 deletions(-)\n create mode 100644 t/lib-encoding.sh\n create mode 100644 t/lib-parallel-checkout.sh\n create mode 100755 t/t2080-parallel-checkout-basics.sh\n create mode 100755 t/t2081-parallel-checkout-collisions.sh\n create mode 100755 t/t2082-parallel-checkout-attributes.sh\n\nRange-diff against v2:\n1: f870040bfb ! 1: bf6c7114aa make_transient_cache_entry(): optionally alloc from mem_pool\n @@ cache.h: struct cache_entry *make_empty_cache_entry(struct index_state *istate,\n - * Create a cache_entry that is not intended to be added to an index.\n - * Caller is responsible for discarding the cache_entry\n - * with `discard_cache_entry`.\n -+ * Create a cache_entry that is not intended to be added to an index. If `mp`\n -+ * is not NULL, the entry is allocated within the given memory pool. Caller is\n -+ * responsible for discarding \"loose\" entries with `discard_cache_entry()` and\n -+ * the mem_pool with `mem_pool_discard(mp, should_validate_cache_entries())`.\n ++ * Create a cache_entry that is not intended to be added to an index. If\n ++ * `ce_mem_pool` is not NULL, the entry is allocated within the given memory\n ++ * pool. Caller is responsible for discarding \"loose\" entries with\n ++ * `discard_cache_entry()` and the memory pool with\n ++ * `mem_pool_discard(ce_mem_pool, should_validate_cache_entries())`.\n */\n struct cache_entry *make_transient_cache_entry(unsigned int mode,\n \t\t\t\t\t const struct object_id *oid,\n \t\t\t\t\t const char *path,\n -\t\t\t\t\t int stage);\n -+\t\t\t\t\t int stage, struct mem_pool *mp);\n ++\t\t\t\t\t int stage,\n ++\t\t\t\t\t struct mem_pool *ce_mem_pool);\n \n -struct cache_entry *make_empty_transient_cache_entry(size_t name_len);\n -+struct cache_entry *make_empty_transient_cache_entry(size_t len, struct mem_pool *mp);\n ++struct cache_entry *make_empty_transient_cache_entry(size_t len,\n ++\t\t\t\t\t\t struct mem_pool *ce_mem_pool);\n \n /*\n * Discard cache entry.\n @@ read-cache.c: struct cache_entry *make_empty_cache_entry(struct index_state *ist\n }\n \n -struct cache_entry *make_empty_transient_cache_entry(size_t len)\n -+struct cache_entry *make_empty_transient_cache_entry(size_t len, struct mem_pool *mp)\n ++struct cache_entry *make_empty_transient_cache_entry(size_t len,\n ++\t\t\t\t\t\t struct mem_pool *ce_mem_pool)\n {\n -+\tif (mp)\n -+\t\treturn mem_pool__ce_calloc(mp, len);\n ++\tif (ce_mem_pool)\n ++\t\treturn mem_pool__ce_calloc(ce_mem_pool, len);\n \treturn xcalloc(1, cache_entry_size(len));\n }\n \n @@ read-cache.c: struct cache_entry *make_cache_entry(struct index_state *istate,\n -\t\t\t\t\t const char *path, int stage)\n +struct cache_entry *make_transient_cache_entry(unsigned int mode,\n +\t\t\t\t\t const struct object_id *oid,\n -+\t\t\t\t\t const char *path, int stage,\n -+\t\t\t\t\t struct mem_pool *mp)\n ++\t\t\t\t\t const char *path,\n ++\t\t\t\t\t int stage,\n ++\t\t\t\t\t struct mem_pool *ce_mem_pool)\n {\n \tstruct cache_entry *ce;\n \tint len;\n @@ read-cache.c: struct cache_entry *make_transient_cache_entry(unsigned int mode,\n \n \tlen = strlen(path);\n -\tce = make_empty_transient_cache_entry(len);\n -+\tce = make_empty_transient_cache_entry(len, mp);\n ++\tce = make_empty_transient_cache_entry(len, ce_mem_pool);\n \n \toidcpy(&ce->oid, oid);\n \tmemcpy(ce->name, path, len);\n2: e2d82c4337 ! 2: e898889787 builtin/checkout.c: complete parallel checkout support\n @@ Commit message\n checkout_entry() directly, instead of unpack_trees(). Let's add parallel\n checkout support for this code path too.\n \n - Note: the transient cache entries allocated in checkout_merged() are now\n + The transient cache entries allocated in checkout_merged() are now\n allocated in a mem_pool which is only discarded after parallel checkout\n finishes. This is done because the entries need to be valid when\n run_parallel_checkout() is called.\n @@ builtin/checkout.c: static int checkout_worktree(const struct checkout_opts *opt\n \tenable_delayed_checkout(&state);\n +\tif (pc_workers > 1)\n +\t\tinit_parallel_checkout();\n - \tfor (pos = 0; pos < active_nr; pos++) {\n - \t\tstruct cache_entry *ce = active_cache[pos];\n - \t\tif (ce->ce_flags & CE_MATCHED) {\n + \n + \t/* TODO: audit for interaction with sparse-index. */\n + \tensure_full_index(&the_index);\n @@ builtin/checkout.c: static int checkout_worktree(const struct checkout_opts *opts,\n \t\t\t\t\t\t &nr_checkouts, opts->overlay_mode);\n \t\t\telse if (opts->merge)\n3: 0fe1a5fabc ! 3: 916f391a46 checkout-index: add parallel checkout support\n @@ Metadata\n ## Commit message ##\n checkout-index: add parallel checkout support\n \n - Note: previously, `checkout_all()` would not return on errors, but\n - instead call `exit()` with a non-zero code. However, it only did that\n - after calling `checkout_entry()` for all index entries, thus not\n - stopping on the first error, but attempting to write the maximum number\n - of entries possible. In order to maintain this behavior we now propagate\n - `checkout_all()`s error status to `cmd_checkout_index()`, so that it can\n - call `run_parallel_checkout()` and attempt to write the queued entries\n - before exiting with the error code.\n + Allow checkout-index to use the parallel checkout framework, honoring\n + the checkout.workers configuration.\n +\n + There are two code paths in checkout-index which call\n + `checkout_entry()`, and thus, can make use of parallel checkout:\n + `checkout_file()`, which is used to write paths explicitly given at the\n + command line; and `checkout_all()`, which is used to write all paths in\n + the index, when the `--all` option is given.\n +\n + In both operation modes, checkout-index doesn't abort immediately on a\n + `checkout_entry()` failure. Instead, it tries to check out all remaining\n + paths before exiting with a non-zero exit code. To keep this behavior\n + when parallel checkout is being used, we must allow\n + `run_parallel_checkout()` to try writing the queued entries before we\n + exit, even if we already got an error code from a previous\n + `checkout_entry()` call.\n +\n + However, `checkout_all()` doesn't return on errors, it calls `exit()`\n + with code 128. We could make it call `run_parallel_checkout()` before\n + exiting, but it makes the code easier to follow if we unify the exit\n + path for both checkout-index modes at `cmd_checkout_index()`, and let\n + this function take care of the interactions with the parallel checkout\n + API. So let's do that.\n +\n + With this change, we also have to consider whether we want to keep using\n + 128 as the error code for `git checkout-index --all`, while we use 1 for\n + `git checkout-index ` (even when the actual error is the same).\n + Since there is not much value in having code 128 only for `--all`, and\n + there is no mention about it in the docs (so it's unlikely that changing\n + it will break any existing script), let's make both modes exit with code\n + 1 on `checkout_entry()` errors.\n \n Signed-off-by: Matheus Tavares \n \n4: f604c50dba = 4: 667777053a parallel-checkout: add tests for basic operations\n5: eae6d3a1c1 = 5: dcb3acab1d parallel-checkout: add tests related to path collisions\n6: 9161cd1503 = 6: 6141c46051 t0028: extract encoding helpers to lib-encoding.sh\n7: bc584897e6 = 7: 5350689a30 parallel-checkout: add tests related to .gitattributes\n8: 1bc5c523c5 ! 8: 7b2966c488 ci: run test round with parallel-checkout enabled\n @@ parallel-checkout.c: static const int DEFAULT_NUM_WORKERS = 1;\n \telse if (*num_workers < 1)\n \n ## t/README ##\n -@@ t/README: and \"sha256\".\n - GIT_TEST_WRITE_REV_INDEX=, when true enables the\n - 'pack.writeReverseIndex' setting.\n +@@ t/README: GIT_TEST_WRITE_REV_INDEX=, when true enables the\n + GIT_TEST_SPARSE_INDEX=, when true enables index writes to use the\n + sparse-index format by default.\n \n +GIT_TEST_CHECKOUT_WORKERS= overrides the 'checkout.workers' setting\n +to and 'checkout.thresholdForParallelism' to 0, forcing the\n-- \n2.30.1\n\n"},{"id":"423583","messageId":"bf6c7114aa241fc7a8d6254545b4a37ac21e5bc4.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"[PATCH v3 1/8] make_transient_cache_entry(): optionally alloc from mem_pool","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:28Z","receivedAt":"2021-05-04T16:27:53Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Allow make_transient_cache_entry() to optionally receive a mem_pool\nstruct in which it should allocate the entry. This will be used in the\nfollowing patch, to store some transient entries which should persist\nuntil parallel checkout finishes.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout--worker.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/difftool.c | 2 +-\n cache.h | 14 +++++++++-----\n read-cache.c | 14 ++++++++++----\n unpack-trees.c | 2 +-\n 6 files changed, 23 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c\nindex 31e0de2f7e..289a9b8f89 100644\n--- a/builtin/checkout--worker.c\n+++ b/builtin/checkout--worker.c\n@@ -39,7 +39,7 @@ static void packet_to_pc_item(const char *buffer, int len,\n \t}\n \n \tmemset(pc_item, 0, sizeof(*pc_item));\n-\tpc_item->ce = make_empty_transient_cache_entry(fixed_portion->name_len);\n+\tpc_item->ce = make_empty_transient_cache_entry(fixed_portion->name_len, NULL);\n \tpc_item->ce->ce_namelen = fixed_portion->name_len;\n \tpc_item->ce->ce_mode = fixed_portion->ce_mode;\n \tmemcpy(pc_item->ce->name, variant, pc_item->ce->ce_namelen);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5bd9128d1a..a597f31d53 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -291,7 +291,7 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko\n \tif (write_object_file(result_buf.ptr, result_buf.size, blob_type, &oid))\n \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n \tfree(result_buf.ptr);\n-\tce = make_transient_cache_entry(mode, &oid, path, 2);\n+\tce = make_transient_cache_entry(mode, &oid, path, 2, NULL);\n \tif (!ce)\n \t\tdie(_(\"make_cache_entry failed for path '%s'\"), path);\n \tstatus = checkout_entry(ce, state, NULL, nr_checkouts);\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 0202a43052..89334b77fb 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -323,7 +323,7 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n \tstruct cache_entry *ce;\n \tint ret;\n \n-\tce = make_transient_cache_entry(mode, oid, path, 0);\n+\tce = make_transient_cache_entry(mode, oid, path, 0, NULL);\n \tret = checkout_entry(ce, state, NULL, NULL);\n \n \tdiscard_cache_entry(ce);\ndiff --git a/cache.h b/cache.h\nindex b785ffb383..0e2b952647 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -370,16 +370,20 @@ struct cache_entry *make_empty_cache_entry(struct index_state *istate,\n \t\t\t\t\t size_t name_len);\n \n /*\n- * Create a cache_entry that is not intended to be added to an index.\n- * Caller is responsible for discarding the cache_entry\n- * with `discard_cache_entry`.\n+ * Create a cache_entry that is not intended to be added to an index. If\n+ * `ce_mem_pool` is not NULL, the entry is allocated within the given memory\n+ * pool. Caller is responsible for discarding \"loose\" entries with\n+ * `discard_cache_entry()` and the memory pool with\n+ * `mem_pool_discard(ce_mem_pool, should_validate_cache_entries())`.\n */\n struct cache_entry *make_transient_cache_entry(unsigned int mode,\n \t\t\t\t\t const struct object_id *oid,\n \t\t\t\t\t const char *path,\n-\t\t\t\t\t int stage);\n+\t\t\t\t\t int stage,\n+\t\t\t\t\t struct mem_pool *ce_mem_pool);\n \n-struct cache_entry *make_empty_transient_cache_entry(size_t name_len);\n+struct cache_entry *make_empty_transient_cache_entry(size_t len,\n+\t\t\t\t\t\t struct mem_pool *ce_mem_pool);\n \n /*\n * Discard cache entry.\ndiff --git a/read-cache.c b/read-cache.c\nindex 72a1d339c9..86f95fe62e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -839,8 +839,11 @@ struct cache_entry *make_empty_cache_entry(struct index_state *istate, size_t le\n \treturn mem_pool__ce_calloc(find_mem_pool(istate), len);\n }\n \n-struct cache_entry *make_empty_transient_cache_entry(size_t len)\n+struct cache_entry *make_empty_transient_cache_entry(size_t len,\n+\t\t\t\t\t\t struct mem_pool *ce_mem_pool)\n {\n+\tif (ce_mem_pool)\n+\t\treturn mem_pool__ce_calloc(ce_mem_pool, len);\n \treturn xcalloc(1, cache_entry_size(len));\n }\n \n@@ -874,8 +877,11 @@ struct cache_entry *make_cache_entry(struct index_state *istate,\n \treturn ret;\n }\n \n-struct cache_entry *make_transient_cache_entry(unsigned int mode, const struct object_id *oid,\n-\t\t\t\t\t const char *path, int stage)\n+struct cache_entry *make_transient_cache_entry(unsigned int mode,\n+\t\t\t\t\t const struct object_id *oid,\n+\t\t\t\t\t const char *path,\n+\t\t\t\t\t int stage,\n+\t\t\t\t\t struct mem_pool *ce_mem_pool)\n {\n \tstruct cache_entry *ce;\n \tint len;\n@@ -886,7 +892,7 @@ struct cache_entry *make_transient_cache_entry(unsigned int mode, const struct o\n \t}\n \n \tlen = strlen(path);\n-\tce = make_empty_transient_cache_entry(len);\n+\tce = make_empty_transient_cache_entry(len, ce_mem_pool);\n \n \toidcpy(&ce->oid, oid);\n \tmemcpy(ce->name, path, len);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7a1804c314..f88a69f8e7 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1038,7 +1038,7 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,\n \tsize_t len = traverse_path_len(info, tree_entry_len(n));\n \tstruct cache_entry *ce =\n \t\tis_transient ?\n-\t\tmake_empty_transient_cache_entry(len) :\n+\t\tmake_empty_transient_cache_entry(len, NULL) :\n \t\tmake_empty_cache_entry(istate, len);\n \n \tce->ce_mode = create_ce_mode(n->mode);\n-- \n2.30.1\n\n"},{"id":"423584","messageId":"e8988897876b932b7b0b812addbe3683ad52221f.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"[PATCH v3 2/8] builtin/checkout.c: complete parallel checkout support","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:29Z","receivedAt":"2021-05-04T16:27:54Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Pathspec-limited checkouts (like `git checkout *.txt`) are performed by\na code path that doesn't yet support parallel checkout because it calls\ncheckout_entry() directly, instead of unpack_trees(). Let's add parallel\ncheckout support for this code path too.\n\nThe transient cache entries allocated in checkout_merged() are now\nallocated in a mem_pool which is only discarded after parallel checkout\nfinishes. This is done because the entries need to be valid when\nrun_parallel_checkout() is called.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a597f31d53..2632dd9eff 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -27,6 +27,7 @@\n #include \"wt-status.h\"\n #include \"xdiff-interface.h\"\n #include \"entry.h\"\n+#include \"parallel-checkout.h\"\n \n static const char * const checkout_usage[] = {\n \tN_(\"git checkout [] \"),\n@@ -230,7 +231,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,\n \t\treturn error(_(\"path '%s' does not have their version\"), ce->name);\n }\n \n-static int checkout_merged(int pos, const struct checkout *state, int *nr_checkouts)\n+static int checkout_merged(int pos, const struct checkout *state,\n+\t\t\t int *nr_checkouts, struct mem_pool *ce_mem_pool)\n {\n \tstruct cache_entry *ce = active_cache[pos];\n \tconst char *path = ce->name;\n@@ -291,11 +293,10 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko\n \tif (write_object_file(result_buf.ptr, result_buf.size, blob_type, &oid))\n \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n \tfree(result_buf.ptr);\n-\tce = make_transient_cache_entry(mode, &oid, path, 2, NULL);\n+\tce = make_transient_cache_entry(mode, &oid, path, 2, ce_mem_pool);\n \tif (!ce)\n \t\tdie(_(\"make_cache_entry failed for path '%s'\"), path);\n \tstatus = checkout_entry(ce, state, NULL, nr_checkouts);\n-\tdiscard_cache_entry(ce);\n \treturn status;\n }\n \n@@ -359,16 +360,22 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \tint nr_checkouts = 0, nr_unmerged = 0;\n \tint errs = 0;\n \tint pos;\n+\tint pc_workers, pc_threshold;\n+\tstruct mem_pool ce_mem_pool;\n \n \tstate.force = 1;\n \tstate.refresh_cache = 1;\n \tstate.istate = &the_index;\n \n+\tmem_pool_init(&ce_mem_pool, 0);\n+\tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n \tinit_checkout_metadata(&state.meta, info->refname,\n \t\t\t info->commit ? &info->commit->object.oid : &info->oid,\n \t\t\t NULL);\n \n \tenable_delayed_checkout(&state);\n+\tif (pc_workers > 1)\n+\t\tinit_parallel_checkout();\n \n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(&the_index);\n@@ -387,10 +394,15 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \t\t\t\t\t\t &nr_checkouts, opts->overlay_mode);\n \t\t\telse if (opts->merge)\n \t\t\t\terrs |= checkout_merged(pos, &state,\n-\t\t\t\t\t\t\t&nr_unmerged);\n+\t\t\t\t\t\t\t&nr_unmerged,\n+\t\t\t\t\t\t\t&ce_mem_pool);\n \t\t\tpos = skip_same_name(ce, pos) - 1;\n \t\t}\n \t}\n+\tif (pc_workers > 1)\n+\t\terrs |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n+\t\t\t\t\t NULL, NULL);\n+\tmem_pool_discard(&ce_mem_pool, should_validate_cache_entries());\n \tremove_marked_cache_entries(&the_index, 1);\n \tremove_scheduled_dirs();\n \terrs |= finish_delayed_checkout(&state, &nr_checkouts);\n-- \n2.30.1\n\n"},{"id":"423585","messageId":"916f391a46a1ccedee3fca52893d0339ff37864a.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"[PATCH v3 3/8] checkout-index: add parallel checkout support","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:30Z","receivedAt":"2021-05-04T16:27:55Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Allow checkout-index to use the parallel checkout framework, honoring\nthe checkout.workers configuration.\n\nThere are two code paths in checkout-index which call\n`checkout_entry()`, and thus, can make use of parallel checkout:\n`checkout_file()`, which is used to write paths explicitly given at the\ncommand line; and `checkout_all()`, which is used to write all paths in\nthe index, when the `--all` option is given.\n\nIn both operation modes, checkout-index doesn't abort immediately on a\n`checkout_entry()` failure. Instead, it tries to check out all remaining\npaths before exiting with a non-zero exit code. To keep this behavior\nwhen parallel checkout is being used, we must allow\n`run_parallel_checkout()` to try writing the queued entries before we\nexit, even if we already got an error code from a previous\n`checkout_entry()` call.\n\nHowever, `checkout_all()` doesn't return on errors, it calls `exit()`\nwith code 128. We could make it call `run_parallel_checkout()` before\nexiting, but it makes the code easier to follow if we unify the exit\npath for both checkout-index modes at `cmd_checkout_index()`, and let\nthis function take care of the interactions with the parallel checkout\nAPI. So let's do that.\n\nWith this change, we also have to consider whether we want to keep using\n128 as the error code for `git checkout-index --all`, while we use 1 for\n`git checkout-index ` (even when the actual error is the same).\nSince there is not much value in having code 128 only for `--all`, and\nthere is no mention about it in the docs (so it's unlikely that changing\nit will break any existing script), let's make both modes exit with code\n1 on `checkout_entry()` errors.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/checkout-index.c | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex c9a3c71914..e21620d964 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -12,6 +12,7 @@\n #include \"cache-tree.h\"\n #include \"parse-options.h\"\n #include \"entry.h\"\n+#include \"parallel-checkout.h\"\n \n #define CHECKOUT_ALL 4\n static int nul_term_line;\n@@ -115,7 +116,7 @@ static int checkout_file(const char *name, const char *prefix)\n \treturn -1;\n }\n \n-static void checkout_all(const char *prefix, int prefix_length)\n+static int checkout_all(const char *prefix, int prefix_length)\n {\n \tint i, errs = 0;\n \tstruct cache_entry *last_ce = NULL;\n@@ -144,11 +145,7 @@ static void checkout_all(const char *prefix, int prefix_length)\n \t}\n \tif (last_ce && to_tempfile)\n \t\twrite_tempfile_record(last_ce->name, prefix);\n-\tif (errs)\n-\t\t/* we have already done our error reporting.\n-\t\t * exit with the same code as die().\n-\t\t */\n-\t\texit(128);\n+\treturn !!errs;\n }\n \n static const char * const builtin_checkout_index_usage[] = {\n@@ -184,6 +181,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tint force = 0, quiet = 0, not_new = 0;\n \tint index_opt = 0;\n \tint err = 0;\n+\tint pc_workers, pc_threshold;\n \tstruct option builtin_checkout_index_options[] = {\n \t\tOPT_BOOL('a', \"all\", &all,\n \t\t\tN_(\"check out all files in the index\")),\n@@ -238,6 +236,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n+\tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n+\tif (pc_workers > 1)\n+\t\tinit_parallel_checkout();\n+\n \t/* Check out named files first */\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n@@ -277,12 +279,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \t}\n \n+\tif (all)\n+\t\terr |= checkout_all(prefix, prefix_length);\n+\n+\tif (pc_workers > 1)\n+\t\terr |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n+\t\t\t\t\t NULL, NULL);\n+\n \tif (err)\n \t\treturn 1;\n \n-\tif (all)\n-\t\tcheckout_all(prefix, prefix_length);\n-\n \tif (is_lock_file_locked(&lock_file) &&\n \t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(\"Unable to write new index file\");\n-- \n2.30.1\n\n"},{"id":"423586","messageId":"667777053ab853ef2f94b7ba4ab39f7241d2b49c.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"[PATCH v3 4/8] parallel-checkout: add tests for basic operations","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:31Z","receivedAt":"2021-05-04T16:27:58Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to populate the working tree during clone and checkout using\nsequential and parallel mode, to confirm that they produce identical\nresults. Also test basic checkout mechanics, such as checking for\nsymlinks in the leading directories and the abidance to --force.\n\nNote: some helper functions are added to a common lib file which is only\nincluded by t2080 for now. But they will also be used by other\nparallel-checkout tests in the following patches.\n\nCo-authored-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n t/lib-parallel-checkout.sh | 42 +++++\n t/t2080-parallel-checkout-basics.sh | 229 ++++++++++++++++++++++++++++\n 2 files changed, 271 insertions(+)\n create mode 100644 t/lib-parallel-checkout.sh\n create mode 100755 t/t2080-parallel-checkout-basics.sh\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nnew file mode 100644\nindex 0000000000..f60b22ef34\n--- /dev/null\n+++ b/t/lib-parallel-checkout.sh\n@@ -0,0 +1,42 @@\n+# Helpers for tests invoking parallel-checkout\n+\n+set_checkout_config () {\n+\tif test $# -ne 2\n+\tthen\n+\t\tBUG \"usage: set_checkout_config \"\n+\tfi &&\n+\n+\ttest_config_global checkout.workers $1 &&\n+\ttest_config_global checkout.thresholdForParallelism $2\n+}\n+\n+# Run \"${@:2}\" and check that $1 checkout workers were used\n+test_checkout_workers () {\n+\tif test $# -lt 2\n+\tthen\n+\t\tBUG \"too few arguments to test_checkout_workers\"\n+\tfi &&\n+\n+\tlocal expected_workers=$1 &&\n+\tshift &&\n+\n+\tlocal trace_file=trace-test-checkout-workers &&\n+\trm -f \"$trace_file\" &&\n+\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" &&\n+\n+\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n+\ttest $workers -eq $expected_workers &&\n+\trm \"$trace_file\"\n+}\n+\n+# Verify that both the working tree and the index were created correctly\n+verify_checkout () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tBUG \"usage: verify_checkout \"\n+\tfi &&\n+\n+\tgit -C \"$1\" diff-index --ignore-submodules=none --exit-code HEAD -- &&\n+\tgit -C \"$1\" status --porcelain >\"$1\".status &&\n+\ttest_must_be_empty \"$1\".status\n+}\ndiff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\nnew file mode 100755\nindex 0000000000..7087818550\n--- /dev/null\n+++ b/t/t2080-parallel-checkout-basics.sh\n@@ -0,0 +1,229 @@\n+#!/bin/sh\n+\n+test_description='parallel-checkout basics\n+\n+Ensure that parallel-checkout basically works on clone and checkout, spawning\n+the required number of workers and correctly populating both the index and the\n+working tree.\n+'\n+\n+TEST_NO_CREATE_REPO=1\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+\n+# Test parallel-checkout with a branch switch containing a variety of file\n+# creations, deletions, and modifications, involving different entry types.\n+# The branches B1 and B2 have the following paths:\n+#\n+# B1 B2\n+# a/a (file) a (file)\n+# b (file) b/b (file)\n+#\n+# c/c (file) c (symlink)\n+# d (symlink) d/d (file)\n+#\n+# e/e (file) e (submodule)\n+# f (submodule) f/f (file)\n+#\n+# g (submodule) g (symlink)\n+# h (symlink) h (submodule)\n+#\n+# Additionally, the following paths are present on both branches, but with\n+# different contents:\n+#\n+# i (file) i (file)\n+# j (symlink) j (symlink)\n+# k (submodule) k (submodule)\n+#\n+# And the following paths are only present in one of the branches:\n+#\n+# l/l (file) -\n+# - m/m (file)\n+#\n+test_expect_success 'setup repo for checkout with various types of changes' '\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit checkout -b B2 &&\n+\t\techo B2 >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m file &&\n+\n+\t\tgit checkout -b B1 &&\n+\t\techo B1 >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m file\n+\t) &&\n+\n+\tgit init various &&\n+\t(\n+\t\tcd various &&\n+\n+\t\tgit checkout -b B1 &&\n+\t\tmkdir a c e &&\n+\t\techo a/a >a/a &&\n+\t\techo b >b &&\n+\t\techo c/c >c/c &&\n+\t\ttest_ln_s_add c d &&\n+\t\techo e/e >e/e &&\n+\t\tgit submodule add ../sub f &&\n+\t\tgit submodule add ../sub g &&\n+\t\ttest_ln_s_add c h &&\n+\n+\t\techo \"B1 i\" >i &&\n+\t\ttest_ln_s_add c j &&\n+\t\tgit submodule add -b B1 ../sub k &&\n+\t\tmkdir l &&\n+\t\techo l/l >l/l &&\n+\n+\t\tgit add . &&\n+\t\tgit commit -m B1 &&\n+\n+\t\tgit checkout -b B2 &&\n+\t\tgit rm -rf :^.gitmodules :^k &&\n+\t\tmkdir b d f &&\n+\t\techo a >a &&\n+\t\techo b/b >b/b &&\n+\t\ttest_ln_s_add b c &&\n+\t\techo d/d >d/d &&\n+\t\tgit submodule add ../sub e &&\n+\t\techo f/f >f/f &&\n+\t\ttest_ln_s_add b g &&\n+\t\tgit submodule add ../sub h &&\n+\n+\t\techo \"B2 i\" >i &&\n+\t\ttest_ln_s_add b j &&\n+\t\tgit -C k checkout B2 &&\n+\t\tmkdir m &&\n+\t\techo m/m >m/m &&\n+\n+\t\tgit add . &&\n+\t\tgit commit -m B2 &&\n+\n+\t\tgit checkout --recurse-submodules B1\n+\t)\n+'\n+\n+for mode in sequential parallel sequential-fallback\n+do\n+\tcase $mode in\n+\tsequential) workers=1 threshold=0 expected_workers=0 ;;\n+\tparallel) workers=2 threshold=0 expected_workers=2 ;;\n+\tsequential-fallback) workers=2 threshold=100 expected_workers=0 ;;\n+\tesac\n+\n+\ttest_expect_success \"$mode checkout\" '\n+\t\trepo=various_$mode &&\n+\t\tcp -R various $repo &&\n+\n+\t\t# The just copied files have more recent timestamps than their\n+\t\t# associated index entries. So refresh the cached timestamps\n+\t\t# to avoid an \"entry not up-to-date\" error from `git checkout`.\n+\t\t# We only have to do this for the submodules as `git checkout`\n+\t\t# will already refresh the superproject index before performing\n+\t\t# the up-to-date check.\n+\t\t#\n+\t\tgit -C $repo submodule foreach \"git update-index --refresh\" &&\n+\n+\t\tset_checkout_config $workers $threshold &&\n+\t\ttest_checkout_workers $expected_workers \\\n+\t\t\tgit -C $repo checkout --recurse-submodules B2 &&\n+\t\tverify_checkout $repo\n+\t'\n+done\n+\n+for mode in parallel sequential-fallback\n+do\n+\tcase $mode in\n+\tparallel) workers=2 threshold=0 expected_workers=2 ;;\n+\tsequential-fallback) workers=2 threshold=100 expected_workers=0 ;;\n+\tesac\n+\n+\ttest_expect_success \"$mode checkout on clone\" '\n+\t\trepo=various_${mode}_clone &&\n+\t\tset_checkout_config $workers $threshold &&\n+\t\ttest_checkout_workers $expected_workers \\\n+\t\t\tgit clone --recurse-submodules --branch B2 various $repo &&\n+\t\tverify_checkout $repo\n+\t'\n+done\n+\n+# Just to be paranoid, actually compare the working trees' contents directly.\n+test_expect_success 'compare the working trees' '\n+\trm -rf various_*/.git &&\n+\trm -rf various_*/*/.git &&\n+\n+\t# We use `git diff` instead of `diff -r` because the latter would\n+\t# follow symlinks, and not all `diff` implementations support the\n+\t# `--no-dereference` option.\n+\t#\n+\tgit diff --no-index various_sequential various_parallel &&\n+\tgit diff --no-index various_sequential various_parallel_clone &&\n+\tgit diff --no-index various_sequential various_sequential-fallback &&\n+\tgit diff --no-index various_sequential various_sequential-fallback_clone\n+'\n+\n+# Currently, each submodule is checked out in a separated child process, but\n+# these subprocesses must also be able to use parallel checkout workers to\n+# write the submodules' entries.\n+test_expect_success 'submodules can use parallel checkout' '\n+\tset_checkout_config 2 0 &&\n+\tgit init super &&\n+\t(\n+\t\tcd super &&\n+\t\tgit init sub &&\n+\t\ttest_commit -C sub A &&\n+\t\ttest_commit -C sub B &&\n+\t\tgit submodule add ./sub &&\n+\t\tgit commit -m sub &&\n+\t\trm sub/* &&\n+\t\ttest_checkout_workers 2 git checkout --recurse-submodules .\n+\t)\n+'\n+\n+test_expect_success 'parallel checkout respects --[no]-force' '\n+\tset_checkout_config 2 0 &&\n+\tgit init dirty &&\n+\t(\n+\t\tcd dirty &&\n+\t\tmkdir D &&\n+\t\ttest_commit D/F &&\n+\t\ttest_commit F &&\n+\n+\t\trm -rf D &&\n+\t\techo changed >D &&\n+\t\techo changed >F.t &&\n+\n+\t\t# We expect 0 workers because there is nothing to be done\n+\t\ttest_checkout_workers 0 git checkout HEAD &&\n+\t\ttest_path_is_file D &&\n+\t\tgrep changed D &&\n+\t\tgrep changed F.t &&\n+\n+\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n+\t\ttest_path_is_dir D &&\n+\t\tgrep D/F D/F.t &&\n+\t\tgrep F F.t\n+\t)\n+'\n+\n+test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading dirs' '\n+\tset_checkout_config 2 0 &&\n+\tgit init symlinks &&\n+\t(\n+\t\tcd symlinks &&\n+\t\tmkdir D untracked &&\n+\t\t# Commit 2 files to have enough work for 2 parallel workers\n+\t\ttest_commit D/A &&\n+\t\ttest_commit D/B &&\n+\t\trm -rf D &&\n+\t\tln -s untracked D &&\n+\n+\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n+\t\t! test -h D &&\n+\t\tgrep D/A D/A.t &&\n+\t\tgrep D/B D/B.t\n+\t)\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"423587","messageId":"dcb3acab1d6ed8304da32b10a1eaa9c3f6f81d3a.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"[PATCH v3 5/8] parallel-checkout: add tests related to path collisions","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:32Z","receivedAt":"2021-05-04T16:28:01Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to confirm that path collisions are properly detected by\ncheckout workers, both to avoid race conditions and to report colliding\nentries on clone.\n\nCo-authored-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n parallel-checkout.c | 4 +\n t/lib-parallel-checkout.sh | 4 +-\n t/t2081-parallel-checkout-collisions.sh | 162 ++++++++++++++++++++++++\n 3 files changed, 168 insertions(+), 2 deletions(-)\n create mode 100755 t/t2081-parallel-checkout-collisions.sh\n\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 09e8b10a35..6fb3f1e6c9 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -8,6 +8,7 @@\n #include \"sigchain.h\"\n #include \"streaming.h\"\n #include \"thread-utils.h\"\n+#include \"trace2.h\"\n \n struct pc_worker {\n \tstruct child_process cp;\n@@ -326,6 +327,7 @@ void write_pc_item(struct parallel_checkout_item *pc_item,\n \tif (dir_sep && !has_dirs_only_path(path.buf, dir_sep - path.buf,\n \t\t\t\t\t state->base_dir_len)) {\n \t\tpc_item->status = PC_ITEM_COLLIDED;\n+\t\ttrace2_data_string(\"pcheckout\", NULL, \"collision/dirname\", path.buf);\n \t\tgoto out;\n \t}\n \n@@ -341,6 +343,8 @@ void write_pc_item(struct parallel_checkout_item *pc_item,\n \t\t\t * call should have already caught these cases.\n \t\t\t */\n \t\t\tpc_item->status = PC_ITEM_COLLIDED;\n+\t\t\ttrace2_data_string(\"pcheckout\", NULL,\n+\t\t\t\t\t \"collision/basename\", path.buf);\n \t\t} else {\n \t\t\terror_errno(\"failed to open file '%s'\", path.buf);\n \t\t\tpc_item->status = PC_ITEM_FAILED;\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex f60b22ef34..d6740425b1 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -22,12 +22,12 @@ test_checkout_workers () {\n \n \tlocal trace_file=trace-test-checkout-workers &&\n \trm -f \"$trace_file\" &&\n-\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" &&\n+\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n \n \tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n \ttest $workers -eq $expected_workers &&\n \trm \"$trace_file\"\n-}\n+} 8>&2 2>&4\n \n # Verify that both the working tree and the index were created correctly\n verify_checkout () {\ndiff --git a/t/t2081-parallel-checkout-collisions.sh b/t/t2081-parallel-checkout-collisions.sh\nnew file mode 100755\nindex 0000000000..f6fcfc0c1e\n--- /dev/null\n+++ b/t/t2081-parallel-checkout-collisions.sh\n@@ -0,0 +1,162 @@\n+#!/bin/sh\n+\n+test_description=\"path collisions during parallel checkout\n+\n+Parallel checkout must detect path collisions to:\n+\n+1) Avoid racily writing to different paths that represent the same file on disk.\n+2) Report the colliding entries on clone.\n+\n+The tests in this file exercise parallel checkout's collision detection code in\n+both these mechanics.\n+\"\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+\n+TEST_ROOT=\"$PWD\"\n+\n+test_expect_success CASE_INSENSITIVE_FS 'setup' '\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t100644 $empty_oid\tFILE_X\n+\t100644 $empty_oid\tFILE_x\n+\t100644 $empty_oid\tfile_X\n+\t100644 $empty_oid\tfile_x\n+\tEOF\n+\tgit update-index --index-info >filter.log\n+\tEOF\n+'\n+\n+test_workers_in_event_trace ()\n+{\n+\ttest $1 -eq $(grep \".event.:.child_start..*checkout--worker\" $2 | wc -l)\n+}\n+\n+test_expect_success CASE_INSENSITIVE_FS 'worker detects basename collision' '\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git \\\n+\t\t-c checkout.workers=2 -c checkout.thresholdForParallelism=0 \\\n+\t\tcheckout . &&\n+\n+\ttest_workers_in_event_trace 2 trace &&\n+\tcollisions=$(grep -i \"category.:.pcheckout.,.key.:.collision/basename.,.value.:.file_x.}\" trace | wc -l) &&\n+\ttest $collisions -eq 3\n+'\n+\n+test_expect_success CASE_INSENSITIVE_FS 'worker detects dirname collision' '\n+\ttest_config filter.logger.smudge \"\\\"$TEST_ROOT/logger_script\\\" %f\" &&\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t100644 $empty_oid\tA/B\n+\t100644 $empty_oid\tA/C\n+\t100644 $empty_oid\ta\n+\t100644 $attr_oid\t.gitattributes\n+\tEOF\n+\tgit rm -rf . &&\n+\tgit update-index --index-info expected.log &&\n+\ttest_cmp filter.log expected.log &&\n+\n+\t# Check that it used the right number of workers and detected the collisions\n+\ttest_workers_in_event_trace 2 trace &&\n+\tgrep \"category.:.pcheckout.,.key.:.collision/dirname.,.value.:.A/B.}\" trace &&\n+\tgrep \"category.:.pcheckout.,.key.:.collision/dirname.,.value.:.A/C.}\" trace\n+'\n+\n+test_expect_success SYMLINKS,CASE_INSENSITIVE_FS 'do not follow symlinks colliding with leading dir' '\n+\tempty_oid=$(git hash-object -w --stdin objs <<-EOF &&\n+\t120000 $symlink_oid\tD\n+\t100644 $empty_oid\td/x\n+\t100644 $empty_oid\te/y\n+\tEOF\n+\tgit rm -rf . &&\n+\tgit update-index --index-info stderr &&\n+\n+\tgrep FILE_X stderr &&\n+\tgrep FILE_x stderr &&\n+\tgrep file_X stderr &&\n+\tgrep file_x stderr &&\n+\tgrep \"the following paths have collided\" stderr\n+'\n+\n+# This test ensures that the collision report code is correctly looking for\n+# colliding peers in the second half of the cache_entry array. This is done by\n+# defining a smudge command for the *last* array entry, which makes it\n+# non-eligible for parallel-checkout. Thus, it is checked out *first*, before\n+# spawning the workers.\n+#\n+# Note: this test doesn't work on Windows because, on this system, the\n+# collision report code uses strcmp() to find the colliding pairs when\n+# core.ignoreCase is false. And we need this setting for this test so that only\n+# 'file_x' matches the pattern of the filter attribute. But the test works on\n+# OSX, where the colliding pairs are found using inode.\n+#\n+test_expect_success CASE_INSENSITIVE_FS,!MINGW,!CYGWIN \\\n+\t'collision report on clone (w/ colliding peer after the detected entry)' '\n+\n+\ttest_config_global filter.logger.smudge \"\\\"$TEST_ROOT/logger_script\\\" %f\" &&\n+\tgit reset --hard basename_collision &&\n+\techo \"file_x filter=logger\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\tgit commit -m \"filter for file_x\" &&\n+\n+\trm -rf clone-repo &&\n+\tset_checkout_config 2 0 &&\n+\ttest_checkout_workers 2 \\\n+\t\tgit -c core.ignoreCase=false clone . clone-repo 2>stderr &&\n+\n+\tgrep FILE_X stderr &&\n+\tgrep FILE_x stderr &&\n+\tgrep file_X stderr &&\n+\tgrep file_x stderr &&\n+\tgrep \"the following paths have collided\" stderr &&\n+\n+\t# Check that only \"file_x\" was filtered\n+\techo file_x >expected.log &&\n+\ttest_cmp clone-repo/filter.log expected.log\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"423588","messageId":"6141c460517b3fd15e8052730dd180df7e445e12.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"[PATCH v3 6/8] t0028: extract encoding helpers to lib-encoding.sh","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:33Z","receivedAt":"2021-05-04T16:28:07Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The following patch will add tests outside t0028 which will also need to\nre-encode some strings. Extract the auxiliary encoding functions from\nt0028 to a common lib file so that they can be reused.\n\nSigned-off-by: Matheus Tavares \n---\n t/lib-encoding.sh | 25 +++++++++++++++++++++++++\n t/t0028-working-tree-encoding.sh | 25 +------------------------\n 2 files changed, 26 insertions(+), 24 deletions(-)\n create mode 100644 t/lib-encoding.sh\n\ndiff --git a/t/lib-encoding.sh b/t/lib-encoding.sh\nnew file mode 100644\nindex 0000000000..2dabc8c73e\n--- /dev/null\n+++ b/t/lib-encoding.sh\n@@ -0,0 +1,25 @@\n+# Encoding helpers\n+\n+test_lazy_prereq NO_UTF16_BOM '\n+\ttest $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6\n+'\n+\n+test_lazy_prereq NO_UTF32_BOM '\n+\ttest $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12\n+'\n+\n+write_utf16 () {\n+\tif test_have_prereq NO_UTF16_BOM\n+\tthen\n+\t\tprintf '\\376\\377'\n+\tfi &&\n+\ticonv -f UTF-8 -t UTF-16\n+}\n+\n+write_utf32 () {\n+\tif test_have_prereq NO_UTF32_BOM\n+\tthen\n+\t\tprintf '\\0\\0\\376\\377'\n+\tfi &&\n+\ticonv -f UTF-8 -t UTF-32\n+}\ndiff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\nindex f970a9806b..82905a2156 100755\n--- a/t/t0028-working-tree-encoding.sh\n+++ b/t/t0028-working-tree-encoding.sh\n@@ -6,33 +6,10 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-encoding.sh\"\n \n GIT_TRACE_WORKING_TREE_ENCODING=1 && export GIT_TRACE_WORKING_TREE_ENCODING\n \n-test_lazy_prereq NO_UTF16_BOM '\n-\ttest $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6\n-'\n-\n-test_lazy_prereq NO_UTF32_BOM '\n-\ttest $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12\n-'\n-\n-write_utf16 () {\n-\tif test_have_prereq NO_UTF16_BOM\n-\tthen\n-\t\tprintf '\\376\\377'\n-\tfi &&\n-\ticonv -f UTF-8 -t UTF-16\n-}\n-\n-write_utf32 () {\n-\tif test_have_prereq NO_UTF32_BOM\n-\tthen\n-\t\tprintf '\\0\\0\\376\\377'\n-\tfi &&\n-\ticonv -f UTF-8 -t UTF-32\n-}\n-\n test_expect_success 'setup test files' '\n \tgit config core.eol lf &&\n \n-- \n2.30.1\n\n"},{"id":"423589","messageId":"5350689a30566f067a10a288e7cdef63119728d1.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"[PATCH v3 7/8] parallel-checkout: add tests related to .gitattributes","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:34Z","receivedAt":"2021-05-04T16:28:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add tests to confirm that the `struct conv_attrs` data is correctly\npassed from the main process to the workers, and that they can properly\nconvert the blobs before writing them to the working tree.\n\nAlso check that parallel-ineligible entries, such as regular files that\nrequire external filters, are correctly smudge and written when\nparallel-checkout is enabled.\n\nCo-authored-by: Jeff Hostetler \nSigned-off-by: Matheus Tavares \n---\n t/t2082-parallel-checkout-attributes.sh | 194 ++++++++++++++++++++++++\n 1 file changed, 194 insertions(+)\n create mode 100755 t/t2082-parallel-checkout-attributes.sh\n\ndiff --git a/t/t2082-parallel-checkout-attributes.sh b/t/t2082-parallel-checkout-attributes.sh\nnew file mode 100755\nindex 0000000000..2525457961\n--- /dev/null\n+++ b/t/t2082-parallel-checkout-attributes.sh\n@@ -0,0 +1,194 @@\n+#!/bin/sh\n+\n+test_description='parallel-checkout: attributes\n+\n+Verify that parallel-checkout correctly creates files that require\n+conversions, as specified in .gitattributes. The main point here is\n+to check that the conv_attr data is correctly sent to the workers\n+and that it contains sufficient information to smudge files\n+properly (without access to the index or attribute stack).\n+'\n+\n+TEST_NO_CREATE_REPO=1\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n+. \"$TEST_DIRECTORY/lib-encoding.sh\"\n+\n+test_expect_success 'parallel-checkout with ident' '\n+\tset_checkout_config 2 0 &&\n+\tgit init ident &&\n+\t(\n+\t\tcd ident &&\n+\t\techo \"A ident\" >.gitattributes &&\n+\t\techo \"\\$Id\\$\" >A &&\n+\t\techo \"\\$Id\\$\" >B &&\n+\t\tgit add -A &&\n+\t\tgit commit -m id &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git reset --hard &&\n+\t\thexsz=$(test_oid hexsz) &&\n+\t\tgrep -E \"\\\\\\$Id: [0-9a-f]{$hexsz} \\\\\\$\" A &&\n+\t\tgrep \"\\\\\\$Id\\\\\\$\" B\n+\t)\n+'\n+\n+test_expect_success 'parallel-checkout with re-encoding' '\n+\tset_checkout_config 2 0 &&\n+\tgit init encoding &&\n+\t(\n+\t\tcd encoding &&\n+\t\techo text >utf8-text &&\n+\t\twrite_utf16 utf16-text &&\n+\n+\t\techo \"A working-tree-encoding=UTF-16\" >.gitattributes &&\n+\t\tcp utf16-text A &&\n+\t\tcp utf8-text B &&\n+\t\tgit add A B .gitattributes &&\n+\t\tgit commit -m encoding &&\n+\n+\t\t# Check that A is stored in UTF-8\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp_bin utf8-text A.internal &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git checkout A B &&\n+\n+\t\t# Check that A (and only A) is re-encoded during checkout\n+\t\ttest_cmp_bin utf16-text A &&\n+\t\ttest_cmp_bin utf8-text B\n+\t)\n+'\n+\n+test_expect_success 'parallel-checkout with eol conversions' '\n+\tset_checkout_config 2 0 &&\n+\tgit init eol &&\n+\t(\n+\t\tcd eol &&\n+\t\tprintf \"multi\\r\\nline\\r\\ntext\" >crlf-text &&\n+\t\tprintf \"multi\\nline\\ntext\" >lf-text &&\n+\n+\t\tgit config core.autocrlf false &&\n+\t\techo \"A eol=crlf\" >.gitattributes &&\n+\t\tcp crlf-text A &&\n+\t\tcp lf-text B &&\n+\t\tgit add A B .gitattributes &&\n+\t\tgit commit -m eol &&\n+\n+\t\t# Check that A is stored with LF format\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp_bin lf-text A.internal &&\n+\n+\t\trm A B &&\n+\t\ttest_checkout_workers 2 git checkout A B &&\n+\n+\t\t# Check that A (and only A) is converted to CRLF during checkout\n+\t\ttest_cmp_bin crlf-text A &&\n+\t\ttest_cmp_bin lf-text B\n+\t)\n+'\n+\n+# Entries that require an external filter are not eligible for parallel\n+# checkout. Check that both the parallel-eligible and non-eligible entries are\n+# properly writen in a single checkout operation.\n+#\n+test_expect_success 'parallel-checkout and external filter' '\n+\tset_checkout_config 2 0 &&\n+\tgit init filter &&\n+\t(\n+\t\tcd filter &&\n+\t\twrite_script <<-\\EOF rot13.sh &&\n+\t\ttr \\\n+\t\t \"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ\" \\\n+\t\t \"nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM\"\n+\t\tEOF\n+\n+\t\tgit config filter.rot13.clean \"\\\"$(pwd)/rot13.sh\\\"\" &&\n+\t\tgit config filter.rot13.smudge \"\\\"$(pwd)/rot13.sh\\\"\" &&\n+\t\tgit config filter.rot13.required true &&\n+\n+\t\techo abcd >original &&\n+\t\techo nopq >rot13 &&\n+\n+\t\techo \"A filter=rot13\" >.gitattributes &&\n+\t\tcp original A &&\n+\t\tcp original B &&\n+\t\tcp original C &&\n+\t\tgit add A B C .gitattributes &&\n+\t\tgit commit -m filter &&\n+\n+\t\t# Check that A (and only A) was cleaned\n+\t\tgit cat-file -p :A >A.internal &&\n+\t\ttest_cmp rot13 A.internal &&\n+\t\tgit cat-file -p :B >B.internal &&\n+\t\ttest_cmp original B.internal &&\n+\t\tgit cat-file -p :C >C.internal &&\n+\t\ttest_cmp original C.internal &&\n+\n+\t\trm A B C *.internal &&\n+\t\ttest_checkout_workers 2 git checkout A B C &&\n+\n+\t\t# Check that A (and only A) was smudged during checkout\n+\t\ttest_cmp original A &&\n+\t\ttest_cmp original B &&\n+\t\ttest_cmp original C\n+\t)\n+'\n+\n+# The delayed queue is independent from the parallel queue, and they should be\n+# able to work together in the same checkout process.\n+#\n+test_expect_success PERL 'parallel-checkout and delayed checkout' '\n+\twrite_script rot13-filter.pl \"$PERL_PATH\" \\\n+\t\t<\"$TEST_DIRECTORY\"/t0021/rot13-filter.pl &&\n+\n+\ttest_config_global filter.delay.process \\\n+\t\t\"\\\"$(pwd)/rot13-filter.pl\\\" --always-delay \\\"$(pwd)/delayed.log\\\" clean smudge delay\" &&\n+\ttest_config_global filter.delay.required true &&\n+\n+\techo \"abcd\" >original &&\n+\techo \"nopq\" >rot13 &&\n+\n+\tgit init delayed &&\n+\t(\n+\t\tcd delayed &&\n+\t\techo \"*.d filter=delay\" >.gitattributes &&\n+\t\tcp ../original W.d &&\n+\t\tcp ../original X.d &&\n+\t\tcp ../original Y &&\n+\t\tcp ../original Z &&\n+\t\tgit add -A &&\n+\t\tgit commit -m delayed &&\n+\n+\t\t# Check that *.d files were cleaned\n+\t\tgit cat-file -p :W.d >W.d.internal &&\n+\t\ttest_cmp W.d.internal ../rot13 &&\n+\t\tgit cat-file -p :X.d >X.d.internal &&\n+\t\ttest_cmp X.d.internal ../rot13 &&\n+\t\tgit cat-file -p :Y >Y.internal &&\n+\t\ttest_cmp Y.internal ../original &&\n+\t\tgit cat-file -p :Z >Z.internal &&\n+\t\ttest_cmp Z.internal ../original &&\n+\n+\t\trm *\n+\t) &&\n+\n+\tset_checkout_config 2 0 &&\n+\ttest_checkout_workers 2 git -C delayed checkout -f &&\n+\tverify_checkout delayed &&\n+\n+\t# Check that the *.d files got to the delay queue and were filtered\n+\tgrep \"smudge W.d .* \\[DELAYED\\]\" delayed.log &&\n+\tgrep \"smudge X.d .* \\[DELAYED\\]\" delayed.log &&\n+\ttest_cmp delayed/W.d original &&\n+\ttest_cmp delayed/X.d original &&\n+\n+\t# Check that the parallel-eligible entries went to the right queue and\n+\t# were not filtered\n+\t! grep \"smudge Y .* \\[DELAYED\\]\" delayed.log &&\n+\t! grep \"smudge Z .* \\[DELAYED\\]\" delayed.log &&\n+\ttest_cmp delayed/Y original &&\n+\ttest_cmp delayed/Z original\n+'\n+\n+test_done\n-- \n2.30.1\n\n"},{"id":"423590","messageId":"7b2966c488c29578aa621c609e3d6fc7e36534a9.1620145501.git.matheus.bernardino@usp.br","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"[PATCH v3 8/8] ci: run test round with parallel-checkout enabled","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T16:27:35Z","receivedAt":"2021-05-04T16:28:12Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"We already have tests for the basic parallel-checkout operations. But\nthis code can also run be executed by other commands, such as\ngit-read-tree and git-sparse-checkout, which are currently not tested\nwith multiple workers. To promote a wider test coverage without\nduplicating tests:\n\n1. Add the GIT_TEST_CHECKOUT_WORKERS environment variable, to optionally\n force parallel-checkout execution during the whole test suite.\n\n2. Set this variable (with a value of 2) in the second test round of our\n linux-gcc CI job. This round runs `make test` again with some\n optional GIT_TEST_* variables enabled, so there is no additional\n overhead in exercising the parallel-checkout code here.\n\nNote that tests checking out less than two parallel-eligible entries\nwill fall back to the sequential mode. Nevertheless, it's still a good\nexercise for the parallel-checkout framework as the fallback codepath\nalso writes the queued entries using the parallel-checkout functions\n(only without spawning any worker).\n\nSigned-off-by: Matheus Tavares \n---\n ci/run-build-and-tests.sh | 1 +\n parallel-checkout.c | 14 ++++++++++++++\n t/README | 4 ++++\n t/lib-parallel-checkout.sh | 3 +++\n 4 files changed, 22 insertions(+)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex d19be40544..3ce81ffee9 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -26,6 +26,7 @@ linux-gcc)\n \texport GIT_TEST_ADD_I_USE_BUILTIN=1\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n \texport GIT_TEST_WRITE_REV_INDEX=1\n+\texport GIT_TEST_CHECKOUT_WORKERS=2\n \tmake test\n \t;;\n linux-clang)\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 6fb3f1e6c9..6b1af32bb3 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -35,6 +35,20 @@ static const int DEFAULT_NUM_WORKERS = 1;\n \n void get_parallel_checkout_configs(int *num_workers, int *threshold)\n {\n+\tchar *env_workers = getenv(\"GIT_TEST_CHECKOUT_WORKERS\");\n+\n+\tif (env_workers && *env_workers) {\n+\t\tif (strtol_i(env_workers, 10, num_workers)) {\n+\t\t\tdie(\"invalid value for GIT_TEST_CHECKOUT_WORKERS: '%s'\",\n+\t\t\t env_workers);\n+\t\t}\n+\t\tif (*num_workers < 1)\n+\t\t\t*num_workers = online_cpus();\n+\n+\t\t*threshold = 0;\n+\t\treturn;\n+\t}\n+\n \tif (git_config_get_int(\"checkout.workers\", num_workers))\n \t\t*num_workers = DEFAULT_NUM_WORKERS;\n \telse if (*num_workers < 1)\ndiff --git a/t/README b/t/README\nindex 8eb9e46b1d..a8cfd37387 100644\n--- a/t/README\n+++ b/t/README\n@@ -439,6 +439,10 @@ GIT_TEST_WRITE_REV_INDEX=, when true enables the\n GIT_TEST_SPARSE_INDEX=, when true enables index writes to use the\n sparse-index format by default.\n \n+GIT_TEST_CHECKOUT_WORKERS= overrides the 'checkout.workers' setting\n+to and 'checkout.thresholdForParallelism' to 0, forcing the\n+execution of the parallel-checkout code.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex d6740425b1..21f5759732 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -1,5 +1,8 @@\n # Helpers for tests invoking parallel-checkout\n \n+# Parallel checkout tests need full control of the number of workers\n+unset GIT_TEST_CHECKOUT_WORKERS\n+\n set_checkout_config () {\n \tif test $# -ne 2\n \tthen\n-- \n2.30.1\n\n"},{"id":"423679","messageId":"67880d76-c1ed-eadd-d33e-25fba0a8473f@gmail.com","threadId":"55540","inReplyTo":"e8988897876b932b7b0b812addbe3683ad52221f.1620145501.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v3 2/8] builtin/checkout.c: complete parallel checkout support","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-05T13:55:16Z","receivedAt":"2021-05-05T13:55:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2021 12:27 PM, Matheus Tavares wrote:> @@ -359,16 +360,22 @@ static int checkout_worktree(const struct checkout_opts *opts,\n> \tint nr_checkouts = 0, nr_unmerged = 0;\n> \tint errs = 0;\n> \tint pos;\n> +\tint pc_workers, pc_threshold;\n> +\tstruct mem_pool ce_mem_pool;\n> \n> \tstate.force = 1;\n> \tstate.refresh_cache = 1;\n> \tstate.istate = &the_index;\n> \n> +\tmem_pool_init(&ce_mem_pool, 0);\n> +\tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n> \tinit_checkout_metadata(&state.meta, info->refname,\n> \t\t\t info->commit ? &info->commit->object.oid : &info->oid,\n> \t\t\t NULL);\n> \n> \tenable_delayed_checkout(&state);\n> +\tif (pc_workers > 1)\n> +\t\tinit_parallel_checkout();\n> \n> \t/* TODO: audit for interaction with sparse-index. */\n> \tensure_full_index(&the_index);\n\nSince this context piece is new as of your rebase, I did a quick check on\nall of the calls you inserted above and found them to be safe with the\nsparse-index. They do not care about the number of cache entries, for\nexample.\n\nThanks,\n-Stolee\n"},{"id":"423681","messageId":"273c4f16-60b6-5635-b95a-17192cf4d5de@gmail.com","threadId":"55540","inReplyTo":"cover.1620145501.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v3 0/8] Parallel Checkout (part 3)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-05T13:57:08Z","receivedAt":"2021-05-05T13:57:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2021 12:27 PM, Matheus Tavares wrote:\n> This is the last part of the parallel checkout series. It adds tests and\n> parallel checkout support to `git checkout-index` and\n> `git checkout `.\n> \n> I rebased this version onto `master`, as part-2 was merged down to\n> `master`.\n\nI read the range-diff and gave the patches another pass. This version\nlooks good to me.\n\nThanks,\n-Stolee\n"},{"id":"423724","messageId":"xmqqa6p83dak.fsf@gitster.g","threadId":"55540","inReplyTo":"273c4f16-60b6-5635-b95a-17192cf4d5de@gmail.com","subject":"Re: [PATCH v3 0/8] Parallel Checkout (part 3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T00:40:19Z","receivedAt":"2021-05-06T00:40:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 5/4/2021 12:27 PM, Matheus Tavares wrote:\n>> This is the last part of the parallel checkout series. It adds tests and\n>> parallel checkout support to `git checkout-index` and\n>> `git checkout `.\n>> \n>> I rebased this version onto `master`, as part-2 was merged down to\n>> `master`.\n>\n> I read the range-diff and gave the patches another pass. This version\n> looks good to me.\n\nThanks. Your assessment matches mine.\n"},{"id":"425581","messageId":"87eedtmj7w.fsf@evledraar.gmail.com","threadId":"55540","inReplyTo":"667777053ab853ef2f94b7ba4ab39f7241d2b49c.1620145501.git.matheus.bernardino@usp.br","subject":"AIX failures on parallel checkout (new in v2.32.0-rc*)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-26T18:36:43Z","receivedAt":"2021-05-26T18:40:58Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 04 2021, Matheus Tavares wrote:\n\n> Add tests to populate the working tree during clone and checkout using\n> sequential and parallel mode, to confirm that they produce identical\n> results. Also test basic checkout mechanics, such as checking for\n> symlinks in the leading directories and the abidance to --force.\n>\n> Note: some helper functions are added to a common lib file which is only\n> included by t2080 for now. But they will also be used by other\n> parallel-checkout tests in the following patches.\n>\n> Co-authored-by: Jeff Hostetler \n> Signed-off-by: Matheus Tavares \n> ---\n> t/lib-parallel-checkout.sh | 42 +++++\n> t/t2080-parallel-checkout-basics.sh | 229 ++++++++++++++++++++++++++++\n> 2 files changed, 271 insertions(+)\n> create mode 100644 t/lib-parallel-checkout.sh\n> create mode 100755 t/t2080-parallel-checkout-basics.sh\n>\n> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> new file mode 100644\n> index 0000000000..f60b22ef34\n> --- /dev/null\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -0,0 +1,42 @@\n> +# Helpers for tests invoking parallel-checkout\n> +\n> +set_checkout_config () {\n> +\tif test $# -ne 2\n> +\tthen\n> +\t\tBUG \"usage: set_checkout_config \"\n> +\tfi &&\n> +\n> +\ttest_config_global checkout.workers $1 &&\n> +\ttest_config_global checkout.thresholdForParallelism $2\n> +}\n> +\n> +# Run \"${@:2}\" and check that $1 checkout workers were used\n> +test_checkout_workers () {\n> +\tif test $# -lt 2\n> +\tthen\n> +\t\tBUG \"too few arguments to test_checkout_workers\"\n> +\tfi &&\n> +\n> +\tlocal expected_workers=$1 &&\n> +\tshift &&\n> +\n> +\tlocal trace_file=trace-test-checkout-workers &&\n> +\trm -f \"$trace_file\" &&\n> +\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" &&\n> +\n> +\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n> +\ttest $workers -eq $expected_workers &&\n> +\trm \"$trace_file\"\n> +}\n> +\n> +# Verify that both the working tree and the index were created correctly\n> +verify_checkout () {\n> +\tif test $# -ne 1\n> +\tthen\n> +\t\tBUG \"usage: verify_checkout \"\n> +\tfi &&\n> +\n> +\tgit -C \"$1\" diff-index --ignore-submodules=none --exit-code HEAD -- &&\n> +\tgit -C \"$1\" status --porcelain >\"$1\".status &&\n> +\ttest_must_be_empty \"$1\".status\n> +}\n> diff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\n> new file mode 100755\n> index 0000000000..7087818550\n> --- /dev/null\n> +++ b/t/t2080-parallel-checkout-basics.sh\n> @@ -0,0 +1,229 @@\n> +#!/bin/sh\n> +\n> +test_description='parallel-checkout basics\n> +\n> +Ensure that parallel-checkout basically works on clone and checkout, spawning\n> +the required number of workers and correctly populating both the index and the\n> +working tree.\n> +'\n> +\n> +TEST_NO_CREATE_REPO=1\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY/lib-parallel-checkout.sh\"\n> +\n> +# Test parallel-checkout with a branch switch containing a variety of file\n> +# creations, deletions, and modifications, involving different entry types.\n> +# The branches B1 and B2 have the following paths:\n> +#\n> +# B1 B2\n> +# a/a (file) a (file)\n> +# b (file) b/b (file)\n> +#\n> +# c/c (file) c (symlink)\n> +# d (symlink) d/d (file)\n> +#\n> +# e/e (file) e (submodule)\n> +# f (submodule) f/f (file)\n> +#\n> +# g (submodule) g (symlink)\n> +# h (symlink) h (submodule)\n> +#\n> +# Additionally, the following paths are present on both branches, but with\n> +# different contents:\n> +#\n> +# i (file) i (file)\n> +# j (symlink) j (symlink)\n> +# k (submodule) k (submodule)\n> +#\n> +# And the following paths are only present in one of the branches:\n> +#\n> +# l/l (file) -\n> +# - m/m (file)\n> +#\n> +test_expect_success 'setup repo for checkout with various types of changes' '\n> +\tgit init sub &&\n> +\t(\n> +\t\tcd sub &&\n> +\t\tgit checkout -b B2 &&\n> +\t\techo B2 >file &&\n> +\t\tgit add file &&\n> +\t\tgit commit -m file &&\n> +\n> +\t\tgit checkout -b B1 &&\n> +\t\techo B1 >file &&\n> +\t\tgit add file &&\n> +\t\tgit commit -m file\n> +\t) &&\n> +\n> +\tgit init various &&\n> +\t(\n> +\t\tcd various &&\n> +\n> +\t\tgit checkout -b B1 &&\n> +\t\tmkdir a c e &&\n> +\t\techo a/a >a/a &&\n> +\t\techo b >b &&\n> +\t\techo c/c >c/c &&\n> +\t\ttest_ln_s_add c d &&\n> +\t\techo e/e >e/e &&\n> +\t\tgit submodule add ../sub f &&\n> +\t\tgit submodule add ../sub g &&\n> +\t\ttest_ln_s_add c h &&\n> +\n> +\t\techo \"B1 i\" >i &&\n> +\t\ttest_ln_s_add c j &&\n> +\t\tgit submodule add -b B1 ../sub k &&\n> +\t\tmkdir l &&\n> +\t\techo l/l >l/l &&\n> +\n> +\t\tgit add . &&\n> +\t\tgit commit -m B1 &&\n> +\n> +\t\tgit checkout -b B2 &&\n> +\t\tgit rm -rf :^.gitmodules :^k &&\n> +\t\tmkdir b d f &&\n> +\t\techo a >a &&\n> +\t\techo b/b >b/b &&\n> +\t\ttest_ln_s_add b c &&\n> +\t\techo d/d >d/d &&\n> +\t\tgit submodule add ../sub e &&\n> +\t\techo f/f >f/f &&\n> +\t\ttest_ln_s_add b g &&\n> +\t\tgit submodule add ../sub h &&\n> +\n> +\t\techo \"B2 i\" >i &&\n> +\t\ttest_ln_s_add b j &&\n> +\t\tgit -C k checkout B2 &&\n> +\t\tmkdir m &&\n> +\t\techo m/m >m/m &&\n> +\n> +\t\tgit add . &&\n> +\t\tgit commit -m B2 &&\n> +\n> +\t\tgit checkout --recurse-submodules B1\n> +\t)\n> +'\n> +\n> +for mode in sequential parallel sequential-fallback\n> +do\n> +\tcase $mode in\n> +\tsequential) workers=1 threshold=0 expected_workers=0 ;;\n> +\tparallel) workers=2 threshold=0 expected_workers=2 ;;\n> +\tsequential-fallback) workers=2 threshold=100 expected_workers=0 ;;\n> +\tesac\n> +\n> +\ttest_expect_success \"$mode checkout\" '\n> +\t\trepo=various_$mode &&\n> +\t\tcp -R various $repo &&\n> +\n> +\t\t# The just copied files have more recent timestamps than their\n> +\t\t# associated index entries. So refresh the cached timestamps\n> +\t\t# to avoid an \"entry not up-to-date\" error from `git checkout`.\n> +\t\t# We only have to do this for the submodules as `git checkout`\n> +\t\t# will already refresh the superproject index before performing\n> +\t\t# the up-to-date check.\n> +\t\t#\n> +\t\tgit -C $repo submodule foreach \"git update-index --refresh\" &&\n> +\n> +\t\tset_checkout_config $workers $threshold &&\n> +\t\ttest_checkout_workers $expected_workers \\\n> +\t\t\tgit -C $repo checkout --recurse-submodules B2 &&\n> +\t\tverify_checkout $repo\n> +\t'\n> +done\n> +\n> +for mode in parallel sequential-fallback\n> +do\n> +\tcase $mode in\n> +\tparallel) workers=2 threshold=0 expected_workers=2 ;;\n> +\tsequential-fallback) workers=2 threshold=100 expected_workers=0 ;;\n> +\tesac\n> +\n> +\ttest_expect_success \"$mode checkout on clone\" '\n> +\t\trepo=various_${mode}_clone &&\n> +\t\tset_checkout_config $workers $threshold &&\n> +\t\ttest_checkout_workers $expected_workers \\\n> +\t\t\tgit clone --recurse-submodules --branch B2 various $repo &&\n> +\t\tverify_checkout $repo\n> +\t'\n> +done\n> +\n> +# Just to be paranoid, actually compare the working trees' contents directly.\n> +test_expect_success 'compare the working trees' '\n> +\trm -rf various_*/.git &&\n> +\trm -rf various_*/*/.git &&\n> +\n> +\t# We use `git diff` instead of `diff -r` because the latter would\n> +\t# follow symlinks, and not all `diff` implementations support the\n> +\t# `--no-dereference` option.\n> +\t#\n> +\tgit diff --no-index various_sequential various_parallel &&\n> +\tgit diff --no-index various_sequential various_parallel_clone &&\n> +\tgit diff --no-index various_sequential various_sequential-fallback &&\n> +\tgit diff --no-index various_sequential various_sequential-fallback_clone\n> +'\n> +\n> +# Currently, each submodule is checked out in a separated child process, but\n> +# these subprocesses must also be able to use parallel checkout workers to\n> +# write the submodules' entries.\n> +test_expect_success 'submodules can use parallel checkout' '\n> +\tset_checkout_config 2 0 &&\n> +\tgit init super &&\n> +\t(\n> +\t\tcd super &&\n> +\t\tgit init sub &&\n> +\t\ttest_commit -C sub A &&\n> +\t\ttest_commit -C sub B &&\n> +\t\tgit submodule add ./sub &&\n> +\t\tgit commit -m sub &&\n> +\t\trm sub/* &&\n> +\t\ttest_checkout_workers 2 git checkout --recurse-submodules .\n> +\t)\n> +'\n> +\n> +test_expect_success 'parallel checkout respects --[no]-force' '\n> +\tset_checkout_config 2 0 &&\n> +\tgit init dirty &&\n> +\t(\n> +\t\tcd dirty &&\n> +\t\tmkdir D &&\n> +\t\ttest_commit D/F &&\n> +\t\ttest_commit F &&\n> +\n> +\t\trm -rf D &&\n> +\t\techo changed >D &&\n> +\t\techo changed >F.t &&\n> +\n> +\t\t# We expect 0 workers because there is nothing to be done\n> +\t\ttest_checkout_workers 0 git checkout HEAD &&\n> +\t\ttest_path_is_file D &&\n> +\t\tgrep changed D &&\n> +\t\tgrep changed F.t &&\n> +\n> +\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n> +\t\ttest_path_is_dir D &&\n> +\t\tgrep D/F D/F.t &&\n> +\t\tgrep F F.t\n> +\t)\n> +'\n> +\n> +test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading dirs' '\n> +\tset_checkout_config 2 0 &&\n> +\tgit init symlinks &&\n> +\t(\n> +\t\tcd symlinks &&\n> +\t\tmkdir D untracked &&\n> +\t\t# Commit 2 files to have enough work for 2 parallel workers\n> +\t\ttest_commit D/A &&\n> +\t\ttest_commit D/B &&\n> +\t\trm -rf D &&\n> +\t\tln -s untracked D &&\n> +\n> +\t\ttest_checkout_workers 2 git checkout --force HEAD &&\n> +\t\t! test -h D &&\n> +\t\tgrep D/A D/A.t &&\n> +\t\tgrep D/B D/B.t\n> +\t)\n> +'\n> +\n> +test_done\n\nI haven't dug into why but these tests fail on AIX (on the fairly easy\nto get access to GCC farm boxes):\n\nE.g. for ./t2080-parallel-checkout-basics.sh:\n \n [...]\n Branch 'B2' set up to track remote branch 'B2' from 'origin'.\n Switched to a new branch 'B2'\n + mkdir m\n + echo m/m\n + 1> m/m\n + git add .\n + git commit -m B2\n [B2 176cae8] B2\n Author: A U Thor \n 19 files changed, 17 insertions(+), 17 deletions(-)\n create mode 100644 a\n delete mode 100644 a/a\n delete mode 100644 b\n create mode 100644 b/b\n create mode 120000 c\n delete mode 100644 c/c\n delete mode 120000 d\n create mode 100644 d/d\n rename f => e (100%)\n delete mode 100644 e/e\n create mode 100644 f/f\n mode change 160000 => 120000 g\n mode change 120000 => 160000 h\n delete mode 100644 l/l\n create mode 100644 m/m\n + git checkout --recurse-submodules B1\n Switched to branch 'B1'\n ok 1 - setup repo for checkout with various types of changes\n \n expecting success of 2080.2 'sequential checkout':\n repo=various_$mode &&\n cp -R various $repo &&\n \n # The just copied files have more recent timestamps than their\n # associated index entries. So refresh the cached timestamps\n # to avoid an \"entry not up-to-date\" error from `git checkout`.\n # We only have to do this for the submodules as `git checkout`\n # will already refresh the superproject index before performing\n # the up-to-date check.\n #\n git -C $repo submodule foreach \"git update-index --refresh\" &&\n \n set_checkout_config $workers $threshold &&\n test_checkout_workers $expected_workers \\\n git -C $repo checkout --recurse-submodules B2 &&\n verify_checkout $repo\n \n + repo=various_sequential\n + cp -R various various_sequential\n + git -C various_sequential submodule foreach git update-index --refresh\n Entering 'f'\n Entering 'g'\n Entering 'k'\n + set_checkout_config 1 0\n + test_checkout_workers 0 git -C various_sequential checkout --recurse-submodules B2\n error: Your local changes to the following files would be overwritten by checkout:\n d\n h\n j\n Please commit your changes or stash them before you switch branches.\n Aborting\n error: last command exited with $?=1\n not ok 2 - sequential checkout\n #\n # repo=various_$mode &&\n # cp -R various $repo &&\n #\n # # The just copied files have more recent timestamps than their\n # # associated index entries. So refresh the cached timestamps\n # # to avoid an \"entry not up-to-date\" error from `git checkout`.\n # # We only have to do this for the submodules as `git checkout`\n # # will already refresh the superproject index before performing\n # # the up-to-date check.\n # #\n # git -C $repo submodule foreach \"git update-index --refresh\" &&\n #\n # set_checkout_config $workers $threshold &&\n # test_checkout_workers $expected_workers \\\n # git -C $repo checkout --recurse-submodules B2 &&\n # verify_checkout $repo\n #\n \nThe test suite has a lot of breakages on AIX that I haven't looked at,\nbut this particular one is new, so I thought I'd send a quick poke about\nit...\n"},{"id":"425588","messageId":"CAHd-oW6X4cwD_yLNFONPnXXUAFPxgDoccv2SOdpeLrqmHCJB4Q@mail.gmail.com","threadId":"55540","inReplyTo":"87eedtmj7w.fsf@evledraar.gmail.com","subject":"Re: AIX failures on parallel checkout (new in v2.32.0-rc*)","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-26T22:01:39Z","receivedAt":"2021-05-26T22:01:55Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Ævar\n\nOn Wed, May 26, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> + test_checkout_workers 0 git -C various_sequential checkout --recurse-submodules B2\n> error: Your local changes to the following files would be overwritten by checkout:\n> d\n> h\n> j\n> Please commit your changes or stash them before you switch branches.\n> Aborting\n\nI requested an account on the GCC farm to debug this, and the problem\nseems to be that AIX's `cp -R` defaults to following symlinks instead\nof copying them. In the above error message, 'd', 'h' and 'j' are all\nsymlinks which were followed when copying the test repo, so these\npaths indeed won't match what's in the index, which makes checkout\nabort.\n\nFortunately, there is a POSIX option to force cp to copy the symlinks:\n'-P'. Adding this flag to the cp invocation at line 117 of t2080 makes\nall parallel checkout tests pass on AIX.\n\nWe also already use `cp -R -P` in the test suite (at t7001), and\nwithout any prereq, so I guess all platforms we are testing git at do\nsupport this flag in cp. I'll send a patch adding the flag at t2080.\n"},{"id":"425592","messageId":"xmqqa6ohayn7.fsf@gitster.g","threadId":"55540","inReplyTo":"CAHd-oW6X4cwD_yLNFONPnXXUAFPxgDoccv2SOdpeLrqmHCJB4Q@mail.gmail.com","subject":"Re: AIX failures on parallel checkout (new in v2.32.0-rc*)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-26T23:00:44Z","receivedAt":"2021-05-26T23:00:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> Hi, Ævar\n>\n> On Wed, May 26, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> + test_checkout_workers 0 git -C various_sequential checkout --recurse-submodules B2\n>> error: Your local changes to the following files would be overwritten by checkout:\n>> d\n>> h\n>> j\n>> Please commit your changes or stash them before you switch branches.\n>> Aborting\n>\n> I requested an account on the GCC farm to debug this, and the problem\n> seems to be that AIX's `cp -R` defaults to following symlinks instead\n> of copying them. In the above error message, 'd', 'h' and 'j' are all\n> symlinks which were followed when copying the test repo, so these\n> paths indeed won't match what's in the index, which makes checkout\n> abort.\n>\n> Fortunately, there is a POSIX option to force cp to copy the symlinks:\n> '-P'. Adding this flag to the cp invocation at line 117 of t2080 makes\n> all parallel checkout tests pass on AIX.\n>\n> We also already use `cp -R -P` in the test suite (at t7001), and\n> without any prereq, so I guess all platforms we are testing git at do\n> support this flag in cp. I'll send a patch adding the flag at t2080.\n\nThanks for a quick analysis. Very much appreciated.\n\n00764ca1 (test: fix t7001 cp to use POSIX options, 2014-04-11) only\nmentions that \"-R -P -p\" is used now in place of \"-a\" that we used\nto use (the latter of which is not POSIX). Please highlight why we\nwant -P in the log message (i.e. we want to copy the link in these\ntests because ...) for the fixing commit.\n\nThanks.\n"}]}